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

TST : move test before others to prevent errors #2230

Closed
wants to merge 4 commits into from

Conversation

pubpub-zz
Copy link
Collaborator

detected in #2228

@pubpub-zz
Copy link
Collaborator Author

@stefan6419846
Based on your analysis I've simply moved the test at very first. This makes it work

@pubpub-zz
Copy link
Collaborator Author

@MartinThoma
this could be an alternative to #2229
up to you

@MartinThoma
Copy link
Member

Ah, I didn't notice that when I merged. I'll check :-)

@MartinThoma
Copy link
Member

Thanks for the hint!

@pubpub-zz pubpub-zz marked this pull request as ready for review October 1, 2023 08:11
@codecov
Copy link

codecov bot commented Oct 1, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (30db3ef) 94.36% compared to head (df2171c) 94.43%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2230      +/-   ##
==========================================
+ Coverage   94.36%   94.43%   +0.06%     
==========================================
  Files          43       43              
  Lines        7591     7595       +4     
  Branches     1498     1500       +2     
==========================================
+ Hits         7163     7172       +9     
+ Misses        264      260       -4     
+ Partials      164      163       -1     
Files Coverage Δ
pypdf/_encryption.py 96.65% <100.00%> (+0.81%) ⬆️

... and 1 file with indirect coverage changes

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

@stefan6419846
Copy link
Collaborator

While this might work for this specific case, I still do not think that this is a clean solution either, as all code should be side-effect free ideally. As soon as I would use the features of pytest-randomly for example, the tests would become unstable again.

@MartinThoma
Copy link
Member

I agree, the order of tests should not matter. I'm closing this PR as we solved the issue via #2229. Thank you for jumping in that quickly again @pubpub-zz an proposing a solution 🙏

@MartinThoma
Copy link
Member

@stefan6419846 I'm open for the idea of addng pytest-xdist :-) That would speed up our CI wall clock time + have a similar effect t pytest-randomly

@stefan6419846
Copy link
Collaborator

@MartinThoma I am not sure whether we can actually deploy pytest-xdist here without breaking stuff, at least as long as we are re-using files from the PDF caching mechanism. We could have (potential) sporadic failures due to simultaneous file operations unless we can ensure that all operations on a file are atomic.

@MartinThoma
Copy link
Member

I see the argument for writing the file, e.g. that a a file could be partially written to the cache and another process tries to read it before it is completely written.

download_test_pdfs is executed by CI before any tests and should solve that issue (but we still would need to add all files there).

Do you think parallel reads are an issue?

@stefan6419846
Copy link
Collaborator

No, I only see parallel downloads/writes of the same files as an issue (maybe leading to subsequent read issues), while pure read operations on existing files or files only used once anyway should work.

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.

4 participants