Add more tests to improve coverage #201
Conversation
|
Warning Rate limit exceeded@jan-janssen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes add a suite of new unit tests to verify the behavior of key classes. In Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #201 +/- ##
==========================================
+ Coverage 86.40% 88.49% +2.09%
==========================================
Files 4 4
Lines 478 478
==========================================
+ Hits 413 423 +10
+ Misses 65 55 -10 ☔ View full report in Codecov by Sentry. |
![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 (5)
tests/test_units.py (1)
7-14: Enhance test coverage and improve test organization.Consider the following improvements to make the tests more comprehensive and maintainable:
- Split the test method into separate test methods for each label to improve readability and isolation.
- Add test cases for edge cases (e.g., empty array, invalid label).
- Add test cases for other unit systems (e.g., "real", "lj").
- Add docstrings to explain test purpose and expected behavior.
Here's an example of how to refactor the tests:
class TestUnits(unittest.TestCase): - def test_convert_array_to_pyiron_units(self): - uc = UnitConverter(units="metal") - result = uc.convert_array_to_pyiron_units(array=np.array([1.0]), label="energy") - self.assertTrue(np.all(np.equal(result, np.array([1.0])))) - result = uc.convert_array_to_pyiron_units(array=np.array([1.0]), label="energy_tot") - self.assertTrue(np.all(np.equal(result, np.array([1.0])))) - result = uc.convert_array_to_pyiron_units(array=np.array([1.0]), label="time") - self.assertTrue(np.all(np.equal(result, np.array([1000.0])))) + def setUp(self): + """Set up test fixtures.""" + self.uc = UnitConverter(units="metal") + + def test_convert_energy_to_pyiron_units(self): + """Test energy conversion in metal units.""" + result = self.uc.convert_array_to_pyiron_units(array=np.array([1.0]), label="energy") + self.assertTrue(np.all(np.equal(result, np.array([1.0])))) + + def test_convert_energy_tot_to_pyiron_units(self): + """Test total energy conversion in metal units.""" + result = self.uc.convert_array_to_pyiron_units(array=np.array([1.0]), label="energy_tot") + self.assertTrue(np.all(np.equal(result, np.array([1.0])))) + + def test_convert_time_to_pyiron_units(self): + """Test time conversion in metal units.""" + result = self.uc.convert_array_to_pyiron_units(array=np.array([1.0]), label="time") + self.assertTrue(np.all(np.equal(result, np.array([1000.0])))) + + def test_convert_array_edge_cases(self): + """Test edge cases for unit conversion.""" + with self.assertRaises(ValueError): + self.uc.convert_array_to_pyiron_units(array=np.array([]), label="energy") + with self.assertRaises(ValueError): + self.uc.convert_array_to_pyiron_units(array=np.array([1.0]), label="invalid_label") + + def test_convert_array_other_unit_systems(self): + """Test unit conversion in other unit systems.""" + uc_real = UnitConverter(units="real") + result = uc_real.convert_array_to_pyiron_units(array=np.array([1.0]), label="energy") + self.assertTrue(np.all(np.equal(result, np.array([4.184])))) # kcal/mol to eVtests/test_structure.py (4)
24-33: Improve test organization and documentation.Consider the following improvements:
- Split the test method into separate test methods for each test case.
- Add docstrings to explain test purpose and expected behavior.
- Document the origin of the reference vector or calculate it dynamically.
Here's an example of how to refactor the tests:
- def test_unfolding_prism(self): - with self.assertRaises(IndexError): - UnfoldingPrism(cell=[]) - with self.assertRaises(TypeError): - UnfoldingPrism(cell=[[0.0, 0.0, 0.0], [0.0, 0.0, 0.0], [0.0, 0.0, 0.0]]) - structure = bulk("Al", a=4.05) - up = UnfoldingPrism(cell=structure.cell, pbc=[False, False, False]) - up_vec = np.array([float(s) for s in up.get_lammps_prism_str()]) - ref_vec = np.array([2.8637824638, 2.4801083646, 2.3382685902, 1.4318912319, 1.4318912319, 0.8267027882]) - self.assertTrue(np.all(np.isclose(up_vec, ref_vec))) + def test_unfolding_prism_empty_cell(self): + """Test that UnfoldingPrism raises IndexError for empty cell.""" + with self.assertRaises(IndexError): + UnfoldingPrism(cell=[]) + + def test_unfolding_prism_invalid_cell(self): + """Test that UnfoldingPrism raises TypeError for invalid cell structure.""" + with self.assertRaises(TypeError): + UnfoldingPrism(cell=[[0.0, 0.0, 0.0], [0.0, 0.0, 0.0], [0.0, 0.0, 0.0]]) + + def test_unfolding_prism_valid_cell(self): + """Test that UnfoldingPrism returns correct output for valid cell. + + The reference vector represents the expected LAMMPS prism format: + [xhi-xlo, yhi-ylo, zhi-zlo, xy, xz, yz] + """ + structure = bulk("Al", a=4.05) + up = UnfoldingPrism(cell=structure.cell, pbc=[False, False, False]) + up_vec = np.array([float(s) for s in up.get_lammps_prism_str()]) + + # Calculate expected values based on the cell parameters + a = 4.05 / np.sqrt(2) # FCC lattice parameter + ref_vec = np.array([ + a, # xhi-xlo + a * np.sqrt(3)/2, # yhi-ylo + a * np.sqrt(6)/3, # zhi-zlo + a/2, # xy + a/2, # xz + a * np.sqrt(3)/6 # yz + ]) + + self.assertTrue(np.all(np.isclose(up_vec, ref_vec)))
161-164: Add test cases for valid input and improve documentation.The test method only covers the error case. Consider adding test cases for valid input and improving documentation.
Here's an example of how to improve the test:
- def test_lammps_structure(self): - ls = LammpsStructure() - with self.assertRaises(ValueError): - ls.get_lammps_id_dict(el_eam_lst=[]) + def test_lammps_structure_get_id_dict(self): + """Test LammpsStructure.get_lammps_id_dict method.""" + ls = LammpsStructure() + + # Test error case + with self.assertRaises(ValueError): + ls.get_lammps_id_dict(el_eam_lst=[]) + + # Test valid input + result = ls.get_lammps_id_dict(el_eam_lst=["Al", "Fe", "Ni"]) + expected = {"Al": 1, "Fe": 2, "Ni": 3} + self.assertEqual(result, expected)
166-194: Add test cases for different parameter combinations and improve documentation.Consider the following improvements:
- Add docstring explaining test purpose and expected behavior.
- Add test cases for different parameter combinations.
- Document the structure of the reference string.
Here's an example of how to improve the test:
- def test_lammps_structure_header(self): + def test_lammps_structure_header_with_bonds_and_angles(self): + """Test LammpsStructure.lammps_header method with bonds and angles. + + The header should include: + - Number of atoms and atom types + - Number of bonds and bond types + - Number of angles and angle types + - Cell dimensions + - Masses section with atom types + """ structure = bulk("Al", a=4.0, cubic=True) ls = LammpsStructure() output_str = ls.lammps_header( structure=structure, cell_dimensions="3", species_lammps_id_dict={"Al": 1}, nbonds=95, nangles=50, nbond_types=10, nangle_types=18, ) reference_str = [ "Start File for LAMMPS ", "4 atoms ", "1 atom types ", "95 bonds", "50 angles", "10 bond types", "18 angle types", "", "3", "Masses", "", " 1 26.981538 # (Al) ", "", "", ] self.assertEqual(output_str.split("\n"), reference_str) + + def test_lammps_structure_header_without_bonds_and_angles(self): + """Test LammpsStructure.lammps_header method without bonds and angles.""" + structure = bulk("Al", a=4.0, cubic=True) + ls = LammpsStructure() + output_str = ls.lammps_header( + structure=structure, + cell_dimensions="3", + species_lammps_id_dict={"Al": 1}, + ) + reference_str = [ + "Start File for LAMMPS ", + "4 atoms ", + "1 atom types ", + "", + "3", + "Masses", + "", + " 1 26.981538 # (Al) ", + "", + "", + ] + self.assertEqual(output_str.split("\n"), reference_str)
5-5: Remove unused import.The
ase.atoms.Atomsimport is not used in the code.-from ase.atoms import Atoms🧰 Tools
🪛 Ruff (0.8.2)
5-5:
ase.atoms.Atomsimported but unusedRemove unused import:
ase.atoms.Atoms(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/test_structure.py(3 hunks)tests/test_units.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_structure.py
5-5: ase.atoms.Atoms imported but unused
Remove unused import: ase.atoms.Atoms
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: unittest_matrix (windows-latest, 3.12)
- GitHub Check: unittest_matrix (macos-latest, 3.12)
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_structure.py (2)
170-173: Enhance test coverage forLammpsStructure.While testing the error case is good, consider adding tests for:
- Valid input cases with different element lists
- Edge cases (null/undefined values)
- Other methods of the
LammpsStructureclassWould you like me to help generate additional test cases to improve coverage?
175-203: Add more test scenarios for header generation.The test effectively validates header generation, but consider adding tests for:
- Edge cases (zero bonds/angles)
- Invalid input validation
- Different species combinations
Example test scenarios:
def test_lammps_structure_header_edge_cases(self): structure = bulk("Al", a=4.0, cubic=True) ls = LammpsStructure() # Test with zero bonds/angles output_str = ls.lammps_header( structure=structure, cell_dimensions="3", species_lammps_id_dict={"Al": 1}, nbonds=0, nangles=0, nbond_types=0, nangle_types=0, ) # Add assertions... # Test with multiple species output_str = ls.lammps_header( structure=structure, cell_dimensions="3", species_lammps_id_dict={"Al": 1, "Fe": 2, "Cu": 3}, nbonds=10, nangles=5, nbond_types=2, nangle_types=1, ) # Add assertions...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/test_structure.py(3 hunks)tests/test_units.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_units.py
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_structure.py
5-5: ase.atoms.Atoms imported but unused
Remove unused import: ase.atoms.Atoms
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: unittest_matrix (windows-latest, 3.12)
- GitHub Check: unittest_matrix (macos-latest, 3.12)
🔇 Additional comments (1)
tests/test_structure.py (1)
24-42: Well-structured test with good coverage!The test method effectively covers error cases and validates the output against a reference vector. Good job on following testing best practices by:
- Testing error conditions first
- Using appropriate assertions
- Including a comprehensive validation case
tests/test_structure.py
Outdated
| @@ -2,10 +2,13 @@ | |||
| import numpy as np | |||
| import os | |||
| from shutil import rmtree | |||
| from ase.atoms import Atoms | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify if the Atoms import is needed.
The Atoms import appears to be unused. However, it might be required by the bulk function from ase.build. Please verify if this import can be safely removed.
🏁 Script executed:
#!/bin/bash
# Description: Check if Atoms is used by the bulk function
# Test: Search for Atoms usage in bulk function definition
ast-grep --pattern 'from ase.atoms import Atoms' && ast-grep --pattern 'def bulk'Length of output: 191
Remove the unused Atoms import in tests/test_structure.py.
- The
Atomsimport on line 5 is not used anywhere in this file. - Our verification did not find any occurrences of the function
bulk(or any alternative indirect usage) in the repository that would justify keeping this import. - If there’s no intended indirect use (e.g., via
ase.build.bulkor similar mechanisms), please remove the import to clean up the file.
🧰 Tools
🪛 Ruff (0.8.2)
5-5: ase.atoms.Atoms imported but unused
Remove unused import: ase.atoms.Atoms
(F401)
for more information, see https://pre-commit.ci
# Conflicts: # tests/test_structure.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_structure.py (2)
174-177: Consider adding more test cases.While testing the empty list case is good, consider adding more test cases to verify:
- Valid element list returns expected mapping
- Invalid element types raise appropriate errors
- Duplicate elements are handled correctly
179-207: Improve test maintainability.Consider these improvements:
- Use a heredoc or multiline string for the reference string instead of a list
- Add comments explaining the significance of the test values (95 bonds, 50 angles, etc.)
- Consider parameterizing the test to verify different combinations of inputs
Example refactor:
- reference_str = [ - "Start File for LAMMPS ", - "4 atoms ", - "1 atom types ", - "95 bonds", - "50 angles", - "10 bond types", - "18 angle types", - "", - "3", - "Masses", - "", - " 1 26.981538 # (Al) ", - "", - "", - ] - self.assertEqual(output_str.split("\n"), reference_str) + reference_str = """Start File for LAMMPS +4 atoms +1 atom types +95 bonds +50 angles +10 bond types +18 angle types + +3 +Masses + + 1 26.981538 # (Al) + + +""" + self.assertEqual(output_str, reference_str)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_structure.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_structure.py
5-5: ase.atoms.Atoms imported but unused
Remove unused import: ase.atoms.Atoms
(F401)
🪛 GitHub Actions: Pipeline
tests/test_structure.py
[error] 26-26: numpy.linalg.LinAlgError: 1-dimensional array given. Array must be at least two-dimensional.
🔇 Additional comments (1)
tests/test_structure.py (1)
5-5: Remove the unusedAtomsimport.The
Atomsimport appears to be unused in this file.🧰 Tools
🪛 Ruff (0.8.2)
5-5:
ase.atoms.Atomsimported but unusedRemove unused import:
ase.atoms.Atoms(F401)
tests/test_structure.py
Outdated
| def test_unfolding_prism(self): | ||
| with self.assertRaises(IndexError): | ||
| UnfoldingPrism(cell=np.array([])) | ||
| with self.assertRaises(np.linalg.LinAlgError): | ||
| UnfoldingPrism( | ||
| cell=np.array([[0.0, 0.0, 0.0], [0.0, 0.0, 0.0], [0.0, 0.0, 0.0]]) | ||
| ) | ||
| with self.assertRaises(TypeError): | ||
| UnfoldingPrism(cell=[[0.0, 1.0], [1.0, 0.0], [0.0, 0.0]]) | ||
| structure = bulk("Al", a=4.05) | ||
| up = UnfoldingPrism(cell=structure.cell, pbc=[False, False, False]) | ||
| up_vec = np.array([float(s) for s in up.get_lammps_prism_str()]) | ||
| ref_vec = np.array( | ||
| [ | ||
| 2.8637824638, | ||
| 2.4801083646, | ||
| 2.3382685902, | ||
| 1.4318912319, | ||
| 1.4318912319, | ||
| 0.8267027882, | ||
| ] | ||
| ) | ||
| self.assertTrue(np.all(np.isclose(up_vec, ref_vec))) |
There was a problem hiding this comment.
Fix the empty cell test case.
The empty cell test case is failing because np.array([]) creates a 1-dimensional array, but UnfoldingPrism expects a 2-dimensional array.
Apply this diff to fix the test:
- UnfoldingPrism(cell=np.array([]))
+ UnfoldingPrism(cell=np.array([[], [], []]))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_unfolding_prism(self): | |
| with self.assertRaises(IndexError): | |
| UnfoldingPrism(cell=np.array([])) | |
| with self.assertRaises(np.linalg.LinAlgError): | |
| UnfoldingPrism( | |
| cell=np.array([[0.0, 0.0, 0.0], [0.0, 0.0, 0.0], [0.0, 0.0, 0.0]]) | |
| ) | |
| with self.assertRaises(TypeError): | |
| UnfoldingPrism(cell=[[0.0, 1.0], [1.0, 0.0], [0.0, 0.0]]) | |
| structure = bulk("Al", a=4.05) | |
| up = UnfoldingPrism(cell=structure.cell, pbc=[False, False, False]) | |
| up_vec = np.array([float(s) for s in up.get_lammps_prism_str()]) | |
| ref_vec = np.array( | |
| [ | |
| 2.8637824638, | |
| 2.4801083646, | |
| 2.3382685902, | |
| 1.4318912319, | |
| 1.4318912319, | |
| 0.8267027882, | |
| ] | |
| ) | |
| self.assertTrue(np.all(np.isclose(up_vec, ref_vec))) | |
| def test_unfolding_prism(self): | |
| with self.assertRaises(IndexError): | |
| - UnfoldingPrism(cell=np.array([])) | |
| + UnfoldingPrism(cell=np.array([[], [], []])) | |
| with self.assertRaises(np.linalg.LinAlgError): | |
| UnfoldingPrism( | |
| cell=np.array([[0.0, 0.0, 0.0], [0.0, 0.0, 0.0], [0.0, 0.0, 0.0]]) | |
| ) | |
| with self.assertRaises(TypeError): | |
| UnfoldingPrism(cell=[[0.0, 1.0], [1.0, 0.0], [0.0, 0.0]]) | |
| structure = bulk("Al", a=4.05) | |
| up = UnfoldingPrism(cell=structure.cell, pbc=[False, False, False]) | |
| up_vec = np.array([float(s) for s in up.get_lammps_prism_str()]) | |
| ref_vec = np.array( | |
| [ | |
| 2.8637824638, | |
| 2.4801083646, | |
| 2.3382685902, | |
| 1.4318912319, | |
| 1.4318912319, | |
| 0.8267027882, | |
| ] | |
| ) | |
| self.assertTrue(np.all(np.isclose(up_vec, ref_vec))) |
🧰 Tools
🪛 GitHub Actions: Pipeline
[error] 26-26: numpy.linalg.LinAlgError: 1-dimensional array given. Array must be at least two-dimensional.
Summary by CodeRabbit
UnfoldingPrismandLammpsStructureclasses to validate error handling and output.UnitConverterto validate unit conversions from different labels.