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

Fixed #32611 -- Prevented unecessary setup()/teardown() calls when using --bisect/--pair runtests options. #14230

Merged

Conversation

cjerdonek
Copy link
Contributor

@cjerdonek cjerdonek force-pushed the ticket-32611-bisect-and-paired-tests-setup branch from 469f434 to 9d8d506 Compare April 7, 2021 05:18
@felixxm
Copy link
Member

felixxm commented Apr 19, 2021

@cjerdonek Thanks for this patch 👍 I have a question about the second commit. bisect_tests() and paired_tests() are not called by and don't call django_tests(), so moving logging from setup() to django_tests() removes the initial "Testing against ..." output when test_labels are not provided, which is fine, IMO. Am I right? The ticket description suggests that you wanted to keep this output, that's why I'm asking.

…ing --bisect/--pair runtests options.

This commit changes runtests.py's bisect_tests() and paired_tests() to
change settings only when necessary, namely when specific test names
aren't provided.
@felixxm felixxm force-pushed the ticket-32611-bisect-and-paired-tests-setup branch from 9d8d506 to a41ed8f Compare April 19, 2021 07:21
@felixxm felixxm changed the title Fixed #32611 -- Changed runtests.py to change settings only when needed. Fixed #32611 -- Prevented unecessary setup()/teardown() calls when using --bisect/--pair runtests options/ Apr 19, 2021
@felixxm felixxm changed the title Fixed #32611 -- Prevented unecessary setup()/teardown() calls when using --bisect/--pair runtests options/ Fixed #32611 -- Prevented unecessary setup()/teardown() calls when using --bisect/--pair runtests options. Apr 19, 2021
@cjerdonek
Copy link
Contributor Author

Good question, @felixxm. Thanks for asking. Once I started working on this, I noticed that a consequence of only calling setup() when test_labels isn't provided is that the initial logging would be different depending on whether test_labels is or isn't provided, which wouldn't be ideal. But then I realized that that initial logging isn't actually important for bisect_tests() and paired_tests() because that log message gets repeated when they each call the subprocess anyways (which will happen multiple times). So I thought it would be better just to do the call when the tests are actually being run (in the same process), instead of having the message duplicated by the parent process.

@felixxm
Copy link
Member

felixxm commented Apr 19, 2021

So I thought it would be better just to do the call when the tests are actually being run (in the same process), instead of having the message duplicated by the parent process.

Agreed, thanks 👍

@felixxm felixxm merged commit a41ed8f into django:main Apr 19, 2021
@cjerdonek cjerdonek deleted the ticket-32611-bisect-and-paired-tests-setup branch April 19, 2021 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants