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

chore:add,update prints in leaf/branch_stage tests #566

Conversation

gabriele-0201
Copy link
Contributor

@gabriele-0201 gabriele-0201 commented Nov 18, 2024

printlns aims to simplify debugging in case of a bug found in the update logic A future pr will contain a refactor of those prints to be part of a dump if the test fails.

edit. it's not worth adding complexity to print things on CI if the test is easily reproducible locally

Copy link
Contributor Author

gabriele-0201 commented Nov 18, 2024

@gabriele-0201 gabriele-0201 force-pushed the gm_leaf_stage_binary_search_follow_up branch from eb3cf96 to c76d252 Compare November 19, 2024 08:46
@gabriele-0201 gabriele-0201 force-pushed the gm_quickcheck_tests_add_printlns branch from 14c01c5 to 21d5177 Compare November 19, 2024 08:46
Copy link
Contributor

@rphmeier rphmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like having printlns committed, even in tests. Often, the prints that are best to add are very dependent on the issue you're debugging. I would prefer to reject this change, even if it means adding the prints redundantly when debugging.

Copy link
Contributor

@rphmeier rphmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception being when the test is random, and we need to somehow save the seed. But that should be it!

@gabriele-0201
Copy link
Contributor Author

No problem rejecting this. I also don't like printlns on the master branch.
However, since they were added in this commit, I simply wanted to make them symmetric between the leaf and branch stages, but this seems to be a total misunderstanding. I will modify #570 and remove the unnecessary printlns there

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