-
Notifications
You must be signed in to change notification settings - Fork 171
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
Reset 'seen' flag in log watcher after check #8686
Reset 'seen' flag in log watcher after check #8686
Conversation
The change looks good to me. Would you update the documentation to describe that |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8686 +/- ##
=======================================
Coverage 60.36% 60.36%
=======================================
Files 372 372
Lines 38325 38325
=======================================
Hits 23136 23136
Misses 15189 15189 ☔ View full report in Codecov by Sentry. |
Thanks for opening this PR. Alternatively, do you think documenting that the |
I added some documentation. Personally, I've found that single-assert-per-test is unrealistic when working with science functions. There's often a lot of setup and preprocessing that has to happen before a function can run, and sometimes the functions themselves take a long time to run. It would make unit tests take a lot longer if we could only test a single condition for each test. |
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!
The expensive setup and run can be put in a fixture. Here's one example in the jwst regression tests.
https://github.com/spacetelescope/jwst/blob/master/jwst/regtest/test_fgs_image2.py
Small PR to make LogWatcher reusable for multiple messages.
Using this helper to develop tests for #8669, I found it's convenient to use the same watcher to check for different messages, but the flag needs to be reset between calls. I can do it manually, but that's failure prone. With this change, I can do something like:
Checklist for PR authors (skip items if you don't have permissions or they are not applicable)
added entry inCHANGES.rst
within the relevant release sectionupdated or added relevant testsupdated relevant documentationran regression tests, post a link to the Jenkins job below.How to run regression tests on a PR
Make sure the JIRA ticket is resolved properly