Skip to content
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

Fix assert_str_content_equal, add tests for testing utils #4205

Merged
merged 19 commits into from
Dec 11, 2024

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Nov 29, 2024

Summary

  • Fix assert_str_content_equal not raising AssertionError
  • Add unit tests for util.testing as it's intended for public usage (and quite some external code use PymatgenTest)
  • More human-readable error messages for assert methods in PymatgenTest
  • [NEED CONFIRM] Simplify single-file module testing/__init__.py to testing.py

Warning

[NEED CONFIRM] Simplify single-file module testing/__init__.py to testing.py (Is this intended for some purpose?), I don't think this would be breaking (though the definition of MODULE_DIR would change from util/testing to util, but no code import this AFAIK as the util/testing has nothing else, I believe it's intended for STRUCTURES_DIR and I have updated this)


Fix assert_str_content_equal not raising AssertionError

Current implementation would not raise AssertionError when two strings are not equal (it only returns a bool):

def assert_str_content_equal(actual, expected):
"""Test if two strings are equal, ignoring things like trailing spaces, etc."""
strip_whitespace = {ord(c): None for c in string.whitespace}
return actual.translate(strip_whitespace) == expected.translate(strip_whitespace)

And current usage of it take the pattern of self.assert_str_content_equal without assert.

Several issues found:

@DanielYang59 DanielYang59 changed the title Migrate PymatgenTest and tests to pytest Add tests for testing utils, fix assert_str_content_equal Nov 29, 2024
from pymatgen.core import Structure
from pymatgen.util.typing import PathLike

_MODULE_DIR: Path = Path(__file__).absolute().parent
Copy link
Contributor Author

@DanielYang59 DanielYang59 Nov 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

[NEED CONFIRM] make MODULE_DIR private, as the util.testing directory contains nothing else other than__init__.py, I guess it's used to define STRUCTURES_DIR only:

MODULE_DIR = Path(__file__).absolute().parent
STRUCTURES_DIR = MODULE_DIR / ".." / "structures"

@DanielYang59 DanielYang59 changed the title Add tests for testing utils, fix assert_str_content_equal Fix assert_str_content_equal, add tests for testing utils Nov 30, 2024
json_str = json.dumps(obj.as_dict(), cls=MontyEncoder)
round_trip = json.loads(json_str, cls=MontyDecoder)
if not issubclass(type(round_trip), type(obj)):
raise TypeError(f"The reconstructed {round_trip.__class__.__name__} object is not a subclass of {obj_name}")
Copy link
Contributor Author

@DanielYang59 DanielYang59 Nov 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More readable error message

Before: <class 'util.test_testing.TestAssertMSONable.test_not_round_trip.<locals>.NotAKpoints'> != <class 'pymatgen.io.vasp.inputs.Kpoints'>

Now: The reconstructed NotAKpoints object is not a subclass of Kpoints

@DanielYang59 DanielYang59 force-pushed the migrate-to-pytest branch 2 times, most recently from 6126945 to d409b13 Compare November 30, 2024 04:21
@DanielYang59 DanielYang59 marked this pull request as ready for review November 30, 2024 04:26
@shyuep shyuep merged commit 7837761 into materialsproject:master Dec 11, 2024
42 of 43 checks passed
@DanielYang59 DanielYang59 deleted the migrate-to-pytest branch December 11, 2024 02:31
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.

2 participants