Skip to content

Add define to skip greentea sync #9835

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
Mar 1, 2019
Merged

Add define to skip greentea sync #9835

merged 2 commits into from
Mar 1, 2019

Conversation

orenc17
Copy link
Contributor

@orenc17 orenc17 commented Feb 25, 2019

Description

Add a define to allow debuging greentea tests that do not require input from host without host-tests

Pull request type

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

Reviewers

Release Notes

@ciarmcom ciarmcom requested a review from a team February 25, 2019 14:00
@ciarmcom
Copy link
Member

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

@cmonr
Copy link
Contributor

cmonr commented Feb 25, 2019

Hmm. @orenc17 Would you mind sharing how you'd expect this to be used?

Does this new macro also need a Docs PR and Release Notes blurb?

@orenc17
Copy link
Contributor Author

orenc17 commented Feb 25, 2019

Hmm. @orenc17 Would you mind sharing how you'd expect this to be used?

Does this new macro also need a Docs PR and Release Notes blurb?

@cmonr let's call it an easter egg

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 26, 2019

I also do not understand this change. What use case is there and why greentea would include "no greentea" macro?

@0xc0170 0xc0170 requested a review from a team February 26, 2019 11:54
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 26, 2019

@ARMmbed/mbed-os-core Can you review, please

@NirSonnenschein
Copy link
Contributor

This is the general idea: tests are usually run using greentea and this is very convenient , except when debugging them. if a test is complied for greentea it will wait for a green tea command before starting making debugging it a pain. for these scenarios it is useful to build the same test using exactly the same config but leave out the greentea setup call, which will yield a very similar binary, but one that doesn't wait on startup and is easy to debug.

@cmonr
Copy link
Contributor

cmonr commented Feb 27, 2019

Hmm, considering that the use of this macro isn't new (https://github.com/ARMmbed/mbed-os/search?q=NO_GREENTEA&unscoped_q=NO_GREENTEA), I think it's fine to bring in.

It's also curious that the ARM_MUSCA_A1 PR is also using it: https://github.com/ARMmbed/mbed-os/pull/9221/files#diff-088d539546fc854c07455c735a069539R441

https://github.com/ARMmbed/mbed-os-5-docs/search?q=NO_GREENTEA&unscoped_q=NO_GREENTEA

Considering that this is already in use in the repo, this needs docs (CC @AnotherButler)

@cmonr cmonr requested a review from a team February 27, 2019 18:13
@orenc17
Copy link
Contributor Author

orenc17 commented Feb 27, 2019

@cmonr good catch

@cmonr
Copy link
Contributor

cmonr commented Feb 27, 2019

CI started whilst wainting on final reviews

@mbed-ci
Copy link

mbed-ci commented Feb 27, 2019

Test run: FAILED

Summary: 1 of 8 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM

@cmonr
Copy link
Contributor

cmonr commented Feb 27, 2019

Info: A CI config issue appears to be affecting NUMAKER_PFM_M2351 builds. Please ignore build errors against the target for now.

Other build failures should still be investigated, if any. Will restart CI when appropriate.

@cmonr cmonr added the risk: A label Feb 27, 2019
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 28, 2019

Hmm, considering that the use of this macro isn't new (https://github.com/ARMmbed/mbed-os/search?q=NO_GREENTEA&unscoped_q=NO_GREENTEA), I think it's fine to bring in.

Only some small subset, used in psa/spm files . Anyway, looks fine to me and also used something similar previously.

One last question before restarting CI: why not added as config as well with some help text. We got already config file there - if this macro is not documented (I would assume this would go only in the sources, not known to anyone).

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 28, 2019

    "config": {
        "skip_host_sync": {
            "help": "No greentea host involved, helps debugging tests with no host interaction",
            "value": 0
        },

Not sure if we shall rename but this is what I understood from this PR and how it was used (thus used skip in the name).
@orenc17 Please review

@orenc17
Copy link
Contributor Author

orenc17 commented Feb 28, 2019

@0xc0170 the idea of putting this define in an mbed_lib.json crossed my mind, but the thing is for a user to actually change this, the user will need to compile his test with an mbed_app.json
and from previous discussions this idea is frowned upon A LOT...

so i would like to stick with the very simple and stupid -DNO_GREENTEA

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 28, 2019

Valid point. Will need to be documented (otherwise this is hidden feature :) )

@ARMmbed/mbed-os-core please review

@alekla01
Copy link
Contributor

old build-ARM hook set green, so does not potentially cause issues when/if rebuilding without commits in between.

Copy link

@deepikabhavnani deepikabhavnani left a comment

Choose a reason for hiding this comment

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

Thanks for this addition 👍

@cmonr
Copy link
Contributor

cmonr commented Feb 28, 2019

CI restarted.

Afaik, restarting a single build job won't propigate successfully, so restarting from scratch

@mbed-ci
Copy link

mbed-ci commented Mar 1, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 2
Build artifacts

@cmonr cmonr merged commit 024cae5 into ARMmbed:master Mar 1, 2019
@cmonr cmonr removed the needs: CI label Mar 1, 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.

8 participants