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

CMake: QSPIFBlockDevice: Guard unit test directory #15073

Merged
merged 1 commit into from
Sep 15, 2021

Conversation

rwalton-arm
Copy link
Contributor

Summary of changes

In the QSPIFBlockDevice component we checked if BUILD_TESTING was
enabled before adding the QSPIFBlockDevice/UNITTESTS subdirectory. In
the parent blockdevice/CMakeLists.txt we added the QSPIFBlockDevice
subdirectory to the build under two separate conditions:

  • when "QSPIF" is present in MBED_TARGET_LABELS
  • when building only unit tests

This wasn't quite enough, as when we build greentea tests for some
targets QSPIF is enabled in MBED_TARGET_LABELS, causing the
QSPIFBlockDevice subdirectory and its unit tests to be added when
building greentea tests. This caused a test failure on targets which
enable QSPIF:

The following tests FAILED:
	 40 - qspif-unittest_NOT_BUILT (Not Run)

To fix this we need to specifically check that we're not building
greentea tests before adding the QSPIFBlockDevice/UNITTESTS directory to
the project.

Part of this issue is our reliance on MBED_TARGET_LABELS to pull
features in to the build. We should refactor our usage of
MBED_TARGET_LABELS and use CMake target dependencies where possible.
Then we wouldn't need to add subdirectories under various, often
conflicting, scenarios. Instead the targets would always be available
and we would just choose which ones to actually build in different cases
using CMake target linking.

Impact of changes

Migration actions required

Documentation


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)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers


In the QSPIFBlockDevice component we checked if BUILD_TESTING was
enabled before adding the QSPIFBlockDevice/UNITTESTS subdirectory. In
the parent blockdevice/CMakeLists.txt we added the QSPIFBlockDevice
subdirectory to the build under two separate conditions:

* when "QSPIF" is present in MBED_TARGET_LABELS
* when building only unit tests

This wasn't quite enough, as when we build greentea tests for some
targets QSPIF is enabled in MBED_TARGET_LABELS, causing the
QSPIFBlockDevice subdirectory and its unit tests to be added when
building greentea tests. This caused a test failure on targets which
enable QSPIF:

```
The following tests FAILED:
	 40 - qspif-unittest_NOT_BUILT (Not Run)
```

To fix this we need to specifically check that we're not building
greentea tests before adding the QSPIFBlockDevice/UNITTESTS directory to
the project.

Part of this issue is our reliance on MBED_TARGET_LABELS to pull
features in to the build. We should refactor our usage of
MBED_TARGET_LABELS and use CMake target dependencies where possible.
Then we wouldn't need to add subdirectories under various, often
conflicting, scenarios. Instead the targets would always be available
and we would just choose which ones to actually build in different cases
using CMake target linking.
@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Sep 15, 2021
@ciarmcom
Copy link
Member

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

@LDong-Arm LDong-Arm added this to In progress (8) in Mbed Core via automation Sep 15, 2021
@LDong-Arm LDong-Arm moved this from In progress (8) to Reviewer approved & awaiting CI & merge (3) in Mbed Core Sep 15, 2021
@mergify mergify bot added needs: CI and removed needs: review labels Sep 15, 2021
@Patater
Copy link
Contributor

Patater commented Sep 15, 2021

CI started

@mbed-ci
Copy link

mbed-ci commented Sep 15, 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_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-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@Patater Patater merged commit 8698a55 into ARMmbed:master Sep 15, 2021
Mbed Core automation moved this from Reviewer approved & awaiting CI & merge (3) to Done Sep 15, 2021
@mergify mergify bot removed the ready for merge label Sep 15, 2021
@Patater Patater moved this from Done to Reported in Mbed Core Sep 17, 2021
@mbedmain mbedmain removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Mbed Core
  
Reported
Development

Successfully merging this pull request may close these issues.

None yet

6 participants