Skip to content

Conversation

@IAyaanHere
Copy link
Contributor

@IAyaanHere IAyaanHere commented Jan 14, 2026

Fixes #5127

Changes made in this Pull Request:

  • Added a new test case test_missing_coords_error in test_lammps.py.
  • Verified that LAMMPS.DumpReader correctly raises a ValueError when the input file lacks coordinate columns (x, y, z).
  • Uses tmpdir to 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

  • Issue raised/referenced?
  • Tests updated/added?
  • Documentation updated/added?
  • package/CHANGELOG file updated?
  • Is your name in package/AUTHORS? (If it is not, add it!)
  • LLM/AI disclosure was updated.

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/

@codecov
Copy link

codecov bot commented Jan 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.71%. Comparing base (9cbc8ea) to head (54683be).
⚠️ Report is 1 commits behind head on develop.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@BradyAJohnston
Copy link
Member

This seems OK - can you please run Black formatting to get the linting test to pass?

@BradyAJohnston BradyAJohnston self-assigned this Jan 14, 2026
@IAyaanHere
Copy link
Contributor Author

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"):
Copy link
Member

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?

Copy link
Contributor Author

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.

@IAyaanHere IAyaanHere force-pushed the test-lammps-error branch 2 times, most recently from 391184a to a482c5b Compare January 16, 2026 18: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.

missing test for LAMMPS DumpReader for ValueError when missing coordinates

3 participants