-
Notifications
You must be signed in to change notification settings - Fork 652
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
Addition of pytest case coverage of backend
and AnalysisBase.run()
using different n_workers
values
#4768
base: develop
Are you sure you want to change the base?
Conversation
Added case of backend and AnalysisBase.run() having different worker numbers
Added Changelog for the pytest cover case of Backend and AnalysisBase having different n_worker values
typo adjust
Hello @talagayev! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2024-10-28 09:08:04 UTC |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4768 +/- ##
========================================
Coverage 93.66% 93.67%
========================================
Files 177 177
Lines 21726 21726
Branches 3052 3052
========================================
+ Hits 20349 20351 +2
+ Misses 930 929 -1
+ Partials 447 446 -1 ☔ View full report in Codecov by Sentry. |
PEP fixes
PEP fix
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.
LGTM thanks @talagayev
removed line due to duplication
How is it now the case, would this and the other PR, which also only adresses a pytest be currently frozen until the release of 2.8.0 and merged after the release of 2.8.0? |
Happy to help :) |
I believe we are in a code freeze for now, can be merged if we like but I will leave at discretion of release manager, @IAlibay. |
If it's not on the 2.8 milestone, I would prefer it if we didn't merge it until after the release. |
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.
Same as the other PR.
removed changelog change
Yup sounds good :) |
Fixes #4649
Adds the coverage of the
ValueError
case, wherebackend
andAnalysisBase.run()
havedifferent values for
n_workers
Changes made in this Pull Request:
test_n_workers_conflict_raises_value_error
intest_base.py
to cover cases, whereboth
backend
andAnalysisBase.run()
have different values ofn_workers
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4768.org.readthedocs.build/en/4768/