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
gh-102402: Make test_relativeCreated_has_higher_precision less implementation dependent #118062
gh-102402: Make test_relativeCreated_has_higher_precision less implementation dependent #118062
Conversation
serhiy-storchaka
commented
Apr 18, 2024
•
edited by bedevere-app
bot
edited by bedevere-app
bot
- Issue: Logging's msecs doesn't handle "100ms" well. #102402
…implementation dependent
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.
LGTM
for offset_ns, line in zip(offsets_ns, out.splitlines(), strict=True): | ||
with self.subTest(offset_ns=offset_ns): | ||
created, relativeCreated = map(float, line.split()) | ||
self.assertAlmostEqual(created, (start_ns + offset_ns) / 1e9, places=6) |
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.
You could implement these checks in the child process (in "code").
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.
How? if not math.isclose(...): sys.exit(1)
? I do not remember parameters of math.isclose()
, and in any case the TestCase
methods provide more informative error reports.
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.
Maybe write a test case and call unittest.main(). But I'm fine with your code.
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.
It will add unpredicable complexity. What if unittest
uses the time
module, directly or indirectly?
We need also a way to translate the result from the subprocess and translate it to the outer TestCase. Much complex work for no gain. I prefer if we pass a raw data from a subprocess. It is not always possible if the data is too large or complex.
Thank you for your review @vstinner.
def test_relativeCreated_has_higher_precision(self): | ||
# See issue gh-102402 | ||
ns = 1_677_903_920_000_998_503 # approx. 2023-03-04 04:25:20 UTC | ||
start_ns = 1_677_903_920_000_998_503 # approx. 2023-03-04 04:25:20 UTC | ||
offsets_ns = (200, 500, 12_354, 99_999, 1_677_903_456_999_123_456) |
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.
Add maybe a comment explaining why you spawn a subprocess? In short, I understood that it's hard to avoid side effects of re-importing the logging module, such as reference leaks.
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.
Done.