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

Fix failing 3.11 Windows tests due to temporary files #111

Merged
merged 1 commit into from
May 11, 2024

Conversation

Snotpus
Copy link
Collaborator

@Snotpus Snotpus commented May 10, 2024

I couldn't replicate these tests failing locally at all (something specific to the runner?).

I eventually found this pytest issue that discusses
similar(ish) behaviors. This lead me to attempt to catch the PermissionError exceptions and essentially ignore them.

I'm not sure if this is a good approach, but it appears to resolve the issue.

I couldn't replicate this locally at all (something specific to the runner?).

I eventually found this [pytest issue](pytest-dev/pytest#7491) that discusses
similar(ish) behaviors. This lead me to attempt to catch the PermissionError exception and essentially ignore it.

I'm not sure if this is a good approach, but it appears to resolve the issue.
@Snotpus Snotpus changed the base branch from dev to scientific-notation May 10, 2024 22:10
@Mikuana
Copy link
Owner

Mikuana commented May 11, 2024

First of all, that was some great detective work. Nice job digging into this.

I think the solution is great. If I was really wanting to dig deep, I'd say we should bury the exception handling a little lower in the TemporaryDirectory cleanup, but that would mean we couldn't use the context handler (I think), and then we'd be doing a lot of work just to keep a test from complaining. And in the end, if the test doesn't work perfectly we'll still know it, regardless of whether we ignore a permission error on writing the file initially. So I'd say this is more than good enough.

@Mikuana Mikuana merged commit 1aed064 into scientific-notation May 11, 2024
12 checks passed
@Mikuana Mikuana deleted the test-fix-maybe branch May 11, 2024 01:08
Mikuana added a commit that referenced this pull request May 11, 2024
* Working on rules for sn identification

* Refine regex

Adding tests

* Build update

Adding latest pythons and removing mac-os because it's
complaining. `Version 3.7 with arch x64 not found`

* Make '3.10' explicit

Build treated this as 3.1

* Add Number to ALL import

So this field gets brought in with others on a
* import.

* Add readthedocs config

Required to build.

* Remove comments

* Add sphinx requirements

* Ignore cleanup errors for temp directories

We don't really care about the details of why this isn't working I don't think. Our runner is ephemeral so who cares if something goes weird with the test cleanup?

* Revert "Ignore cleanup errors for temp directories"

This reverts commit d749102.

* Conditionally ignore cleanup errors in newer versions of python tests

* Mark extra function as a fixture....

* Revert "Mark extra function as a fixture...."

This reverts commit 29361b4.

* Revert "Conditionally ignore cleanup errors in newer versions of python tests"

This reverts commit 2a9d47a.

* Revert "Revert "Ignore cleanup errors for temp directories""

This reverts commit 134997d.

* Revert "Ignore cleanup errors for temp directories"

This reverts commit d749102.

* Add exception handling to usage of tempfile.TemporaryDirectory() (#111)

I couldn't replicate this locally at all (something specific to the runner?).

I eventually found this [pytest issue](pytest-dev/pytest#7491) that discusses
similar(ish) behaviors. This lead me to attempt to catch the PermissionError exception and essentially ignore it.

I'm not sure if this is a good approach, but it appears to resolve the issue.

Co-authored-by: Jacob Campbell <jacob.campbell@hca.wa.gov>

* whitespace

* bump

---------

Co-authored-by: Jacob Campbell <jacob.campbell@hca.wa.gov>
Co-authored-by: Snotpus <jacobl.campbell@gmail.com>
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.

2 participants