-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Generalize PYTEST_CURRENT_TEST for threading
#13837
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
base: main
Are you sure you want to change the base?
Conversation
d7db376 to
286f5b8
Compare
| *.orig | ||
| *~ | ||
| .hypothesis/ | ||
|
|
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 ignore rule is duplicated farther down the file
|
Ah hm, (Current very-WIP work is here: https://github.com/pytest-dev/pytest/compare/main...Liam-DeVoe:pytest:threading?expand=1) |
286f5b8 to
3730be8
Compare
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 a unacceptable wrong prediction
The current test env var itself doesn't translate to threads to begin with
Yeah, agreed. If this PR is no good, how would you prefer |
|
We need one env var per thread So some type of indication would be needed Id says the existing reserved for the pytest session on the main thread Other threads need a curret test of thread style variables Existing of the per thread values should be the indicator of multithread pytest as far ad observing processes are concerned Details should be expermented with |
|
OK nice. Wasn't sure if pytest wanted to support threads as such a first class citizen with special envvars and such. We'll need some way for pytest to know when a "runner" thread is executing a test, vs when a helper thread has been spawned by a test that happens to interact with pytest in some way. In other words I don't think it's sufficient to check |
|
threading.current_thread() is threading.main_thread() only on the main thread, else its another one names and idents will be tricky but one thing is for clear - just hiding key errors on a env var that shouldn't be thread-shared is not gona cut it |
|
as for thread pools - thats not a consideration as of now as setupstate is not separated |
fc96131 to
093a055
Compare
|
Something like this perhaps? Two tests are failing because they invoke |
| if thread is threading.main_thread(): | ||
| return 0 | ||
|
|
||
| return threads.index(thread) + 1 |
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.
We should not invent own thread numbering schemes
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.
should the envvars be named PYTEST_CURRENT_TEST_THREAD_{thread.ident}instead, eg. PYTEST_CURRENT_TEST_THREAD_8422586432? My thinking was that a more human readable approach would be nice, and also that way someone could tell how many threads are active. Happy to just use thread.ident though.
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.
we should require a "pytest thread" name for a thread run to be picked no guessing, no random long integers - the env names should be something people can deal with
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.
(i considered picking the threading thread name but their default is not ux friendly for env vars)
PYTEST_CURRENT_TEST thread-safePYTEST_CURRENT_TEST for threading
|
@Liam-DeVoe would you mind opening a specific issue about |
Part of #13768.
I'm starting to work earnestly on #13768, and figured I would start with a relatively simple fix + some general work for threading tests.
If we really cared about making
PYTEST_CURRENT_TESTas good as it could be, then we could track a stack of pushes/pops toPYTEST_CURRENT_TEST, and when a pop occurs, we set it to the most-recent value on the stack instead of setting it toNone. This would ensure thatPYTEST_CURRENT_TESTalways has a value when pytest is running, and is always "a test that is currently running", though there may be multiple currently-running tests. I am happy to implement this if you guys would prefer!