-
Notifications
You must be signed in to change notification settings - Fork 768
Test: Check for missing coordinates in LAMMPS DumpReader #5206
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
base: develop
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5206 +/- ##
===========================================
- Coverage 92.72% 92.71% -0.02%
===========================================
Files 180 180
Lines 22475 22475
Branches 3190 3190
===========================================
- Hits 20841 20838 -3
- Misses 1177 1179 +2
- Partials 457 458 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This seems OK - can you please run Black formatting to get the linting test to pass? |
Thanks! @BradyAJohnston I've downgraded Black to the required version (v24) and applied the formatting fixes. Everything should be clean now. |
| ) | ||
|
|
||
| # reader should raise ValueError because coordinates are missing | ||
| with pytest.raises(ValueError, match="No coordinate information"): |
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.
This only tests the "No coordinate information", while according to #5127 and https://github.com/MDAnalysis/mdanalysis/pull/5053/changes#r2427645640 the original issue was not having a test catching the wrong f-string. Could you please update the match to check that?
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 @RMeli , You were absolutely right.
I realized that while I was writing the test, the actual source code in LAMMPS.py was still raising the generic error message, which is why the strict regex check would have failed.
I have pushed a new commit that covers everything:
Source Code: Updated MDAnalysis/coordinates/LAMMPS.py to correctly include the filename in the ValueError (this ensures the f-string logic is actually active).
New Test: Updated test_missing_coords_error to use the strict regex match=r"No coordinate information found in .*bad_lammps.dump", verifying the filename is present.
Legacy Test: Updated the existing test_no_coordinate_info to match this new error message format so that the test suite remains green.
Verified locally, and all 157 tests passed.
391184a to
a482c5b
Compare
a482c5b to
54683be
Compare
Fixes #5127
Changes made in this Pull Request:
test_missing_coords_errorintest_lammps.py.LAMMPS.DumpReadercorrectly raises aValueErrorwhen the input file lacks coordinate columns (x, y, z).tmpdirto generate a temporary dummy file for the test execution.LLM / AI generated code disclosure
LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: no
PR Checklist
package/CHANGELOGfile updated?package/AUTHORS? (If it is not, add it!)Developers Certificate of Origin
I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.
📚 Documentation preview 📚: https://mdanalysis--5206.org.readthedocs.build/en/5206/