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

TEST: update python script to enable examples smoke test #10521

Merged
merged 5 commits into from May 21, 2019

Conversation

Projects
None yet
9 participants
@jamesbeyond
Copy link
Contributor

commented May 2, 2019

Description

The goal of this PR is to enable examples smoke test in CI

  • the new scripts will check examples.json if contains 'test', 'compare_log', 'baud_rate' keys
  • the new scripts will dump test_spec.json test in examples compiled successfully

The auto generated test_spec.json can be consumed by GreenTea.
This PR is related with ARMmbed/mbed-os-tools#162

examples.json is not required to be changed at moment.
later on when test cases in each example are ready, corresponding example then will need to be updated.

Pull request type

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

Reviewers

@OPpuolitaival

@ciarmcom ciarmcom requested review from OPpuolitaival and ARMmbed/mbed-os-maintainers May 2, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented May 2, 2019

TEST: update python script to enable example smoke test
 * it will check examples.json if contains 'test', 'compare_log', 'baud_rate' keys
 * it will dump test_spec.json test in examples compiled successfully

@jamesbeyond jamesbeyond force-pushed the jamesbeyond:example_test branch from 226aa33 to b2611fb May 2, 2019

@bridadan
Copy link
Contributor

left a comment

In general this all seems pretty reasonable! I have a few suggestions below,

"""save the given json data to test_spec.json"""
print ("Dumping json test_specs file {}".format(name))
with open(name, 'w') as outfile:
json.dump(json_spec, outfile , indent=4)

This comment has been minimized.

Copy link
@bridadan

bridadan May 2, 2019

Contributor

I believe we have an existing function that you can use: https://github.com/ARMmbed/mbed-os/blob/master/tools/utils.py#L184

This comment has been minimized.

Copy link
@jamesbeyond

jamesbeyond May 10, 2019

Author Contributor

Thanks for the review, all your comments been addressed

if image:
image_info = [{"binary_type": "bootable","path": normpath(join(name,image)),"compare_log":log}]
else:
print ("Warning: could not found built image for example %s" % name)

This comment has been minimized.

Copy link
@bridadan

bridadan May 2, 2019

Contributor

nit pick on the wording of the warning:

Suggested change
print ("Warning: could not found built image for example %s" % name)
print ("Warning: could not find built image for example %s" % name)
print ("\n#### STDOUT ####\n%s\n#### STDERR ####\n%s\n#### End of STDOUT/STDERR ####\n" % (std_out,std_err))

if test_example:
log = example['compare_log'].pop(0)

This comment has been minimized.

Copy link
@bridadan

bridadan May 2, 2019

Contributor

Any reason this needs to be a pop vs the following:

Suggested change
log = example['compare_log'].pop(0)
log = example['compare_log']

This comment has been minimized.

Copy link
@jamesbeyond

jamesbeyond May 10, 2019

Author Contributor

Here pop() is used because the value of compare_log will be a list of logs files.
The reason for that is because some of the example repos contains more than one example, e.g. the mbed-os-example-tls and mbed-os-example-ble. So the each log file need to match with
the individual sub-example. So we need to pop the file out of list regardless the compilation for each sub-example pass of fail

This comment has been minimized.

Copy link
@mark-edgeworth

mark-edgeworth May 15, 2019

Contributor

May I request that this comment is added to the code please.

test_json['builds'][test_group]['tests'][name]={"binaries":image_info}
else:
print("Warning: Test for %s will not be generated." % name)
print("One or more of 'test' 'baud_rate' and 'compare_log' keys are missing from the json file\n")

This comment has been minimized.

Copy link
@bridadan

bridadan May 2, 2019

Contributor

nit pick on wording, missing some commas:

Suggested change
print("One or more of 'test' 'baud_rate' and 'compare_log' keys are missing from the json file\n")
print("One or more of 'test', 'baud_rate', and 'compare_log' keys are missing from the example config json file\n")
Show resolved Hide resolved tools/test/examples/examples_lib.py Outdated

@0xc0170 0xc0170 added needs: work and removed needs: review labels May 3, 2019

@0xc0170

0xc0170 approved these changes May 3, 2019

Copy link
Member

left a comment

Besides above suggestions, LGTM

@OPpuolitaival
Copy link
Contributor

left a comment

Need documenting at least

@@ -380,31 +382,60 @@ def compile_repos(config, toolchains, targets, profile, verbose, examples):
successes = []
compiled = True
pass_status = True
if example.has_key('test') and example.has_key('baud_rate') and example.has_key('compare_log'):

This comment has been minimized.

Copy link
@OPpuolitaival

OPpuolitaival May 3, 2019

Contributor

This is true for this kind of json object:
{
"name": "Something",
"test": False,
"baund_rate": "invalid",
"compare_log": null
}

Why it need to have "test" key even if the value is not documented or does not even matter?

These values really need to be documented. Actually same problem with whole examples.json. @adbridge do you know any documentation for that json file?

This comment has been minimized.

Copy link
@adbridge

adbridge May 3, 2019

Contributor

No not that i am aware of....

This comment has been minimized.

Copy link
@jamesbeyond

jamesbeyond May 3, 2019

Author Contributor

To me, the test key is just a on/off switch. similar to the compile key, just following the conventions.
It can be used, when we have the compare_log and baud_rate ready, but choose not run the test for some reasons. (e.g. the example requires some special HW, but that's not available on the RaaS, or there are some breaking change happening, we need to turn off tests for some examples temperately )

This comment has been minimized.

Copy link
@OPpuolitaival

OPpuolitaival May 6, 2019

Contributor

@jamesbeyond then the condition need to check the value of example['test'] alse. For example:
if example.has_key('test') and example['test'] and example.has_key('baud_rate') and example.has_key('compare_log'):

This comment has been minimized.

Copy link
@jamesbeyond

jamesbeyond May 10, 2019

Author Contributor

Yes, I was checking that condition in later code, after reviewing, I think what you suggested would be more make scene. So this is fixed

@jamesbeyond

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

all comments been addressed, please review again @bridadan @OPpuolitaival

@bridadan
Copy link
Contributor

left a comment

Thanks for addressing my comments!

@0xc0170

This comment has been minimized.

Copy link
Member

commented May 13, 2019

While waiting for final approvals, CI started

@mbed-ci

This comment has been minimized.

Copy link

commented May 13, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 added needs: review and removed needs: work labels May 13, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented May 15, 2019

@OPpuolitaival @adbridge Happy with this?

PRs to examples are slowly getting integrated

@adbridge
Copy link
Contributor

left a comment

It would be better to use the logging module rather than print but I'm not going to block the PR on that. That could always be a downline improvement.

@jamesbeyond

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

It would be better to use the logging module rather than print but I'm not going to block the PR on that. That could always be a downline improvement.

Yeah, I just following the original script that using the print. I agree logging module would be a better solution. I think we will consider the improvements in separate PR

@adbridge

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

@mark-edgeworth are you happy with the comment?

@mark-edgeworth
Copy link
Contributor

left a comment

Yes, that's what I had in mind 👍

@jamesbeyond

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

I think we got good level of reviews, can we have the CI start? @ARMmbed/mbed-os-maintainers

@0xc0170 0xc0170 removed the needs: review label May 20, 2019

@0xc0170 0xc0170 added the needs: CI label May 20, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented May 20, 2019

CI started

@jamesbeyond

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

seems something wrong with checkout stage of the CI? @ARMmbed/mbed-os-maintainers

@cmonr

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

Pipeline restarted.
Recently, GitHub has increased the degree of repo clone throttling.

@mbed-ci

This comment has been minimized.

Copy link

commented May 20, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 3
Build artifacts

@0xc0170 0xc0170 merged commit 7d0cc69 into ARMmbed:master May 21, 2019

26 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/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage Success
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8685 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8448B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
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.