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

DEV: Use pytest-xdist #2254

Merged
merged 9 commits into from
Oct 17, 2023
Merged

DEV: Use pytest-xdist #2254

merged 9 commits into from
Oct 17, 2023

Conversation

MartinThoma
Copy link
Member

@MartinThoma MartinThoma commented Oct 14, 2023

Before (example):

image

After (this PR):

image

and a second run:

image

@codecov
Copy link

codecov bot commented Oct 14, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (448c379) 94.44% compared to head (792ea4b) 94.44%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2254   +/-   ##
=======================================
  Coverage   94.44%   94.44%           
=======================================
  Files          43       43           
  Lines        7634     7634           
  Branches     1506     1506           
=======================================
  Hits         7210     7210           
  Misses        262      262           
  Partials      162      162           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MartinThoma
Copy link
Member Author

MartinThoma commented Oct 15, 2023

It still seems not to combine the coverage data correctly. I guess the reason is a race-condition between the different python versions.

@stefan6419846
Copy link
Collaborator

stefan6419846 commented Oct 15, 2023

This appears to be a naming issue: Looking at https:/py-pdf/pypdf/actions/runs/6516162435/job/17699481914, we have

Run actions/download-artifact@v3
Starting download for coverage-data
Directory structure has been setup for the artifact
Total number of files that will be downloaded: 7
Total file count: 7 ---- Processed file #2 (28.5%)
Total file count: 7 ---- Processed file #6 (85.7%)
Artifact coverage-data was downloaded to /home/runner/work/pypdf/pypdf
Artifact download has finished successfully

Run python -m coverage combine
Combined data file .coverage.fv-az460-300.3519.834511
Combined data file .coverage.fv-az670-727.3495.027472
Combined data file .coverage.fv-az296-65.3484.685834
Combined data file .coverage.fv-az457-547.3483.149504
Combined data file .coverage.fv-az615-884.4343.623930
Combined data file .coverage.fv-az137-215.3683.313624
Combined data file .coverage.fv-az956-684.3600.108483
Wrote XML report to coverage.xml

For https:/py-pdf/pypdf/actions/runs/6523292132/job/17713841210, we get

Run actions/download-artifact@v3
Starting download for coverage-data
Directory structure has been setup for the artifact
Total number of files that will be downloaded: 1
Artifact coverage-data was downloaded to /home/runner/work/pypdf/pypdf
Artifact download has finished successfully

Run python -m coverage xml
Wrote XML report to coverage.xml

instead.

Possible solution: Add prefix or suffix to the coverage database before uploading the artifact to ensure unique names instead of one common name and thus the files overwriting theirselves.

@MartinThoma
Copy link
Member Author

@stefan6419846 I think I fixed the issue :-)

What do you think about the overall PR?

It seems to reduce the CI wall-clock time from ~14min 23s to 9min 20s.

@MartinThoma MartinThoma added the nf-ci Non-functional change: Continuous Integration label Oct 15, 2023
@stefan6419846
Copy link
Collaborator

stefan6419846 commented Oct 15, 2023

While in general the randomness source should be sufficient enough, I am not sure whether we can use a more stable approach there (the current Python version as given by the matrix?) or at least ensure that when combining the coverage/downloading the artifacts we actually have the correct number of files.

As for the runtime: As it does not have any apparent side effects and it does not change anything relevant on the remaining test setup, this at least is some enhancement. Reducing the runtime by roughly 1/3 while using two parallel executors seems to be a little bit worse than expected, but it does not take into account the pure tests runtime, but the general setup, and thus this might be as good as we can get without adding more parallel executors.

@MartinThoma MartinThoma marked this pull request as ready for review October 15, 2023 14:42
@MartinThoma MartinThoma merged commit 8aef1de into main Oct 17, 2023
12 checks passed
@MartinThoma MartinThoma deleted the xdist branch October 17, 2023 18:48
MartinThoma added a commit that referenced this pull request Oct 29, 2023
## What's new

### Security (SEC)
-  Infinite recursion when using PdfWriter(clone_from=reader) (#2264) by @Alexhuszagh

### New Features (ENH)
-  Add parameter to select images to be removed (#2214) by @pubpub-zz

### Bug Fixes (BUG)
-  Correctly handle image mode 1 with FlateDecode (#2249) by @stefan6419846
-  Error when filling a value with parentheses #2268 (#2269) by @KanorUbu
-  Handle empty root outline (#2239) by @pubpub-zz

### Documentation (DOC)
-  Improve merging docs (#2247) by @stefan6419846

### Developer Experience (DEV)
-  Test Python 3.7 with cryptopgraphy provider as well (#2276) by @stefan6419846
-  Run CI with windows-latest (#2258) by @MartinThoma
-  Use pytest-xdist (#2254) by @MartinThoma
-  Attribute correct authors in the release notes (#2246) by @stefan6419846

### Maintenance (MAINT)
-  Apply pre-commit hooks (#2277) by @MartinThoma
-  Update requirements + mypy fixes (#2275) by @MartinThoma
-  Explicitly provide Any for IO generic argument (#2272) by @nilehmann

### Testing (TST)
-  Fix test_image_without_pillow in windows environment (#2257) by @pubpub-zz

### Code Style (STY)
-  Remove unused import by @MartinThoma

[Full Changelog](3.16.4...3.17.0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nf-ci Non-functional change: Continuous Integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants