-
Notifications
You must be signed in to change notification settings - Fork 192
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
Using xdist to run pytest in parallel #6620
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6620 +/- ##
==========================================
+ Coverage 77.51% 77.89% +0.39%
==========================================
Files 560 567 +7
Lines 41444 42180 +736
==========================================
+ Hits 32120 32852 +732
- Misses 9324 9328 +4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very cool, thanks for doing this!
The PR itself looks good, but I have running into errors when I try to run locally. I'll need to investigate a bit.
tests/orm/test_fields.py
Outdated
) | ||
def test_all_node_fields(group, name, data_regression): | ||
@pytest.fixture | ||
def available_entry_points(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is much better. Maybe the name of the fixture should be more specific, since you're only returning node and data entry points. Perhaps node_and_data_entry_points
?
Run with my 16 cores, I found some issues that storage is accessed at the same time.
The typical error of not using tmp_path fixture is:
More tests randomly failed, should be fixed
|
Hi @danielhollas, for tests of creating an archive, it randomly failed because it tries to use the shared DB where may have unsealed data nodes. @pytest.mark.usefixtures('aiida_localhost')
def test_create_all_no_authinfo(tmp_path):
"""Test archive creation that does not include authinfo."""
filename1 = tmp_path / 'export1.aiida'
create_archive(None, filename=filename1, include_authinfos=False)
with get_format().open(filename1, 'r') as archive:
assert archive.querybuilder().append(orm.AuthInfo).count() == 0
@pytest.mark.usefixtures('aiida_localhost')
def test_create_all_with_authinfo(tmp_path):
"""Test archive creation that does include authinfo."""
filename1 = tmp_path / 'export1.aiida'
create_archive(None, filename=filename1, include_authinfos=True)
with get_format().open(filename1, 'r') as archive:
assert archive.querybuilder().append(orm.AuthInfo).count() == 1 EDIT: never mind, I think I can just put the |
I test with both my |
@@ -353,6 +353,7 @@ def test_graph_node_identifiers(self, node_id_type, monkeypatch, file_regression | |||
# The order of certain output lines can be randomly ordered so we split the file in lines, sort, and then join | |||
# them into a single string again. The node identifiers generated by the engine are of the form ``N{pk}`` and | |||
# they also clearly vary, so they are replaced with the ``NODE`` placeholder. | |||
string = '\n'.join(sorted(graph.graphviz.source.strip().split('\n'))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the sort happened first, then the order may depend on the pk number and the test fail with different string where lines order changed. I sort the lines after replace the pk number.
In my workstation, it failed now sometime with SSH banner, but I think it is because the SSH connection is too many and probably I have some sshd limitation setup on the machine. But I think it is fine. |
Running tests in parallel using
pytest-xdist
reduce the time of ci-codes (2 cores).ci-code / tests (3.9)
: ~28m -> ~15mci-code / tests (3.12)
: ~20m -> 12mci-code / presto
: ~11m -> 6mFix all failed tests, see if it possible.
Bring the inconsistent test back to run.