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.

Sign up for GitHub

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

Jump to bottom

Run Test Application on target #13

Merged
merged 3 commits into from
Mar 8, 2022
Merged

Run Test Application on target #13

merged 3 commits into from
Mar 8, 2022

Conversation

tore-espressif
Copy link
Collaborator

@tore-espressif tore-espressif commented Feb 17, 2022 •
edited
Loading

The artifacts will be used you 'run_test' action

@tore-espressif tore-espressif force-pushed the feature/ci/test_app branch 9 times, most recently from 6c1460c to 1db10bc Compare February 22, 2022 10:10
@tore-espressif tore-espressif changed the title ci: Upload build artifacts Run Test Application in QEMU Feb 22, 2022
@tore-espressif
Copy link
Collaborator Author

FYI @igrr

@tore-espressif tore-espressif mentioned this pull request Feb 23, 2022
Closed
@tore-espressif tore-espressif force-pushed the feature/ci/test_app branch 15 times, most recently from 306a29d to a0884cc Compare February 24, 2022 09:19
@tore-espressif tore-espressif changed the title Run Test Application in QEMU Run Test Application on target Feb 24, 2022
@tore-espressif tore-espressif force-pushed the feature/ci/test_app branch 2 times, most recently from 5a664f4 to 19f82b7 Compare February 24, 2022 11:47
@espressif espressif deleted a comment from github-actions bot Feb 28, 2022
@espressif espressif deleted a comment from github-actions bot Feb 28, 2022
@espressif espressif deleted a comment from github-actions bot Feb 28, 2022
@espressif espressif deleted a comment from github-actions bot Feb 28, 2022
@espressif espressif deleted a comment from github-actions bot Feb 28, 2022
@espressif espressif deleted a comment from github-actions bot Feb 28, 2022
@tore-espressif tore-espressif force-pushed the feature/ci/test_app branch 2 times, most recently from b73e5c6 to a0cc600 Compare February 28, 2022 18:23
@tore-espressif
Copy link
Collaborator Author

@mahavirj @igrr @hfudev @tomassebestik @kumekay @Ouss4 PTAL

In this PR:

  1. All components are build in CI (even if they don't have unit tests)
  2. Unit test_app is run on our self-hosted runners. It uses pytest-embedded test framework.
  3. Test results are parsed by github action bot.

hfudev
hfudev reviewed Mar 1, 2022
View reviewed changes
Copy link
Member

@hfudev hfudev left a comment

Choose a reason for hiding this comment

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

nice job. LGTM!

.github/workflows/build_and_run_test_app.yml Show resolved Hide resolved
igrr
igrr approved these changes Mar 1, 2022
View reviewed changes
Copy link
Member

@igrr igrr left a comment

Choose a reason for hiding this comment

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

Looks very neat, I really like the idea to publish the test result in the PR comment.
All comments are pretty minor. You can note them down and fix in another PR if you like.

Just one stupid question: have you tested that CI job actually fails when one of the unit tests fails? Looking at the CI yml file, i think it should, but I'm not 100% sure.

.github/workflows/build_and_run_test_app.yml Show resolved Hide resolved
.github/workflows/build_and_run_test_app.yml Outdated Show resolved Hide resolved
test_app/main/test_app_main.c Outdated Show resolved Hide resolved
test_app/CMakeLists.txt Outdated Show resolved Hide resolved
@Ouss4
Copy link

Ouss4 commented Mar 2, 2022

Just one stupid question: have you tested that CI job actually fails when one of the unit tests fails? Looking at the CI yml file, i think it should, but I'm not 100% sure.

I think it should be that pytest returns a non-zero value for a failed test, that would make its step and workflow fail too.

@tore-espressif tore-espressif force-pushed the feature/ci/test_app branch 3 times, most recently from b821d66 to 806f40b Compare March 3, 2022 10:26
@tore-espressif tore-espressif mentioned this pull request Mar 3, 2022
Closed
@tore-espressif
Copy link
Collaborator Author

Just one stupid question: have you tested that CI job actually fails when one of the unit tests fails? Looking at the CI yml file, i think it should, but I'm not 100% sure.

I think it should be that pytest returns a non-zero value for a failed test, that would make its step and workflow fail too.

Yes, the CI failed in case of failed tests. However, the test results were not propagated to next steps in case of failure. It is fiexd now.

See here for results with tampered expat test https://github.com/espressif/idf-extra-components/runs/5405924004

@tore-espressif
Copy link
Collaborator Author

@kumekay @igrr @Ouss4 @hfudev Thank you very much for your reviews!

I've updated the PR accordingly, please let me know whether you agree with the proposed resolutions. Thanks

@tore-espressif tore-espressif self-assigned this Mar 3, 2022
@tore-espressif
Copy link
Collaborator Author

I'll merge today, if there are no objections

@tore-espressif tore-espressif merged commit d99c3ea into master Mar 8, 2022
@tore-espressif tore-espressif deleted the feature/ci/test_app branch March 8, 2022 09:25
@tore-espressif tore-espressif mentioned this pull request Mar 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hfudev hfudev hfudev left review comments

@Ouss4 Ouss4 Ouss4 left review comments

@kumekay kumekay kumekay left review comments

@igrr igrr igrr approved these changes

Assignees

@tore-espressif tore-espressif

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tore-espressif @Ouss4 @kumekay @igrr @hfudev