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.

Sign up for GitHub

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

Jump to bottom

test(threading): Add spec for ThreadPoolExecutor #2259

Merged
merged 8 commits into from
Aug 16, 2023
Merged

test(threading): Add spec for ThreadPoolExecutor #2259

merged 8 commits into from
Aug 16, 2023

Conversation

gggritso
Copy link
Member

ThreadPoolExecutor also obeys hub propagation, but there wasn't a spec for it. This PR adds a bit more coverage.

@gggritso
Add test for ThreadPool
2a97d24 
`ThreadPool` also obeys hub propagation, but there was no spec for it.
@sl0thentr0py
Copy link
Member

will fix the 2.7 stuff

@sl0thentr0py
Copy link
Member

alright that works too

@gggritso
Copy link
Member Author

@sl0thentr0py I keep playing whack-a-mole with old Python versions 😅 I copy-pasted from other specs for now, but some stuff is still failing. What's the right way to do things like import concurrent? Should I do it inside the spec instead of top-of-file?

@sl0thentr0py
Copy link
Member

pushed fix, we always do an except ImportError see last commit

@gggritso
Copy link
Member Author

@sl0thentr0py ugh, sorry, one more mysterious failure! Something is wrong in test_cloud_resource_context.py, but I can't see the reason. Re-running didn't help, and the spec looks unrelated. Any idea what's going on?

@sl0thentr0py
Copy link
Member

waddafaq, looking

@sl0thentr0py
Copy link
Member

obviously it works on my machine

@antonpirker antonpirker self-assigned this Jul 19, 2023
@gggritso
Copy link
Member Author

@antonpirker whatever you did, it seems like it worked? Was the test failure just a weird flake of some kind?

@gggritso gggritso marked this pull request as ready for review July 26, 2023 14:37
@gggritso
Copy link
Member Author

@sl0thentr0py @antonpirker what do you think, is this PR good to merge? Is it even worth it?

@antonpirker
Copy link
Member

Hey @gggritso
Sorry for the late reply, I was on vacation.
More coverage is always good, so I will merge this.

@antonpirker antonpirker enabled auto-merge (squash) August 16, 2023 09:12
@antonpirker antonpirker merged commit 3845489 into master Aug 16, 2023
@antonpirker antonpirker deleted the chore/ggg/threadpool-hub-propagation-spec branch August 16, 2023 09:18
@gggritso
Copy link
Member Author

@antonpirker awesome, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sl0thentr0py sl0thentr0py sl0thentr0py approved these changes

Assignees

@antonpirker antonpirker

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gggritso @sl0thentr0py @antonpirker