Skip to content

Remove #ifndef NO_GREENTEA from tests #9914

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 1 commit into from
Mar 15, 2019
Merged

Remove #ifndef NO_GREENTEA from tests #9914

merged 1 commit into from
Mar 15, 2019

Conversation

orenc17
Copy link
Contributor

@orenc17 orenc17 commented Mar 3, 2019

Description

Remove #ifndef NO_GREENTEA from PSA tests as #9835 was merged

wait for #9823

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[X] Test update
[ ] Breaking change

Reviewers

@ARMmbed/mbed-os-psa @ARMmbed/mbed-os-maintainers

Release Notes

@ciarmcom ciarmcom requested review from a team March 3, 2019 14:00
@ciarmcom
Copy link
Member

ciarmcom commented Mar 3, 2019

@orenc17, thank you for your changes.
@ARMmbed/mbed-os-hal @ARMmbed/mbed-os-test @ARMmbed/mbed-os-crypto @ARMmbed/mbed-os-maintainers please review.

Copy link
Contributor

@NirSonnenschein NirSonnenschein left a comment

Choose a reason for hiding this comment

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

LGTM

@orenc17
Copy link
Contributor Author

orenc17 commented Mar 5, 2019

@ARMmbed/mbed-os-maintainers review is done, can we get this merged?

@jamesbeyond
Copy link
Contributor

Hey @orenc17, I got a question here, if wen have NO_GREENTEA defined in greentea_test_env.cpp, why do we still need this in each test?

@orenc17
Copy link
Contributor Author

orenc17 commented Mar 5, 2019

@jamesbeyond This PR REMOVES all the excess defines from the tests

@jamesbeyond
Copy link
Contributor

jamesbeyond commented Mar 5, 2019

Sorry, I was mistaken this PR as adding them. one thing to mention, maybe that Macro should be called NO_GREENTEA_SYNC which is more make sense

Copy link
Contributor

@jamesbeyond jamesbeyond 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!

@cmonr
Copy link
Contributor

cmonr commented Mar 5, 2019

review is done, can we get this merged?

Sure, but at the moment, this is marked for 5.12.1. Until RC1 is generated, and the 5.12 branch made, we can't merge 5.12.1 PRs.

This could come into 5.12.1, but that would require an ACK from @ChiefBureaucraticOfficer

@cmonr
Copy link
Contributor

cmonr commented Mar 5, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Mar 6, 2019

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 1
Build artifacts

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 14, 2019

@orenc17 quick question - is this still up to date (been few days on hold due to 5.12rcs). If yes, can go in

@orenc17
Copy link
Contributor Author

orenc17 commented Mar 14, 2019

I'll rebase and verify

@orenc17
Copy link
Contributor Author

orenc17 commented Mar 14, 2019

@0xc0170 rebased and removed another usage of #ifdef NO_GREENTEA from PSA compliance tests

ready for CI

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 15, 2019

No more changes are expected there.

This was added to the rollup PR #10116 and it's currently in CI. If any new commit comes, please let us know immediately.

@0xc0170 0xc0170 merged commit fc97a75 into ARMmbed:master Mar 15, 2019
@0xc0170 0xc0170 removed the needs: CI label Mar 15, 2019
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.

9 participants