Add more unit tests #372
Add more unit tests #372
Conversation
for more information, see https://pre-commit.ci
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds comprehensive MPI-backed LAMMPS tests via pylammpsmpi, introducing a new test module for MPI interactions and extending an existing local library test. Coverage includes extraction APIs, gather/scatter, box/global state, thermo, version detection, installed packages, and feature flags. Tests initialize LAMMPS with an input file and ensure proper teardown. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant MPIWrapper as pylammpsmpi (select_cmd)
participant LAMMPS as LAMMPS Core
Tester->>MPIWrapper: init + load in.simple
MPIWrapper->>LAMMPS: initialize, read_data
Note over MPIWrapper,LAMMPS: Setup complete
rect rgba(200,230,255,0.3)
Tester->>MPIWrapper: extract_atom / extract_compute
MPIWrapper->>LAMMPS: query atom/compute data
LAMMPS-->>MPIWrapper: results (scalars, vectors)
MPIWrapper-->>Tester: values and arrays
end
rect rgba(220,255,220,0.3)
Tester->>MPIWrapper: gather_atoms
MPIWrapper->>LAMMPS: gather across ranks
LAMMPS-->>MPIWrapper: concatenated data
Tester->>MPIWrapper: scatter_atoms (mutate subset)
MPIWrapper->>LAMMPS: scatter to ranks
Tester->>MPIWrapper: re-gather to verify
end
rect rgba(255,240,200,0.3)
Tester->>MPIWrapper: extract_box / extract_global
MPIWrapper->>LAMMPS: box/lo-hi queries
MPIWrapper-->>Tester: descriptors and vectors
end
rect rgba(255,220,220,0.3)
Tester->>MPIWrapper: get_thermo, run, get_thermo
MPIWrapper->>LAMMPS: run step(s), read thermo
MPIWrapper-->>Tester: thermo values
end
Tester->>MPIWrapper: version, properties, installed_packages
MPIWrapper->>LAMMPS: metadata queries
MPIWrapper-->>Tester: version ints, flags, package list
Tester-->>MPIWrapper: close
MPIWrapper->>LAMMPS: finalize
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #372 +/- ##
==========================================
- Coverage 82.31% 81.26% -1.05%
==========================================
Files 5 6 +1
Lines 554 790 +236
==========================================
+ Hits 456 642 +186
- Misses 98 148 +50 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
tests/test_pylammpsmpi_local.py (2)
26-30: Avoid hard-coding version lists; centralize to prevent drift across tests and ease updates.This exact list is duplicated in several test modules. Consider hoisting the allowed LAMMPS versions to a single shared constant to keep tests consistent and reduce churn when a new LAMMPS release appears.
Apply this minimal diff here, then import the shared constant (see new-file snippet below):
- self.assertTrue( - self.lmp.version - in [20220623, 20230802, 20231121, 20240207, 20240627, 20240829] - ) + from tests._known import KNOWN_LAMMPS_VERSIONS + self.assertTrue(self.lmp.version in KNOWN_LAMMPS_VERSIONS)Add this helper once for the test suite (new file):
# tests/_known.py KNOWN_LAMMPS_VERSIONS = [20220623, 20230802, 20231121, 20240207, 20240627, 20240829]Optionally, for forward-compatibility, you could accept any version >= min(KNOWN_LAMMPS_VERSIONS) and warn if it’s unseen, but centralizing the list is already a strong improvement.
103-109: Feature-flag assertions may be environment-dependent; add an opt-in strict mode to avoid CI/environment flakiness.JPEG/PNG/FFmpeg support depends on how LAMMPS was built. Keeping strict checks is fine for your CI, but they can fail on downstream users’ setups.
You can make strict checks opt-in via an env var while still validating return types by default:
- self.assertEqual(self.lmp.has_exceptions, True) - self.assertEqual(self.lmp.has_gzip_support, True) - self.assertEqual(self.lmp.has_png_support, True) - self.assertEqual(self.lmp.has_jpeg_support, True) - self.assertEqual(self.lmp.has_ffmpeg_support, False) + strict = os.getenv("PYLAMMPSMPI_STRICT_FEATURES", "1") == "1" + if strict: + self.assertEqual(self.lmp.has_exceptions, True) + self.assertEqual(self.lmp.has_gzip_support, True) + self.assertEqual(self.lmp.has_png_support, True) + self.assertEqual(self.lmp.has_jpeg_support, True) + self.assertEqual(self.lmp.has_ffmpeg_support, False) + else: + for name, value in { + "has_exceptions": self.lmp.has_exceptions, + "has_gzip_support": self.lmp.has_gzip_support, + "has_png_support": self.lmp.has_png_support, + "has_jpeg_support": self.lmp.has_jpeg_support, + "has_ffmpeg_support": self.lmp.has_ffmpeg_support, + }.items(): + with self.subTest(name=name): + self.assertIsInstance(value, bool)tests/test_mpi_backend.py (7)
10-15: Be explicit about disabling log file creation to keep the workspace clean.LAMMPS may write a log file by default. Passing "-log none" makes the test side-effect free.
- cls.lmp = lammps() + cls.lmp = lammps(cmdargs=["-log", "none"])
57-63: Align variable-length handling with other tests to avoid Python/ABI-related flakiness.The local tests accept length in [256, 512] and gated by Python >= 3.11. Here you assert exactly 256, which can intermittently fail depending on the wrapper/array type returned.
Add sys import and relax the check similarly:
@@ - x = select_cmd("extract_variable")(job=self.lmp, funct_args=["fx", "all", 1]) - self.assertEqual(len(x), 256) - self.assertEqual(np.round(x[0], 2), -0.26) + import sys + x = select_cmd("extract_variable")(job=self.lmp, funct_args=["fx", "all", 1]) + if sys.version_info >= (3, 11): + self.assertIn(len(x), [256, 512]) + # Value check remains meaningful regardless of length representation + self.assertEqual(np.round(x[0], 2), -0.26)
86-90: Avoid duplicating known-version lists; centralize for maintainability.Same suggestion as for the local library tests to prevent drift.
- self.assertTrue( - select_cmd("get_version")(job=self.lmp, funct_args=[]) - in [20220623, 20230802, 20231121, 20240207, 20240627, 20240829] - ) + from tests._known import KNOWN_LAMMPS_VERSIONS + self.assertIn(select_cmd("get_version")(job=self.lmp, funct_args=[]), KNOWN_LAMMPS_VERSIONS)
93-101: Use tolerance-based float comparisons for global box values.Exact float equality can be brittle across compilers/MPI builds. A small absolute tolerance keeps intent while avoiding noise.
- self.assertEqual( - select_cmd("extract_global")(job=self.lmp, funct_args=["boxhi"]), - [6.718384765530029, 6.718384765530029, 6.718384765530029], - ) - self.assertEqual( - select_cmd("extract_global")(job=self.lmp, funct_args=["boxlo"]), - [0.0, 0.0, 0.0], - ) + np.testing.assert_allclose( + select_cmd("extract_global")(job=self.lmp, funct_args=["boxhi"]), + [6.718384765530029, 6.718384765530029, 6.718384765530029], + rtol=0.0, + atol=1e-6, + ) + np.testing.assert_allclose( + select_cmd("extract_global")(job=self.lmp, funct_args=["boxlo"]), + [0.0, 0.0, 0.0], + rtol=0.0, + atol=1e-12, + )
119-128: Prefer tolerant checks for thermo values; also consider using the explicit string-commands helper.Thermo floats can differ at the last bit across platforms. Additionally, calling the explicit commands_string helper clarifies intent when sending a single LAMMPS command as a string.
- self.assertEqual( - float(select_cmd("get_thermo")(job=self.lmp, funct_args=["temp"])), - 1.1298532212880312, - ) - select_cmd("command")(job=self.lmp, funct_args="run 0") - self.assertEqual( - float(select_cmd("get_thermo")(job=self.lmp, funct_args=["temp"])), - 1.129853221288031, - ) + self.assertAlmostEqual( + float(select_cmd("get_thermo")(job=self.lmp, funct_args=["temp"])), + 1.1298532212880312, + places=6, + ) + # Send a single LAMMPS command; the dedicated helper avoids ambiguity. + select_cmd("commands_string")(job=self.lmp, funct_args="run 0") + self.assertAlmostEqual( + float(select_cmd("get_thermo")(job=self.lmp, funct_args=["temp"])), + 1.129853221288031, + places=6, + )If you prefer to keep using
command, confirm its signature supports a single string argument in yourpylammpsmpi/mpi/lmpmpi.py(some codebases separatecommand,commands_list, andcommands_stringwith different expectations).
130-136: Relax strict package membership for portability; skip when optional packages aren’t compiled.“MANYBODY” and “KSPACE” are common; “MC” may not be compiled everywhere. Keep the test informative but portable by skipping only the optional part.
- self.assertIsInstance(packages, list) - self.assertIn("MANYBODY", packages) - self.assertIn("KSPACE", packages) - self.assertIn("MC", packages) + self.assertIsInstance(packages, list) + packages_set = set(packages) + self.assertTrue({"MANYBODY", "KSPACE"}.issubset(packages_set)) + if "MC" not in packages_set: + self.skipTest("MC package not installed in this LAMMPS build") + else: + self.assertIn("MC", packages_set)
1-6: Minor: import ordering and missingsysimport for the earlier suggestion.If you adopt the Python-version guard in
test_extract_variable, you’ll needsys. Consider grouping stdlib imports together.-import unittest -import os -import numpy as np +import os +import sys +import unittest +import numpy as np
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/test_mpi_backend.py(1 hunks)tests/test_pylammpsmpi_local.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_pylammpsmpi_local.py (6)
tests/test_mpi_backend.py (2)
test_version(86-90)test_properties(102-117)tests/test_base.py (2)
test_version(112-116)test_properties(125-130)tests/test_concurrent.py (2)
test_version(98-102)test_properties(113-118)pylammpsmpi/wrapper/concurrent.py (6)
version(131-140)has_exceptions(355-364)has_gzip_support(367-376)has_png_support(379-388)has_jpeg_support(391-400)has_ffmpeg_support(403-404)pylammpsmpi/wrapper/base.py (6)
version(22-30)has_exceptions(271-273)has_gzip_support(276-277)has_png_support(280-281)has_jpeg_support(284-285)has_ffmpeg_support(288-289)pylammpsmpi/mpi/lmpmpi.py (5)
has_exceptions(267-268)has_gzip_support(271-272)has_png_support(275-276)has_jpeg_support(279-280)has_ffmpeg_support(283-284)
tests/test_mpi_backend.py (2)
pylammpsmpi/mpi/lmpmpi.py (1)
select_cmd(376-426)tests/test_pylammpsmpi_local.py (12)
setUpClass(15-20)tearDownClass(23-24)test_extract_atom(32-38)test_extract_compute_global(40-42)test_extract_compute_per_atom(44-46)test_gather_atoms(48-56)test_extract_fix(58-60)test_extract_variable(62-68)test_scatter_atoms(70-83)test_extract_box(85-98)test_version(26-30)test_properties(103-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: unittest_old
…and that all tests are passing.
for more information, see https://pre-commit.ci
2 participants
Summary by CodeRabbit