Skip to content
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

Greentea tests: user-configurable timeouts #9718

Merged

Conversation

Projects
None yet
8 participants
@michalpasztamobica
Copy link
Contributor

commented Feb 14, 2019

Description

Now it is enough to add:
"macros": [
"MBED_GREENTEA_TEST_XXXSOCKET_TIMEOUT_S=20"
],
to mbed_app.json, where XXX is on of {DNS, TLS, UDP, TCP}.

Pull request type

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

Reviewers

@SeppoTakalo
@AriParkkila
@pekkapesu

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

macros , not rather test config?

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

@0xc0170 , could you elaborate on this idea or point me to some existing test config?

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

@0xc0170 , do you mean adding something into the "config" section of the mbed_app.json? Like so:

{
    "config": {
        "tcp_timeout" : {
            "help" : "TCP timeout",
            "value" : 100
        },

? This would make sense, indeed. I didn't think about it...

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

I recall some tools had some problems with "MBED_GREENTEA_TEST_XXXSOCKET_TIMEOUT_S=20" but can't recall further details at the moment.

I am not fan of having macros used this way (setting default values and expect a user to overwrite them), rather use config.

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

Well... test cases cannot expose mbed_lib.json so they cannot easily expose configuration values.

We do configure test cases, but that is done through mbed_app.json through well known configuration values. Like this: https://github.com/ARMmbed/mbed-os/blob/master/tools/test_configs/HeapBlockDeviceAndWifiInterface.json

I'm OK with both approaches, macros or known mbed_app.json values. I want default that work 90 % of the time, and is not required to specify anywhere. And the rest is 10 % which is Cellular testing and requires specific environment anyway.

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

I assume the configs should be used for something that changes frequently (like communication baudrate, shield board type, etc.). Macros seem to be more of a possibility to make a temporary hack (like heap size increase).

I think macros are a better choice in our case. As Seppo suggested: most of the time we want to keep the same value of the timeout. In few special runs, at the moment only for cellular modules, we want to allow some larger timeouts and we want to avoid hacking the source code to do that.
I honestly don't fully support the idea of ports and server addresses for echo/discard tests being configured via the configs - they almost never change.

@0xc0170 , would you agree with the above?
@AriParkkila @pekkapesu , do you have any strong opinion here?

@ciarmcom ciarmcom requested review from AriParkkila, pekkapesu, SeppoTakalo and ARMmbed/mbed-os-maintainers Feb 14, 2019

@ciarmcom

This comment has been minimized.

@ciarmcom ciarmcom requested a review from ARMmbed/mbed-os-test Feb 14, 2019

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:greentea_tcpsocket_suite_timeout branch Feb 15, 2019

@VeijoPesonen
Copy link
Contributor

left a comment

I assume tests-network-interface doesn't require anything similar

@AriParkkila

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

I assume tests-network-interface doesn't require anything similar

I guess it might due to joining to cellular network may take minutes...

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

I assume tests-network-interface doesn't require anything similar

I guess it might due to joining to cellular network may take minutes...

Ok, I will add them in a minute.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:greentea_tcpsocket_suite_timeout branch Feb 15, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

Please fix astyle error (1 line)

@0xc0170 0xc0170 added needs: work and removed needs: review labels Feb 15, 2019

Greentea tests: user-configurable timeouts
Now it is enough to add:
    "macros": [
        "MBED_GREENTEA_TEST_XXXSOCKET_TIMEOUT_S=20"
    ],
to mbed_app.json, where XXX is on of {DNS, TLS, UDP, TCP}.
Also network-* tests are now configurable: network-interface, network-wifi, network-emac with a similar macro.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:greentea_tcpsocket_suite_timeout branch to ca57bdd Feb 15, 2019

@cmonr cmonr added needs: review and removed needs: work labels Feb 15, 2019

@cmonr

cmonr approved these changes Feb 15, 2019

Copy link
Contributor

left a comment

Neat!

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 15, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter
@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

CI was restart, all green

@0xc0170 0xc0170 merged commit 31da50e into ARMmbed:master Feb 18, 2019

25 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/ARMmbed/mbed-os/mbed-os-ci/PR-9718/build-ARM Success
Details
jenkins-ci/ARMmbed/mbed-os/mbed-os-ci/PR-9718/build-ARMC6 Success
Details
jenkins-ci/ARMmbed/mbed-os/mbed-os-ci/PR-9718/build-GCC_ARM Success
Details
jenkins-ci/ARMmbed/mbed-os/mbed-os-ci/PR-9718/build-IAR Success
Details
jenkins-ci/ARMmbed/mbed-os/mbed-os-ci/PR-9718/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/ARMmbed/mbed-os/mbed-os-ci/PR-9718/exporter Success
Details
jenkins-ci/ARMmbed/mbed-os/mbed-os-ci/PR-9718/greentea-test Success
Details
jenkins-ci/ARMmbed/mbed-os/mbed-os-ci/PR-9718/mbed2-build-ARM Success
Details
jenkins-ci/ARMmbed/mbed-os/mbed-os-ci/PR-9718/mbed2-build-GCC_ARM Success
Details
jenkins-ci/ARMmbed/mbed-os/mbed-os-ci/PR-9718/mbed2-build-IAR Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9072 cycles (-292 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details
@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

Interesting. Unsure why this didn't want to be applied to the mbed-os-5.11 branch. Suspect that a config param was introduced into 5.12. Retargeting this PR for 5.12.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.