common-io: Refactoring Timer test cases #1887
Conversation
| @@ -49,7 +49,7 @@ | |||
| /* Globals values which can be overwritten by the test | |||
| * framework invoking these tests */ | |||
| /*-----------------------------------------------------------*/ | |||
| extern IotTimerHandle_t gIotTimerHandle; | |||
| IotTimerHandle_t gIotTimerHandle; | |||
There was a problem hiding this comment.
Please keep it extern - in the case where application already uses the timer - you can still open the handle.
There was a problem hiding this comment.
@vjjunnut Is this a valid use case in the test system? Are you running tests in parallel to applications that already allocate the timer?
There was a problem hiding this comment.
Test application can use the time in logging which uses these APIs - so in those cases, timer is already initialized.
There was a problem hiding this comment.
Much like in the Flash use-case, I don't really see how this was intended to work in the scenario you describe.
Most of the tests close the handle, the majority of the tests to not reset the handle to NULL when doing this. This means that any logging depending on this handle would get screwed as soon as the first test finish.
Strictly speaking, depending on the module you aim to test while testing it sound like it could cause headache further down the line. Maybe an alternative timer should be used for the logging purpose in this case, is that an alternative?
There was a problem hiding this comment.
@qiutongs Any thought on this?
| testIotTIMER_DEFAULT_DELAY_US ); | ||
|
|
||
| /* Expect either "Not running" or "Success" */ | ||
| if( lRetVal != IOT_TIMER_NOT_RUNNING ) |
There was a problem hiding this comment.
You need to add the iot_timer_delay after starting the timer so that semaphoreTake can be unblocked. Since we are updating the test to check for NOT_RUNNING, lets also fix the test to do the delay after starting the timer and expect the semaphore take to succeed.
There was a problem hiding this comment.
@vjjunnut II have asked this question to @lundinc2 before to what the expected behavior for calling a delay after start is. My understanding is that setting a delay before start will still result in the delay being perform from the point of "start" which means the semaphore will be unblocked as we do start the timer.
The expected behavior of "delay" being called after the timer has been started is unclear. I would imagine that it would start the "delay counter" right away but this was not what the tests implied at first which is why I have not altered the test sequence, just added a check for possible return values as these are clearly defined in the API.
There was a problem hiding this comment.
The test had a bug. We can sync up internally and get back.
|
Sorry for the delay. I am engaging lab126 for providing the comment. |
|
/bot run checks |
1 similar comment
|
/bot run checks |
|
There is a build failure that is not related to your PR. But it needs to rebase to the latest master. Sorry for that inconvenience. Do you mind giving me writing access to your fork? I am help rebase it. |
qiutongs
force-pushed
the
fix/common-io-timer-test-refactoring
branch
from
1ea7589 to
bce696f
Compare
|
/bot run checks |
…cases (aws#1887) Co-authored-by: Max Wennerfeldt <[email protected]>
Description
Checklist:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.