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.

Sign up for GitHub

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

Workaround random test_suite_platform fail in time test #7419

Merged
merged 8 commits into from
Apr 28, 2023
Merged

Workaround random test_suite_platform fail in time test #7419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fce8577
try to reproduce random assert fail
yuhaoth Apr 10, 2023
c9c3e62
workaround the assert fail with tollerance
yuhaoth Apr 11, 2023
2f1e85f
fix comments issues
yuhaoth Apr 17, 2023
4852bb8
remove time delay tests
yuhaoth Apr 18, 2023
d1190a5
Update comments and remove delay seconds test
yuhaoth Apr 18, 2023
ed9b9a7
Add warning to reserve the reason
yuhaoth Apr 18, 2023
d3c7d53
Improve comments about the time_delay test.
yuhaoth Apr 19, 2023
ad2091d
fix grammar issues
yuhaoth Apr 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions tests/suites/test_suite_platform.data
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,3 @@ time_get_seconds:

Time: delay milliseconds
time_delay_milliseconds:1000

Time: delay seconds
time_delay_seconds:1
28 changes: 25 additions & 3 deletions tests/suites/test_suite_platform.function
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ void sleep_ms(int milliseconds)

/* END_DEPENDENCIES */



/* BEGIN_CASE depends_on:MBEDTLS_HAVE_TIME */
void time_get_milliseconds()
{
Expand Down Expand Up @@ -76,6 +74,14 @@ void time_delay_milliseconds(int delay_ms)
/* END_CASE */

/* BEGIN_CASE depends_on:MBEDTLS_HAVE_TIME */

/*
* WARNING: DO NOT ENABLE THIS TEST. We keep the code here to document the
* reason.
*
* The test often failed on the CI. See #1517. CI failures cannot be
* completely avoided due to out-of-sync clock sources.
*/
void time_delay_seconds(int delay_secs)
{
mbedtls_time_t current = mbedtls_time(NULL);
Expand All @@ -84,7 +90,23 @@ void time_delay_seconds(int delay_secs)
sleep_ms(delay_secs * 1000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need these tests?

Historically, we had somewhat similar tests in the timing module. They often failed on the CI and so we ended up removing them. I fear that we've reintroduced the problem, and this pull request is just one of many.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it.

Should I remove these tests here? Or another PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delay test were removed.

I think we should keep *get

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely removing the tests may be too much. How about keeping the calls to the functions (so at least we know they don't e.g. crash), but not testing delays?

Possibly even check that t1 = ms_time(); sleep(small); t2 = ms_time(); ASSERT(t2 > t1). @yuhaoth Can you clarify

Built-in mbedtls_time function returns the number of seconds since the
Epoch. That is affected by discontinuous jumps and cause test fail.
Workaround it with 1 seconds tollerance.
Epoch. That is affected by discontinuous jumps. And nanosleep use
CLOCK_MONOTONIC(monotonically-increasing time source), That will cause
negative elapsed time difference.

What can cause a negative elapsed time difference? E.g. Can this happen from automatic drift adjustment? I would have expected that t2 - t1 might be less than small, but not negative.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this happen from automatic drift adjustment?

It happens only in time_delay_seconds because time source are different. Built-in mbedtls_time was defined as standard time function, the time source is CLOCK_REALTIME . And nanosleep take CLOCK_MONOTONIC as time source.

If CLOCK_MONOTONIC is faster than CLOCK_REALTIME and CLOCK_REALTIME was adjusted during sleep, sometime t2 - t1 < small happens.

And I think time_delay_milliseconds should not be removed now :) . It use same time source.

Possibly even check that t1 = ms_time(); sleep(small); t2 = ms_time(); ASSERT(t2 > t1).

This check can not resolve the issue.

Copy link
Contributor Author

@yuhaoth yuhaoth Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just update the comments. But I did not mention automatic drift adjustment. I think that's enough.

And I revert last commit without Time: delay seconds test


elapsed_secs = mbedtls_time(NULL) - current;
TEST_ASSERT(elapsed_secs >= delay_secs && elapsed_secs < 4 + delay_secs);

/*
* `mbedtls_time()` was defined as c99 function `time()`, returns the number
* of seconds since the Epoch. And it is affected by discontinuous changes
* from automatic drift adjustment or time setting system call. The POSIX.1
* specification for clock_settime says that discontinuous changes in
* CLOCK_REALTIME should not affect `nanosleep()`.
*
* If discontinuous changes occur during `nanosleep()`, we will get
* `elapsed_secs < delay_secs` for backward or `elapsed_secs > delay_secs`
* for forward.
*
* The following tolerance windows cannot be guaranteed.
* PLEASE DO NOT ENABLE IT IN CI TEST.
*/
TEST_ASSERT(elapsed_secs - delay_secs >= -1 &&
elapsed_secs - delay_secs < 4);
/* This goto is added to avoid warnings from the generated code. */
goto exit;
}
Expand Down