-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[#12052] Update distributed trial for the changed skip behaviour in 3.12.1 #12054
Conversation
src/twisted/trial/reporter.py
Outdated
@@ -104,6 +109,8 @@ def __init__(self): | |||
self.unexpectedSuccesses = [] | |||
self.successes = 0 | |||
self._timings = [] | |||
self._testStarted = 0 |
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.
the problem was that with 3.12.1 startTest()
was no longer called for skipped tests... but stopTest()
was called.
This is my take on the patch... it might not be the best option.
But I hope it's a start.
@exarkun @glyph Please review the changes. This enabled 3.12.1 tests , as the previous tests were 3.12.0
Feel free to suggest a better fix for this. I don't know what type of test to write here... I think that the current needs-review |
src/twisted/trial/reporter.py
Outdated
@@ -96,6 +96,11 @@ class TestResult(pyunit.TestResult): | |||
expectedFailures: List[Tuple[itrial.ITestCase, str, "Todo"]] # type: ignore[assignment] | |||
unexpectedSuccesses: List[Tuple[itrial.ITestCase, str]] # type: ignore[assignment] | |||
successes: int | |||
# The time when the test was started. | |||
# It might be 0 if the tests was skipped, so never started. | |||
_testStarted: int |
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.
A Maybe int
(so, possibly Optional[int]
) would be a better way to represent "time when test started, or never started" would. Leaving 0 here lets code easily incorrectly interpret the start time as the UNIX epoch.
Likewise for _lastTime
.
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.
Since it's the difference between disttrial working at all on 3.12.1 and not, I think this might be worth actually describing in the user-facing news.
Thanks Jean-Paul for the review. Not sure how to add a reminder. Once 3.12.0 is the minimum supported version of Python in trial, we can remove the custom duration code and use the stdlib introduced in 3.12.0 as part of python/cpython#12271 needs-review |
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.
Thanks. One last suggestion inline but otherwise this seems like the right short-term fix (longer term might be to use less of stdlib unittest to have more control).
Co-authored-by: Jean-Paul Calderone <exarkun@twistedmatrix.com>
I applied the changes since the old comment was grammatically as well as semantically incorrect, and hopefully CI will be happy and this will auto-land shortly. |
Many thanks for your reviews and feedback |
Scope and purpose
Fixes #12052
This updated distributed trial code to work with latest changes from python/cpython@8fc0713#diff-c6fe1ffe930def48a6adf1fa99b974737bac586fdacccacd1474e7b2f11370ebL609