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

Icetea hw restriction #8137

Merged
merged 3 commits into from Sep 17, 2018

Conversation

Projects
None yet
7 participants
@OPpuolitaival
Contributor

OPpuolitaival commented Sep 14, 2018

Description

Restrict the board where test are running. This fix the problem in case that user has multiple different boards connected to the machine same time

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Breaking change
@OPpuolitaival

This comment has been minimized.

Contributor

OPpuolitaival commented Sep 14, 2018

@jonikula @studavekar please review

def get_applications(test):
ret = list()
for dut in test['requirements']['duts'].values():
if 'application' in dut.keys() and 'name' in dut['application'].keys():
if 'application' in dut.keys() and 'name' in dut['applicati on'].keys():

This comment has been minimized.

@jonikula

This comment has been minimized.

@OPpuolitaival

OPpuolitaival Sep 14, 2018

Contributor

thanks, fixed

@OPpuolitaival OPpuolitaival force-pushed the OPpuolitaival:icetea_hw_restriction branch from f6aaff4 to ed18665 Sep 14, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 14, 2018

@jonikula Please take another look as Olli has updated this PR.

@OPpuolitaival

This comment has been minimized.

Contributor

OPpuolitaival commented Sep 14, 2018

@alekla01 please review

@cmonr cmonr requested a review from alekla01 Sep 14, 2018

@OPpuolitaival OPpuolitaival force-pushed the OPpuolitaival:icetea_hw_restriction branch from c648019 to ed18665 Sep 14, 2018

@cmonr cmonr added the risk: A label Sep 14, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 17, 2018

/morph build

@alekla01

(example, does not necessarily have anything to do with this pr)
target = 'K64F', ['1']['allowed_platforms'] = ['K66F']
doesn't this use 'K64F' binaries but flash 'K66F'?

@jupe

This comment has been minimized.

Contributor

jupe commented Sep 17, 2018

For me it looks like setting platform_name would be the correct way to do this. This now overwrites allowed_platforms list from test case.. Unless there is something I just don't get yet..

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 17, 2018

Build : SUCCESS

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

Triggering tests

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 17, 2018

@OPpuolitaival Please review the latest 2 comments, if this requires changes . Should be done asap if that is the case

@OPpuolitaival

This comment has been minimized.

Contributor

OPpuolitaival commented Sep 17, 2018

@jupe Current implementation work surely. You need to understand that it is about generated configuration and we need to limit target devices. Current idea is also that there should not be board specific test cases in Mbed OS. Now it works so that test application binary cannot be build if target board do not support those features. When application cannot be build the tests will be skipped.

@OPpuolitaival

This comment has been minimized.

Contributor

OPpuolitaival commented Sep 17, 2018

@alekla01 do you mean that tests are not real use cases? Yes those are just unit tests which validate function not the bigger use case

@alekla01

This comment has been minimized.

alekla01 commented Sep 17, 2018

@OPpuolitaival
No, i mean if you set ['*']['allowed_platforms'] = target, and the test requirements sets ['1']['allowed_platforms'] = [target, target2], target2 may still get flashed with target binary (if i am not mistaken). However, this is not an issue with the current tests and should not be and issue as long as 'allowed_platforms' is only used in ['*'] and not in ['1...10']. ('Current idea is also that there should not be board specific test cases in Mbed OS' -> 'allowed_platforms' is not used in ['1..10'] -> ok). Both allowed_platforms and platform_name work just as well in this case.

@mbed-ci

This comment has been minimized.

@jupe

This comment has been minimized.

Contributor

jupe commented Sep 17, 2018

Current idea is also that there should not be board specific test cases in Mbed OS

In that case this should overwrite allowed list with empty array (="no board specific test") and use platform_name -property to select platform under test explicitly.. Current implementation causes a bit confusion way how it uses allowed_platform -property even it "seems to work as expected" - it works just because icetea picks first platform from that list by default in case of user does not select explicitly platform_name (which was originally implemented kind of backward compatible reason)... Anyway, I'm fine with current approach - it's not perfect but it do it's job for now..

Another concern related to @alekla01 comment. Do we have/will have tests which require different applications per device ? That might causing more tuning here..

@mbed-ci

This comment has been minimized.

@cmonr cmonr added ready for merge and removed needs: CI labels Sep 17, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 17, 2018

@jupe @alekla01 @OPpuolitaival All approved but based on the comments, the discussion is not yet over. I believe this is good to go (needs some improvements later possibly). If not, now it's the time to request changes.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 17, 2018

Bringing this in, since we need to get RC3 generated in the next couple of hours. Further discussion can continue either here or in an issue (prefereably an issue).

@cmonr cmonr merged commit 7a0c9a6 into ARMmbed:master Sep 17, 2018

14 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0.0%) RAM(-0.12%)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job: successful
Details
travis-ci/astyle Passed, 598 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9883 cycles (-2 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
@OPpuolitaival

This comment has been minimized.

Contributor

OPpuolitaival commented Sep 17, 2018

@0xc0170 this solve the problem. Yes there might be some issues in future that we might need to change this a bit. The reason why this code is in this repository and not in mbed-cli is that icetea usage logic and test cases will be in sync

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