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

Add CMakeLists for USB greentea tests #14937

Closed
wants to merge 1 commit into from
Closed

Add CMakeLists for USB greentea tests #14937

wants to merge 1 commit into from

Conversation

wernerlewis
Copy link
Contributor

Summary of changes

Fixes #14886

CMake definitions are added for USB greentea tests. These are required
to build and run greentea tests using mbed-tools with CTest.

Impact of changes

Migration actions required

Documentation

None

Pull request type 

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

All tests successfully build using cmake with required USB_DEVICE_TESTS macro. Greentea test binaries flash and run correctly using mbedhtrun .

Reviewers

@rajkan01
Copy link
Contributor

@wernerlewis Already rob has a PR for running the greentea test using CTest as part of that, he already updated mbed-drivers-ticker CMake to use with CTest 69e3065, better to add USB test CMake similar to mbed-drivers-ticker one

@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Jul 20, 2021
@ciarmcom ciarmcom requested a review from a team July 20, 2021 13:30
@ciarmcom
Copy link
Member

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

LDong-Arm
LDong-Arm previously requested changes Jul 23, 2021
View reviewed changes
Copy link
Contributor

@LDong-Arm LDong-Arm left a comment

Choose a reason for hiding this comment

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

This PR needs #14902 too. Please look for [NOT_SUPPORTED] in drivers/usb/tests/TESTS/, and add an equivalent CMake check in each CMakeLists.txt and pass TEST_SKIPPED to mbed_greentea_add_test().

@ciarmcom
Copy link
Member

ciarmcom commented Aug 4, 2021

This pull request has automatically been marked as stale because it has had no recent activity. @wernerlewis, please carry out any necessary work to get the changes merged. Thank you for your contributions.

@ciarmcom ciarmcom added the stale Stale Pull Request label Aug 4, 2021
@adbridge adbridge removed the stale Stale Pull Request label Aug 10, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Aug 12, 2021

This PR needs #14902 too. Please look for [NOT_SUPPORTED] in drivers/usb/tests/TESTS/, and add an equivalent CMake check in each CMakeLists.txt and pass TEST_SKIPPED to mbed_greentea_add_test().

The PR got in. @wernerlewis will you look at not supported and skipped now?

@mergify mergify bot dismissed LDong-Arm’s stale review August 13, 2021 10:16

Pull request has been modified.

@0xc0170 0xc0170 requested a review from LDong-Arm August 17, 2021 10:02
Patater
Patater previously approved these changes Aug 18, 2021
View reviewed changes
Copy link
Contributor

@Patater Patater left a comment

Choose a reason for hiding this comment

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

LGTM

drivers/usb/CMakeLists.txt Show resolved Hide resolved
@mergify mergify bot added needs: CI and removed needs: review labels Aug 18, 2021
@Patater
Copy link
Contributor

Patater commented Aug 18, 2021

CI started

@mbed-ci
Copy link

mbed-ci commented Aug 18, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 24, 2021

Set back to review , please @wernerlewis review the comment above

@wernerlewis
Copy link
Contributor Author

Given that USB_DEVICE_TESTS=1 is still needed by Mbed CLI 1, we may not remove it from the tests' .cpp files. But USB has EXCLUDE_FROM_ALL in CMake and won't be built by default anyway, shall we always set this macro in each test's CMakeLists.txt using target_compile_definitions() to make sure the tests are buildable "on request" with CMake? And add a note there.

This works for building the USB tests, however when building ALL, CTest will then add the USB tests. Due to EXCLUDE_FROM_ALL the targets will still not be built, but CTest will then try to run them and fail each time. I'm not sure if there is a method in CMake to allow setting the macro if these targets are being built and pass TEST_SKIPPED otherwise, or for CTest to skip targets that were not built.

@LDong-Arm
Copy link
Contributor

LDong-Arm commented Aug 24, 2021 •
edited
Loading

Due to EXCLUDE_FROM_ALL the targets will still not be built, but CTest will then try to run them and fail each time. I'm not sure if there is a method in CMake to allow setting the macro if these targets are being built and pass TEST_SKIPPED otherwise, or for CTest to skip targets that were not built.

CTest tests are declared at configuration time, so I don't think there's a way to achieve that. Now what I can think of is keeping what this PR already does, and letting users pass --app-config with a json file that defines USB_DEVICE_TESTS=1, and we provide this file in TESTS/configs/ like other json files there. Or, declare a CMake option option(MBED_USB_TESTS OFF) which users can override.

@mergify mergify bot dismissed Patater’s stale review August 26, 2021 08:18

Pull request has been modified.

@ciarmcom ciarmcom added the stale Stale Pull Request label Aug 27, 2021
@ciarmcom
Copy link
Member

This pull request has automatically been marked as stale because it has had no recent activity. , please complete review of the changes to move the PR forward. Thank you for your contributions.

@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Aug 27, 2021
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Sep 3, 2021
@wernerlewis @LDong-Arm
Add CMakeLists for USB greentea tests
59539c6 
CMake definitions are added for USB greentea tests. These are required
to build and run greentea tests using CMake, and enables CTest support
for use with mbed-tools.

Fixes ARMmbed#14886
@LDong-Arm
Copy link
Contributor

@ARMmbed/mbed-os-core FYI I'm taking over this PR and directly pushing to @wernerlewis's fork. Now working on my own review comment.

@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Sep 9, 2021
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Sep 17, 2021
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Sep 24, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Sep 28, 2021

@LDong-Arm Shall we close this and open a new PR?

@ciarmcom ciarmcom removed the stale Stale Pull Request label Sep 28, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 9, 2022

We are closing this pull request due to inactivity

@0xc0170 0xc0170 closed this Feb 9, 2022
@mergify mergify bot removed needs: review component: hal component: core release-type: patch Indentifies a PR as containing just a patch labels Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Patater Patater Patater left review comments

@LDong-Arm LDong-Arm LDong-Arm left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing CMake support for USB tests
9 participants
@wernerlewis @rajkan01 @ciarmcom @0xc0170 @Patater @mbed-ci @LDong-Arm @adbridge @rwalton-arm