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

Test set for LowPowerTicker class #5047

Merged
merged 1 commit into from Nov 2, 2017

Conversation

Projects
None yet
8 participants
@maciejbocianski
Member

maciejbocianski commented Sep 7, 2017

Description

New test suite for LowPowerTicker class

Status

READY

##Blocked
#5185 fixedy by #5347 (merged)
#5150 fixedy by #5347 (merged)

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 7, 2017

TESTS/mbed_drivers/lp_ticker/main.cpp Outdated
#define TICKER_COUNT 16

Semaphore ticker_callback_sem(0, 1);
bool ticker_callback_flag;

This comment has been minimized.

@0xc0170

0xc0170 Sep 7, 2017

Member

assume this is also candidate for volatile?

TESTS/mbed_drivers/lp_ticker/main.cpp Outdated
TestTicker ticker[TICKER_COUNT];

multi_counter = 0;
for(int i = 0; i < TICKER_COUNT; i++) {

This comment has been minimized.

@0xc0170

0xc0170 Sep 7, 2017

Member

a nit: for ( space there

TESTS/mbed_drivers/lp_ticker/main.cpp Outdated
ticker[i].detach();
}

//TEST_ASSERT_EQUAL(TICKER_COUNT, multi_counter);

This comment has been minimized.

@0xc0170

0xc0170 Sep 7, 2017

Member

no dead code

TESTS/mbed_drivers/lp_ticker/main.cpp Outdated
Case("Test multi ticker", test_multi_ticker),
};

utest::v1::status_t greentea_test_setup(const size_t number_of_cases) {

This comment has been minimized.

@0xc0170

0xc0170 Sep 7, 2017

Member

{ new line for function body

TESTS/mbed_drivers/lp_ticker/main.cpp Outdated

using namespace utest::v1;

#define TestTicker LowPowerTicker

This comment has been minimized.

@bulislaw

bulislaw Sep 7, 2017

Member

I don't like that, it's confusing. Is there a particular reason you are doing that?

TESTS/mbed_drivers/lp_ticker/main.cpp Outdated
}


/** Test many tickers run simultaneously

This comment has been minimized.

@bulislaw

bulislaw Sep 7, 2017

Member

This test case does something else than described here.

TESTS/mbed_drivers/lp_ticker/main.cpp Outdated

/** Test multi callback time

Given an Ticker

This comment has been minimized.

@bulislaw

bulislaw Sep 7, 2017

Member

just a nit: a Ticker

TESTS/mbed_drivers/lp_ticker/main.cpp Outdated

Given an Ticker
When schedule callback
Then ticker properly execute callback multiple time

This comment has been minimized.

@bulislaw

bulislaw Sep 7, 2017

Member

time -> times

TESTS/mbed_drivers/lp_ticker/main.cpp Outdated
/** Test multi callback time

Given an Ticker
When schedule callback

This comment has been minimized.

@bulislaw

bulislaw Sep 7, 2017

Member

It's not only once it's scheduled: When the callback is attached multiple times

TESTS/mbed_drivers/lp_ticker/main.cpp Outdated
/** Test if detach cancel scheduled callback event

Given an Ticker with scheduled callback
When schedule is cancelled

This comment has been minimized.

@bulislaw

bulislaw Sep 7, 2017

Member

schedule is not really something that exists in Ticker terms, I know what you mean, but we could use something like:
Given a Ticker with callback attached
When the callback is detached
Then the callback is not being called

Also your test does more than described here.

TESTS/mbed_drivers/lp_ticker/main.cpp Outdated

/** Test single callback time via attach

Given an Ticker

This comment has been minimized.

@bulislaw

bulislaw Sep 7, 2017

Member

That should be rewritten, some examples in previous tests.

TESTS/mbed_drivers/lp_ticker/main.cpp Outdated
ticker.detach();
int time_diff = timer.read_us();

printf("TOLERANCE: %d\n", (uint32_t)TOLERANCE);

This comment has been minimized.

@bulislaw

bulislaw Sep 7, 2017

Member

Lets not print anything if not needed.

TESTS/mbed_drivers/lp_ticker/main.cpp Outdated

/** Test single callback time via attach_us

Given an Ticker

This comment has been minimized.

@bulislaw

bulislaw Sep 7, 2017

Member

Could you rewrite this, examples in previous tests.

TESTS/mbed_drivers/lp_ticker/main.cpp Outdated
ticker.detach();
int time_diff = timer.read_us();

printf("TOLERANCE: %d\n", (uint32_t)TOLERANCE);

This comment has been minimized.

@bulislaw

bulislaw Sep 7, 2017

Member

Lets not print if we don't have to.

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:lp_ticker_tests branch Sep 8, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 8, 2017

@theotherjimmy @studavekar To be aware, I found travis failing for some recent PR, with InsecurePlatformWarning. I restarted it here, please have a look once finished if it is still there. Has anything chagned again, or new module update somewhere causing this?

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Sep 8, 2017

Why is this being tested at the driver layer rather than the HAL layer? The HAL specification will require test cases that will make these redundant.

@bulislaw

This comment has been minimized.

Member

bulislaw commented Sep 11, 2017

I think that should be done on both layers, drivers and HAL (when we finish drafting the new specification).

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:lp_ticker_tests branch Sep 12, 2017

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:lp_ticker_tests branch Sep 20, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 25, 2017

@maciejbocianski @bulislaw What's the status of this PR?

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Sep 26, 2017

@theotherjimmy @bulislaw
test fails on K64F
issue #5185

@0xc0170 0xc0170 added needs: work and removed needs: review labels Sep 26, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 26, 2017

@theotherjimmy @bulislaw
test fails on K64F

Is this the only failure that we should expect. This means this is waiting for the fix. cc @mmahadevan108 for visibility (already tagged in the issue)

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Sep 26, 2017

Yes, it was the only failure from tested pool k64f, nrf51_dk, nucleo_f070rb, nucleo_f429zi,

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:lp_ticker_tests branch Oct 11, 2017

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 11, 2017

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:lp_ticker_tests branch to 9fb4fd0 Oct 11, 2017

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 11, 2017

Build : SUCCESS

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

Triggering tests

/test mbed-os

@0xc0170 0xc0170 removed the needs: work label Oct 19, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 19, 2017

K64F should be fixed via #5347 , what about the rest?

@bulislaw

This comment has been minimized.

Member

bulislaw commented Oct 27, 2017

@maciejbocianski what's the status here?

@maciejbocianski

This comment has been minimized.

Member

maciejbocianski commented Oct 27, 2017

I think it's ready and waiting for morph,
at least not blocked by any issue

@bulislaw

This comment has been minimized.

Member

bulislaw commented Oct 27, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 27, 2017

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-test

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 27, 2017

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-test

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Oct 30, 2017

/morph test

@mbed-ci

This comment has been minimized.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 31, 2017

@c1728p9 @bulislaw Are you guys happy this has been fully reviewed? I can't see any evidence of a review being signed off here?

@theotherjimmy theotherjimmy merged commit 1454e6b into ARMmbed:master Nov 2, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment