Skip to content

mbed test: add argument --ignore to allow passing in mbedignore patterns #6833

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

Merged
merged 2 commits into from
May 14, 2018

Conversation

andrewleech
Copy link
Contributor

Description

This patch adds an extra arg --ignore to test.py for use when running unit tests.
--ignore lets you specify a comma separated list of extra lines to dynamically add to mbedignore for the building of tests.

In particular this simplifies ignoring a project main.cpp file when running unit tests to remove the multiple definition of main(void) function.

Example:

python mbed-os/tools/test.py -t GCC_ARM -m NRF51_DK --source . --build BUILD\tests\NRF51_DK\GCC_ARM --test-spec BUILD\tests\NRF51_DK\GCC_ARM\test_spec.json --ignore src/main.cpp,src/errors.cpp

For this to be used with mbed cli tool the --ignore arg needs to be added there to only pass it on to test.py, else it gets passed to mbed-gt tool as well which then fails due to unknown arg.

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[x] Feature
[ ] Breaking change

andrewleech pushed a commit to andrewleech/mbed-cli that referenced this pull request May 8, 2018
Simply passes the parameter through when building with test.py

For more details see ARMmbed/mbed-os#6833
@0xc0170 0xc0170 requested a review from theotherjimmy May 8, 2018 08:51
@0xc0170
Copy link
Contributor

0xc0170 commented May 8, 2018

Please resolve the conflict

Copy link
Contributor

@theotherjimmy theotherjimmy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the implementation, except that it adds a parameter to build_library, which already has ~25 arguments.

@andrewleech
Copy link
Contributor Author

Wow it's hard to write code for such a fast moving target as mbed sometimes! I built this in a couple of hours from the head of master just yesterday and it's already in conflict!

Yes I get that the build library function has a lot of args already (what's one more then) but I don't see that as a particularly big problem? I don't think it's worth a refactor just because it's big... especially as that'll cause merge fails for the next contributor ;-)

@andrewleech
Copy link
Contributor Author

Speaking of merge issues, particularly when you've got functions with lots of args.... this is why black always formats args one per line with a trailing bracket detented. It takes some getting used to the look of it, but it does have practical purpose.
https://github.com/ambv/black#how-black-wraps-lines

…re args

In particular this allows ignoring a project main.cpp file when running unit tests
@theotherjimmy
Copy link
Contributor

@andrewleech Yeah, I'll trim down the number of arguments. I mentioned that because a large number of arguments indicates that we have a leaky abstraction or a poorly factored set of functions. I'm working on this, and I can already see how to fit your extension into my new vision, so I'm not so worried about it :D


You're welcome do change the argument style to the one-per-line style, as long as you do it in another commit!

@screamerbg screamerbg self-requested a review May 9, 2018 16:50
screamerbg
screamerbg previously approved these changes May 9, 2018
@theotherjimmy
Copy link
Contributor

@andrewleech Black looks like a nice tool. Thanks for the link. I may apply it over the code base one file at a time :D

@theotherjimmy
Copy link
Contributor

/morph build

@mbed-ci
Copy link

mbed-ci commented May 9, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented May 9, 2018

@mbed-ci
Copy link

mbed-ci commented May 10, 2018

Copy link
Contributor

@theotherjimmy theotherjimmy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewleech I just noticed that you did not pass on the ignore from build_tests to individual invocations of build_project. Why is that?

@andrewleech
Copy link
Contributor Author

@theotherjimmy I initially only implemented the minimum required to add the option to unit tests, specifically to resolve the very old issue of multiple main's when running mbed test.

I can see a purpose to having this arg for other mbed operations so have now adding it to (I think) all compile / build / export operations.

Copy link
Contributor

@theotherjimmy theotherjimmy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks.

@theotherjimmy
Copy link
Contributor

Travis failures were fetch failures. I restarted travis.

@cmonr
Copy link
Contributor

cmonr commented May 12, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented May 12, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented May 12, 2018

@mbed-ci
Copy link

mbed-ci commented May 12, 2018

@cmonr
Copy link
Contributor

cmonr commented May 12, 2018

/morph mbed2-build

@cmonr cmonr merged commit d3cc4e1 into ARMmbed:master May 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants