-
-
Notifications
You must be signed in to change notification settings - Fork 75
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
Fix test_time being always 0.0 #292
Conversation
The bug was introduced in commit 651ab46 when setting start_time = 0.0 was added to ProtoTestResult.reinitialize().
It's probably a good idea to do a thorough testing of everything given the amount of recent changes. |
green/test/test_result.py
Outdated
ptr = ProtoTestResult() | ||
test = proto_test(MagicMock()) | ||
ptr.startTest(test) | ||
ptr.stopTest(test) | ||
self.assertGreater(float(ptr.test_time), 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.
For readability it is better to use plain words:
ptr = ProtoTestResult() | |
test = proto_test(MagicMock()) | |
ptr.startTest(test) | |
ptr.stopTest(test) | |
self.assertGreater(float(ptr.test_time), 0) | |
result = ProtoTestResult() | |
test = proto_test(MagicMock()) | |
result.startTest(test) | |
result.stopTest(test) | |
self.assertGreater(float(result.test_time), 0) |
That said I have a bit of concern about this because time is not mocked and on a future very fast machine it would not be impossible for the time to be recorded as a true zero.
We could add https://pypi.org/project/freezegun/ in our requirements-dev.txt file and use it to mock the time to ensure that we have predictable behavior.
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's better to be consistent with other tests. This is the naming and the structure used in all other tests in TestProtoTestResult.
I don't think we need to worry about time being zero. Even calling time.time()
in a tight loop does not produce the same number. If it does become a problem, we can simply add a small sleep in the test. freezegun
sets time to a constant, which will result in test_time being zero. If we need to mock, we can directly mock time.time()
to an iterable, so it returns different values on subsequent calls.
@CleanCut what do you think?
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.
Added mocking for time.time()
and a more precise assert.
Thanks for the PR and sorry for introducing this bug. I'm not sure why I did not notice it. |
Btw, looks like the coverage number in the CI is wrong ^^ |
For the lack of coverage we are tracking this in #289 but I have not had time to look into it since it seems to be limited to green's GH action. Local coverage numbers are fine. |
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
The bug was introduced in commit 651ab46 when setting start_time = 0.0 was added to ProtoTestResult.reinitialize().