Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forest snapshot tests #80

Merged
merged 1 commit into from
Nov 3, 2023
Merged

Forest snapshot tests #80

merged 1 commit into from
Nov 3, 2023

Conversation

edongashi
Copy link
Member

Instead of comparing huge trees manually, I made assertions that hashes are identical with F#.

@edongashi edongashi force-pushed the edon/tests branch 2 times, most recently from 765c18b to 26ba8e3 Compare November 2, 2023 18:51
@edongashi
Copy link
Member Author

edongashi commented Nov 3, 2023

Cristian what do you think about this idea? I want tests for consistency across all stages...

These hashes match with syndiffix #27

@cristianberneanu
Copy link
Contributor

cristianberneanu commented Nov 3, 2023

Cristian what do you think about this idea? I want tests for consistency across all stages...

These hashes match with syndiffix diffix/syndiffix#27

I think they are very opaque and hard to debug. Even worse, they are also tied to the code from another repository.

As you said, they will catch some bugs, like when trying to optimize something without changing behavior. But any change in behavior means updating a hash value blindly.

Ideally, data would be generated dynamically (I know, easier said than done), it should be smaller and only 2 or 3 columns should be used, so we can have a chance to understand the resulting trees.

Besides these, the biggest improvement for the current version that I see would be to drop the hashing and include the tree-to-string dumps in a json/txt file, so we can diff it between different commits.

@edongashi
Copy link
Member Author

Ok, I will revisit after I actually catch what is causing our issues. Forests look OK in any case.

Ideally, data would be generated dynamically

I'm trying to do something like this for stitching. So far it looks like it's working as expected, which leaves us with harvesting and microdata.

@edongashi edongashi force-pushed the edon/tests branch 3 times, most recently from 3c43818 to bacdef8 Compare November 3, 2023 15:58
@edongashi
Copy link
Member Author

edongashi commented Nov 3, 2023

Attempt nr. 2 with a much simpler file. It might even be a little too simple to be of use. But we can follow this pattern with particular test cases in the future. For now I leave this here. When I get the chance I will make similar changes to dep. measure.

@edongashi edongashi merged commit f82999d into master Nov 3, 2023
@edongashi edongashi deleted the edon/tests branch November 3, 2023 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants