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

Disables flash clock and cache test #6340

Merged
merged 1 commit into from Mar 20, 2018

Conversation

Projects
None yet
7 participants
@cmonr
Contributor

cmonr commented Mar 12, 2018

Has been causing intermittent morph test failures with NRF52_DK boards

Description

Disables the flash clock and cache tests. For a few weeks, this test would intermittently fail specifically against the NRF52_DK built against the GCC_ARM compiler.

An example failure:

[1520888954.03][CONN][RXD] >>> Running case #6: 'Flash - clock and cache test'...
[1520888954.11][CONN][INF] found KV pair in stream: {{__testcase_start;Flash - clock and cache test}}, queued...
[1520888954.77][CONN][RXD] :273::FAIL: Values Not Within Delta 3016 Expected 603210 Was 590241
[1520888954.77][CONN][INF] found KV pair in stream: {{__testcase_finish;Flash - clock and cache test;0;1}}, queued...
[1520888954.86][CONN][RXD] >>> 'Flash - clock and cache test': 0 passed, 1 failed with reason 'Assertion Failed'

Note: This is not expected to be a permanent fix, but should make tests more stable and save us some test-restart pains.

Pull request type

  • Fix
  • Refactor
  • New target
  • Feature
  • Breaking change

@cmonr cmonr added the needs: review label Mar 12, 2018

@cmonr cmonr requested review from 0xc0170, adbridge and studavekar Mar 12, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 12, 2018

/morph build

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 12, 2018

Restarting. Let other PR take priority. (#6341)

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Mar 13, 2018

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

const int acceptable_range = timer_diff_start / (ALLOWED_DRIFT_PPM);
TEST_ASSERT_UINT32_WITHIN(acceptable_range, timer_diff_start, timer_diff_end);
}
//void flash_clock_and_cache_test()

This comment has been minimized.

@studavekar

studavekar Mar 13, 2018

Collaborator

can we just comment call to this function.

This comment has been minimized.

@cmonr

cmonr Mar 13, 2018

Contributor

I didn't want to throw an extra warning about an unused function.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 13, 2018

Are we able to reproduce this error? Disabling a test is not an answer usually.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 13, 2018

Are we able to reproduce this error? Disabling a test is not an answer usually.

Not realiably, but it's a failure error we've been seeing more of lately.
This PR was meant as a short term solution to the problem, with the (proper) long term solution to come after.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 13, 2018

This PR was meant as a short term solution to the problem, with the (proper) long term solution to come after.

Wouldn't be better to disable it just for one target then?

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Mar 13, 2018

Are we able to reproduce this error? Disabling a test is not an answer usually.

we have seen this often on NRF52840_DK, it would be a good candidate to reproduce.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 13, 2018

Wouldn't be better to disable it just for one target then?

Wasn't aware that was a thing. Will update the PR to do just that.

@cmonr cmonr added needs: work and removed needs: review labels Mar 13, 2018

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Mar 13, 2018

Wouldn't be better to disable it just for one target then?
Wasn't aware that was a thing. Will update the PR to do just that.

Probably nordic targets may be?

@cmonr cmonr requested a review from bridadan Mar 16, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 16, 2018

@studavekar Re-review requested.

Tested by compiling against NRF52_DK and K66F.

Will squash commits after an OK is given. Not completely sure this is the proper way to go about doing this.

@bridadan

This comment has been minimized.

Contributor

bridadan commented Mar 16, 2018

@cmonr I'd change your check to the following:

#if !defined(TARGET_NRF52840_DK) || !defined(TARGET_NRF52_DK)

That should disable the test just for NRF52840_DK and NRF52_DK.

@bridadan

This comment has been minimized.

Contributor

bridadan commented Mar 16, 2018

Another option is you could use those same checks to increase the expected delta for those targets.

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Mar 16, 2018

I was thinking we protect the test case itself, instead of a pass

Case cases[] = {
    Case("Flash - init", flash_init_test),
    Case("Flash - mapping alignment", flash_mapping_alignment_test),
    Case("Flash - erase sector", flash_erase_sector_test),
    Case("Flash - program page", flash_program_page_test),
    Case("Flash - buffer alignment test", flash_buffer_alignment_test),
#if !defined(TARGET_NRF52840_DK) || !defined(TARGET_NRF52_DK)
    Case("Flash - clock and cache test", flash_clock_and_cache_test),
#endif
};
@SenRamakri

This comment has been minimized.

Contributor

SenRamakri commented Mar 16, 2018

Yes, its better if we can avoid running the tests on targets where its not supported instead of providing a "False Pass" indication.

@bridadan

This comment has been minimized.

Contributor

bridadan commented Mar 16, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 16, 2018

Will make changes per recommendations.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 19, 2018

Will make changes per recommendations.

@cmonr Can we resolve this today? asap

@theotherjimmy

@cmonr This is not how we disable tests for a target, or series of targets.

Instead, could you add a

#ifdef NORDIC
#error [NOT SUPPORTED] ...
#endif

somewhere in this file.

@theotherjimmy

I take it back. I must have had an old cached copy of the code, where all tests were disabled.

@cmonr cmonr force-pushed the cmonr:disable-failing-flash-test branch from 7f54bb7 to f18ecd1 Mar 19, 2018

@cmonr cmonr added needs: review and removed needs: work labels Mar 19, 2018

Disabled flash clock and cache test for NRF52 MCUs.
This is meant to be a temporary fix until the issue has been root caused, and Jenkins CI is no longer intermittently failing.

@studavekar studavekar referenced this pull request Mar 19, 2018

Merged

LoRa refactoring #6279

1 of 5 tasks complete

@cmonr cmonr force-pushed the cmonr:disable-failing-flash-test branch from f18ecd1 to 9f63013 Mar 19, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 19, 2018

@theotherjimmy

Pretty simple.

@bridadan

No problems here

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Mar 20, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Mar 20, 2018

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@0xc0170

We shall not forget to fix this and reenable it later :)

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 20, 2018

cc @marcuschangarm (to be aware of this)

@mbed-ci

This comment has been minimized.

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Mar 20, 2018

@cmonr @0xc0170 @theotherjimmy should we merge this one?

@0xc0170 0xc0170 merged commit 447e96e into ARMmbed:master Mar 20, 2018

11 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-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
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Passed, code size is 10060B
Details
travis-ci/tools Local tools testing has passed
Details
@cmonr

This comment has been minimized.

Contributor

cmonr commented Apr 2, 2018

Making a note here. This PR does not successfully disable the test.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 3, 2018

Making a note here. This PR does not successfully disable the test.

Looking at the macro, would it be better TARGET_MCU_NRF52 ? would that solve it or what is the problem?

@cmonr

This comment has been minimized.

Contributor

cmonr commented Apr 3, 2018

Looking at the macro, would it be better TARGET_MCU_NRF52 ? would that solve it or what is the problem?

I think that would be a better macro. Working on it right now. Will provide pre/post test logs with new PR to confirm the test is actually disabled.

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