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

Fix collect_steps #1011

Merged
merged 1 commit into from
Apr 17, 2024
Merged

Conversation

andyleiserson
Copy link
Collaborator

@andyleiserson andyleiserson commented Apr 12, 2024

Ignore random seed message in captured output.

@andyleiserson andyleiserson requested a review from akoshelev April 12, 2024 16:25
@akoshelev
Copy link
Collaborator

so the idea was to print the random seed if test fails. Given that cargo silences the output by default, that seemed like a good way to avoid having noise when tests pass.

Not sure if there is a better way to deal with it though - I am not sure I am a big fan of having a random seed for unit tests, but it is what it is

@andyleiserson
Copy link
Collaborator Author

so the idea was to print the random seed if test fails. Given that cargo silences the output by default, that seemed like a good way to avoid having noise when tests pass.

A more comprehensive solution might be to figure out how to make tracing (at least at/below info level) go to stdout in unit tests. But for now I made collect_steps skip over this message.

I am not sure I am a big fan of having a random seed for unit tests, but it is what it is

We have a fair amount of unit tests that use randomness (including proptests, and others) -- all of which probably deserve a bit more attention to how seeding is done and ensuring output contains enough information to reproduce a failure.

@andyleiserson andyleiserson changed the title Remove a print that confuses collect_steps Fix collect_steps Apr 16, 2024
Copy link

codecov bot commented Apr 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.46%. Comparing base (428d1e9) to head (8808dc5).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1011      +/-   ##
==========================================
- Coverage   90.47%   90.46%   -0.01%     
==========================================
  Files         167      167              
  Lines       23827    23829       +2     
==========================================
  Hits        21558    21558              
- Misses       2269     2271       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@akoshelev
Copy link
Collaborator

A more comprehensive solution might be to figure out how to make tracing (at least at/below info level) go to stdout in unit tests. But for now I made collect_steps skip over this message.

yea we probably want to use test writer
https://docs.rs/tracing-subscriber/latest/tracing_subscriber/fmt/struct.TestWriter.html

@andyleiserson andyleiserson merged commit e113dd2 into private-attribution:main Apr 17, 2024
11 checks passed
@andyleiserson andyleiserson deleted the steps branch April 17, 2024 00:10
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