-
Notifications
You must be signed in to change notification settings - Fork 35
Fix test to check if we are printing --allow-partial when a user uses --allow-partial
#1564
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1564 +/- ##
==========================================
- Coverage 95.25% 93.21% -2.05%
==========================================
Files 172 172
Lines 14492 14492
==========================================
- Hits 13805 13508 -297
- Misses 687 984 +297
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@atravitz I am a bit confused by this regex, what are we to do with this test? I feel like So then when we get a warning like I don't think At least that is what I took away from a quick look at this, I could be totally wrong! |
atravitz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for catching this! was this causing an error, or did you find it another way?
|
I saw the warning about the escape syntax when I was working on an unrelated thing and you know how I like a good investigation on an esoteric code path! This fixes the escape warning, but what are we trying to test with this test? |
|
|
||
| def test_missing_leg_allow_partial_(self, results_paths_serial_missing_legs: str): | ||
| runner = CliRunner() | ||
| # we *dont* want the suggestion to use --allow-partial if the user already used it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikemhenry is this comment unclear? I added this test because when we ran with --allow-partial, it was still adding the you can use --allow-partial to stdout, which I think is confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed your earlier comment before, and now I understand why this test is insufficient. Is your suggestion that we assert that you can use --allow-partial is not in stdout?
This test used to use pytest.warns to check if we printed a string to the terminal. This isn't the right way to test if things get printed to terminal since it only checks warnings raised in python-land. The test passed since the regex used to match warnings ended up matching warnings raised since it was a negated character class which matches a single character that is not any of the characters listed inside.
|
No API break detected ✅ |
--allow-partial when a user uses --allow-partial
atravitz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, LGTM!
Checklist
newsentryDevelopers certificate of origin