[reboot_test] Fix flaky time.time() assertions on second boundaries#391
Open
DavidZagury wants to merge 2 commits into
Open
[reboot_test] Fix flaky time.time() assertions on second boundaries#391DavidZagury wants to merge 2 commits into
DavidZagury wants to merge 2 commits into
Conversation
The execute_reboot tests compared a mocked populate_reboot_status_flag call against a freshly-sampled int(time.time()) in the assertion. When the wall clock crossed a second boundary between the production call and the assertion, the timestamps differed by 1 and the test failed. Patch time.time to the existing TIME constant in the five affected tests, matching the pattern already used in test_populate_reboot_status_flag*. Production halt-loop uses time.monotonic() and is unaffected. Signed-off-by: david.zagury <davidza@nvidia.com>
Address CodeRabbit docstring-coverage warning on the five tests updated in the previous commit. One-line summary per test describing the scenario being exercised. Signed-off-by: david.zagury <davidza@nvidia.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Member
|
@saiarcot895 could you please help review this? Thanks. |
There was a problem hiding this comment.
Pull request overview
This PR improves test determinism for the host_modules/reboot.py failure-timestamp behavior by removing reliance on live time.time() sampling during assertions in tests/host_modules/reboot_test.py.
Changes:
- Mock
time.time()in fiveexecute_rebootpytest cases to eliminate second-boundary races. - Replace
int(time.time())in assertions with the fixedTIMEconstant to ensure stable expected values. - Add short docstrings to the affected tests to clarify intent.
Comment on lines
272
to
277
| with ( | ||
| mock.patch("reboot._run_command") as mock_run_command, | ||
| mock.patch("time.sleep") as mock_sleep, | ||
| mock.patch("time.time", return_value=TIME), | ||
| mock.patch("reboot.Reboot.is_halt_command_running", return_value=True) as mock_is_halt_command_running, | ||
| mock.patch("reboot.Reboot.is_container_running", return_value=True) as mock_is_container_running, |
saiarcot895
approved these changes
Jun 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why I did it
The execute_reboot pytest cases in tests/host_modules/reboot_test.py were flaky on second boundaries. The production code in host_modules/reboot.py records a failure timestamp via int(time.time()) and the tests compared that recorded value against another freshly-sampled int(time.time()) inside the assertion. When the wall clock ticked over a second between the production call and the assertion, the two integers differed by 1 and the test failed.
This was observed in a SONiC builds where test_execute_reboot_fail_issue_reboot_command_warm failed with:
Four other tests in the same file shared the identical race and would have failed at random as well.
How I did it
In each of the five affected tests, added mock.patch("time.time", return_value=TIME) to the existing with block and replaced the in-assertion int(time.time()) with the literal TIME constant (already defined at the top of the file as 1617811205). This matches the pattern already in use by test_populate_reboot_status_flag and test_populate_reboot_status_flag_with_status in the same file.
Tests updated:
Production code is untouched. The halt-timeout loop in reboot.py uses time.monotonic() rather than time.time(), so patching time.time does not affect any production control flow.