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

Schedule PymatgenTest for migration from unittest to pytest #4209

Closed

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Nov 30, 2024

Summary

Schedule PymatgenTest for migration after a grace period

  • We cannot migrate PymatgenTest directly as it's public and quite some external code use PymatgenTest, added a grace period until 2026-01-01 and let downstream code finish their migration first (get rid of some unittest-specific functions), as pytest is not compatible with some unittest functions
  • OR, do we want to deprecated PymatgenTest with a more general MatSciTest (or some other name) as it's intended to be used by other packages?
  • Issue migration warning, and test methods specific to TestCase such that we know potential side effect, also provide a migration guide (not really our job, but at least provide links to some resources), ruff does have rules (for example PT009) but we cannot trust this as not every project enables ruff

Super brief migration guide

@@ -103,5 +103,6 @@ def test_mcsqs_caller_runtime_error(self):
struct.replace_species({"Ti": {"Ti": 0.5, "Zr": 0.5}, "Zr": {"Ti": 0.5, "Zr": 0.5}})
struct.replace_species({"Pb": {"Ti": 0.2, "Pb": 0.8}})
struct.replace_species({"O": {"F": 0.8, "O": 0.2}})
with pytest.raises(RuntimeError, match="mcsqs exited before timeout reached"):
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.

This test is failing as RuntimeError with a different message is raised:

>       with pytest.raises(RuntimeError, match="mcsqs exited before timeout reached"):
E       AssertionError: Regex pattern did not match.
E        Regex: 'mcsqs exited before timeout reached'
E        Input: 'mcsqs did not generate output files, is search_time sufficient or are number of instances too high?'

But looks like both exceptions are very similar (all pointing to timeout) so I would just change the error message here:

try:
for process in mcsqs_find_sqs_processes:
process.communicate(timeout=search_time * 60)
if instances and instances > 1:
process = Popen(["mcsqs", "-best"])
process.communicate()
if os.path.isfile("bestsqs.out") and os.path.isfile("bestcorr.out"):
return _parse_sqs_path(".")
raise RuntimeError("mcsqs exited before timeout reached")
except TimeoutExpired:
for process in mcsqs_find_sqs_processes:
process.kill()
process.communicate()
# Find the best sqs structures
if instances and instances > 1:
if not os.path.isfile("bestcorr1.out"):
raise RuntimeError(
"mcsqs did not generate output files, "
"is search_time sufficient or are number of instances too high?"
)

@DanielYang59 DanielYang59 force-pushed the real-migrate-pytest branch 4 times, most recently from 6a42d2b to 4d075fa Compare December 1, 2024 10:07
@DanielYang59 DanielYang59 changed the title Migrate pymatgen tests to pytest, schedule PymatgenTest for migration Schedule PymatgenTest for migration from unittest to pytest Dec 1, 2024
@DanielYang59
Copy link
Contributor Author

I would reopen this after #4212 is reviewed/merged as I opened too many PRs for me to track

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.

1 participant