Skip to content

Deprecate PymatgenTest, migrate tests to pytest from unittest #4212

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 49 commits into from
Apr 17, 2025
Merged

Deprecate PymatgenTest, migrate tests to pytest from unittest #4212

merged 49 commits into from
Apr 17, 2025

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Dec 1, 2024 •
edited
Loading

Summary

Warning

  • [NEED CONFIRM] Deprecate PymatgenTest with MatSciTest, set deadline at 2026-01-01

setUp -> setup_method even when not inheriting from TestCase

Test classes that doesn't subclass TestCase and define setUp method would work in pytest, but it's not a native support, so I would still migrate them here:

One thing that might catch users by surprise is that plain setup and teardown methods are not pytest native, they are in fact part of the nose support.
Native pytest support uses setup_method and teardown_method (see Method and function level setup/teardown), so the above should be changed to:

@DanielYang59 DanielYang59 mentioned this pull request Dec 1, 2024
Closed
3 tasks
DanielYang59
DanielYang59 commented Dec 1, 2024
View reviewed changes
tests/command_line/test_gulp_caller.py Outdated
def test_specie_potential(self):
pass

@unittest.expectedFailure
Copy link
Contributor Author

@DanielYang59 DanielYang59 Dec 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I might remove this test as it is hard-coded to check the content of a non-existent file:

pymatgen/tests/command_line/test_gulp_caller.py

Lines 139 to 142 in 31f1e1f

@unittest.expectedFailure
def test_library_line_explicit_path(self):
gin = self.gio.library_line("/Users/mbkumar/Research/Defects/GulpExe/Libraries/catlow.lib")
assert "lib" in gin

@DanielYang59 DanielYang59 force-pushed the real-real-real-migrate-pmg-tests-to-pytest branch from 9c5bc6d to 344a872 Compare December 3, 2024 14:37
@DanielYang59 DanielYang59 force-pushed the real-real-real-migrate-pmg-tests-to-pytest branch from 3ffea5d to ab8bb0e Compare December 4, 2024 14:47
@DanielYang59 DanielYang59 force-pushed the real-real-real-migrate-pmg-tests-to-pytest branch from 9595a2a to 9ae0deb Compare December 4, 2024 15:32
@DanielYang59 DanielYang59 force-pushed the real-real-real-migrate-pmg-tests-to-pytest branch from 95a3e90 to 42f9fb6 Compare December 4, 2024 15:34
DanielYang59
DanielYang59 commented Dec 4, 2024
View reviewed changes
src/pymatgen/util/testing/__init__.py Outdated
@@ -35,8 +35,15 @@
FAKE_POTCAR_DIR = f"{VASP_IN_DIR}/fake_potcars"


class PymatgenTest(TestCase):
"""Extends unittest.TestCase with several assert methods for array and str comparison."""
class MatSciTest:
Copy link
Contributor Author

@DanielYang59 DanielYang59 Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I originally didn't plan to deprecate PymatgenTest in this PR, but it turns out we have to do this here, otherwise tests that subclassing PymatgenTest would still rely on the TestCase and pytest methods like setup_method/class would not be compatible, meaning we have to do another migration after PymatgenTest dropped TestCase inheritance.

So I guess the easiest way is not to drop inheriting TestCase by PymatgenTest, but replacing PymatgenTest with another one that doesn't subclassing TestCase.

I drafted a name here but free feel to comment if you have better ideas (batch rename would be super easy with an IDE so don't worry).

@DanielYang59 DanielYang59 force-pushed the real-real-real-migrate-pmg-tests-to-pytest branch from 71dd429 to def9886 Compare December 12, 2024 05:42
@DanielYang59 DanielYang59 marked this pull request as ready for review December 12, 2024 05:52
@DanielYang59 DanielYang59 changed the title Deprecate PymatgenTest, migrate pymatgen tests to pytest from unittest Deprecate PymatgenTest, migrate tests to pytest from unittest Jan 19, 2025
@shyuep
Copy link
Member

shyuep commented Apr 15, 2025

There seems to be conflicts that need to be adjusted.

@shyuep shyuep enabled auto-merge (squash) April 15, 2025 16:35
@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Apr 15, 2025 •
edited
Loading

Fixed, thanks for reviewing. Looks like COD might be temporarily down thus the failure

auto-merge was automatically disabled April 17, 2025 07:35

Head branch was pushed to by a user without write access

@DanielYang59
Copy link
Contributor Author

@shyuep Can you maybe review this? Merge conflicts solved. Thanks.

@shyuep shyuep merged commit e301acd into materialsproject:master Apr 17, 2025
43 checks passed
@DanielYang59
Copy link
Contributor Author

Thanks

@DanielYang59 DanielYang59 deleted the real-real-real-migrate-pmg-tests-to-pytest branch April 17, 2025 14:45
@esoteric-ephemera esoteric-ephemera mentioned this pull request Apr 17, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shyuep shyuep Awaiting requested review from shyuep shyuep is a code owner

@mkhorton mkhorton Awaiting requested review from mkhorton mkhorton is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Dev/Test] Migrate PymatgenTest from unittest.TestCase to pytest
2 participants
@DanielYang59 @shyuep