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

Rtostimer tests #4947

Merged
merged 3 commits into from Oct 19, 2017

Conversation

Projects
None yet
8 participants
@fkjagodzinski
Member

fkjagodzinski commented Aug 21, 2017

Description

RTOS RtosTimer:

  • added unit tests,
  • reworked API docs based on CMSIS-RTOS2 API docs.

Status

READY

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 21, 2017

retest uvisor

1 similar comment
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 22, 2017

retest uvisor

@fkjagodzinski

This comment has been minimized.

Member

fkjagodzinski commented Aug 23, 2017

TESTS/mbedmicro-rtos-mbed/rtostimer/main.cpp Outdated
osStatus stat = timer.stop();
TEST_ASSERT_EQUAL(osErrorResource, stat);
int32_t slots = sem.wait(0);

This comment has been minimized.

@bulislaw

bulislaw Aug 23, 2017

Member

Why do we do that?

This comment has been minimized.

@fkjagodzinski

fkjagodzinski Aug 23, 2017

Member

Since Semaphore has its own tests this assert is actually redundant. I'll fix that.

This comment has been minimized.

@fkjagodzinski
TESTS/mbedmicro-rtos-mbed/rtostimer/main.cpp Outdated
osStatus stat = timer.stop();
TEST_ASSERT_EQUAL(osErrorResource, stat);
int32_t slots = sem.wait(0);

This comment has been minimized.

@bulislaw

bulislaw Aug 23, 2017

Member

Why? We should assume in this tests semaphores work fine (they have their own tests).

This comment has been minimized.

@fkjagodzinski
TESTS/mbedmicro-rtos-mbed/rtostimer/main.cpp Outdated
* Then timer stops
* and elapsed time matches given delay
*/
void test_correct_delay()

This comment has been minimized.

@bulislaw

bulislaw Aug 23, 2017

Member

What's the difference between this and test_oneshot_not_restarted

This comment has been minimized.

@fkjagodzinski

fkjagodzinski Aug 23, 2017

Member

Since other test cases already check the accuracy of delays I removed this one.

@fkjagodzinski fkjagodzinski force-pushed the fkjagodzinski:rtostimer_tests branch Aug 23, 2017

@bulislaw

@0xc0170 could you have a look

@bulislaw

This comment has been minimized.

Member

bulislaw commented Aug 25, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 25, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1118

Build failed!

@bulislaw

This comment has been minimized.

Member

bulislaw commented Aug 29, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 30, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1137

Test failed!

TESTS/mbedmicro-rtos-mbed/rtostimer/main.cpp Outdated
using namespace utest::v1;
#define THREAD_STACK_SIZE 512

This comment has been minimized.

@0xc0170

0xc0170 Aug 31, 2017

Member

This is not used anywhere, neither creating multiple threads. Is the line 22 above also valid?

This comment has been minimized.

@fkjagodzinski

fkjagodzinski Sep 4, 2017

Member

You're right. I'll remove that.

TESTS/mbedmicro-rtos-mbed/rtostimer/main.cpp Outdated
sem.wait(0);
stat = timer.start(TEST_DELAY_MS);
uint32_t t1 = us_ticker_read();

This comment has been minimized.

@0xc0170

0xc0170 Aug 31, 2017

Member

I would recomend using Timer class instead of us ticker read to capture the duration (C++ classes should provide all its needed)

This comment has been minimized.

@fkjagodzinski

fkjagodzinski Sep 4, 2017

Member

Sure, I'll update time measurement with Timers.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 4, 2017

@fkjagodzinski Any update?

@fkjagodzinski fkjagodzinski force-pushed the fkjagodzinski:rtostimer_tests branch to 33e38cc Sep 4, 2017

@0xc0170

0xc0170 approved these changes Sep 4, 2017

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Sep 4, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 21, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 22, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1354

Test failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 22, 2017

@fkjagodzinski please look at failures, related to this changeset

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Sep 22, 2017

@fkjagodzinski

This comment has been minimized.

Member

fkjagodzinski commented Sep 22, 2017

I suppose the ci/morph-test is failing because of -DMBED_TRAP_ERRORS_ENABLED=1.

Tests need to check osStatus returned by RtosTimer::stop() and RtosTimer::start(), e.g. here. Since these methods call EvrRtxTimerError this leads to test failure when compiled with --profile=debug.

What is the preferred solution? Is it possible to use another profile for these tests?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 22, 2017

What is the preferred solution? Is it possible to use another profile for these tests?

One way could be to separate that failure to another test unit that would catch it via host script - that would pass if it catches the failure via serial. We got some tests like that already , can't recall from top of my head any now.

@c1728p9 or @pan- Any other suggestions?

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Sep 22, 2017

I would recommend adding an installable hook/callback into either error or EvrRtx*Error and assert that it is called in the test if the API is misused.

Tests: RTOS: RtosTimer: Fix tests on debug profile
Redefine error() as noop to fix tests being aborted when compiled
with -DMBED_TRAP_ERRORS_ENABLED=1.
@fkjagodzinski

This comment has been minimized.

Member

fkjagodzinski commented Sep 26, 2017

Pushed a fix that overrides error(). I'm not entirely sure if this is a correct approach though.

* which aborts test program.
*/
#if defined(MBED_TRAP_ERRORS_ENABLED) && MBED_TRAP_ERRORS_ENABLED
void error(const char* format, ...) {

This comment has been minimized.

@bulislaw

bulislaw Sep 27, 2017

Member

Should we validate in the function that causes an error it actually happens by setting some flag here?

This comment has been minimized.

@fkjagodzinski

fkjagodzinski Sep 27, 2017

Member

Isn't that going too deep into internals? The RtosTimer API does not say anything about calling error().

This comment has been minimized.

@bulislaw

bulislaw Sep 28, 2017

Member

True, you're right.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 9, 2017

@c1728p9 @bulislaw Are you happy with the current version of this patch?

@c1728p9

c1728p9 approved these changes Oct 9, 2017

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 9, 2017

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Oct 13, 2017

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 13, 2017

Build : SUCCESS

Build number : 172
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/4947/

Triggering tests

/test mbed-os

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@fkjagodzinski

This comment has been minimized.

Member

fkjagodzinski commented Oct 17, 2017

Unless anyone has any comments, the PR is ready. I don't have any more updates to push.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 17, 2017

Unless anyone has any comments, the PR is ready. I don't have any more updates to push.

We will rerun uvisor CI once their fix lands,m then this should be ready for integration

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 18, 2017

/morph uvisor-test

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 18, 2017

Build : SUCCESS

Build number : 227
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/4947/

Triggering tests

/morph test
/morph uvisor-test

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Oct 19, 2017

@0xc0170 0xc0170 merged commit cbb8a1f into ARMmbed:master Oct 19, 2017

7 checks passed

AWS-CI uVisor Build & Test Success
Details
AWS-CI uvisor Test DIDNT RUN UVISOR TESTS
Details
Cam-CI uvisor Build & Test DIDNT RUN UVISOR TESTS
Details
ci-morph-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@fkjagodzinski fkjagodzinski deleted the fkjagodzinski:rtostimer_tests branch Oct 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment