Move mbed-stubs-platform to the platform directory #14773

rwalton-arm · 2021-06-14T13:55:18Z

Summary of changes

This PR makes the mbed-stubs-platform library more self-contained by
removing the dependencies on the monolithic header libraries we created to
make implementing the stubs easier. We also move the files to the platform
directory so the stubs live with the associated component.

Now the mbed-stubs-platform library should only depend on the headers it
uses. It also now owns and exposes it's own public headers rather than
depending on every available stub header from the mbed-stubs-headers library.

This change makes the flow of dependencies easier to see and allows for a
more modular architecture moving forward.

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

ciarmcom · 2021-06-14T14:00:16Z

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

0xc0170 · 2021-06-14T15:14:41Z

platform/tests/UNITTESTS/doubles/CMakeLists.txt


+add_library(mbed-stubs-platform)


stubs or doubles (the folder is named doubles but we talk or used stubs). Was there a reason to name the folder differently?

I thought doubles was more "correct" as some of these are stubs, some are a bit more like fakes. I can revert the folder name back to stubs if we think that's better/less confusing.

doubles is fine then.

ciarmcom · 2021-06-14T15:30:15Z

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

LDong-Arm

To me this looks like a good, minimal-impact approach. The stub headers and sources are unchanged but just moved into the platform directory, with CMake definitions adjusted accordingly and decoupled.

Let's see if others from @ARMmbed/mbed-os-core agree with this approach.

rajkan01 · 2021-06-16T15:41:35Z

platform/tests/UNITTESTS/doubles/CMakeLists.txt

@@ -31,7 +31,9 @@ target_link_options(mbed-stubs-platform
        --coverage
 )
 target_link_libraries(mbed-stubs-platform
-    PRIVATE
-        mbed-headers
+    PUBLIC


This PR approach is good and minimal-impact but Initially, we have a discussion to use PRIVATE instead of stubs libraries target link libraries with PUBLIC refer #14285 (comment) & #14285 (comment), please share your thoughts

ciarmcom · 2021-06-17T15:50:16Z

This pull request has automatically been marked as stale because it has had no recent activity. @ARMmbed/mbed-os-maintainers, please start CI to get the PR merged.

rajkan01 · 2021-06-23T09:51:19Z

platform/tests/UNITTESTS/doubles/CMakeLists.txt

@@ -17,14 +26,17 @@ target_sources(mbed-stubs-platform
        randLIB_stub.c
        randLIB_stub.cpp
 )
-
+target_include_directories(mbed-stubs-platform
+    PUBLIC


Adding all the platform stub headers into mbed-stubs-platform with PUBLIC again, this will be a problem if library unittest (example ATCmdParser unittest) don't want to include stub platform headers but target link aginst mbed-stubs-platform brought those the platform stub headers. Initially, that was one of the reasons we added a mbed-stubs-headers library, it is better to add a new library mbed-stubs-platform-headers INTERFACE library with all platform stub headers refer our discussion #14285 (comment)

I'm not sure I'm seeing the issue with the example use case you've given. Why wouldn't you want the headers to be available if you specifically linked to the stub library? I could see the inverse of this being a potential problem: you want the "stub header" but want to provide another implementation yourself. However, if a stub has some "public headers" (every header in this mbed-stubs-headers lib is "public") it seems better to expose them as part of the stub library they belong to. Pulling all the headers out into a monolithic header-only library that requires non-obvious dependencies before you can use it seems like an anti-pattern. Also creating a separate "stub header" library for each stub seems unnecessary to me, unless there's a real issue we're solving by adding the extra library.

Maybe I'm missing something, so if you could show me an example of where this is actually an issue it would help me understand why we need to do this. It doesn't seem to cause a problem with the ATCmdParser test you mentioned.

In the original discussion @rajkan01 linked, @paul-szczepanek-arm said

If they are PUBLIC then if you depend on this library you also depend on the libraries it depends on.

This is like a header relying on another header to include something rather than including it itself. I think it's better if things are self contained and just exposing thing they are meant to expose and not what they are using themselves.

My interpretation of the concern is, if stub libraries A and B both have PUBLIC headers, and B depends on A, and a test T directly depends on B, then A will be "inadvertently" be exposed to T, is this the concern? But in order for compilation to succeed (because of how preprocessors work), this exposure is necessary IMO - otherwise how can T #include something from B which then #include something from A? Both A and B need to be in the include path.

Another thing is, the stub headers are for unit tests only which work within mbed-os, not intended for general use by other repositories, so flexibility isn't a concern and we should prioritise simplicity in my opinion. We are not messing with any "actual" Mbed OS libraries.

(Sorry if I completely misunderstood the original concern!)

0xc0170 · 2021-06-24T13:14:04Z

Based on the review comment above, set to as needs: work. Rob is away this week so will get back to this one the next week.

LDong-Arm · 2021-07-05T15:04:06Z

Hopefully this can get in after a rebase, as it's our initial PR to figure out how to refactor stubs of one module.

Pull request has been modified.

mergify · 2021-07-06T12:40:13Z

This PR cannot be merged due to conflicts. Please rebase to resolve them.

Move the platform stub library into the platform component directory. This change is so we can avoid duplicating the mbed-os source tree in a central UNITTESTS folder.

Move the header-only mbed-headers-platform library the unit test stubs depend on into the platform component directory. This makes the platform stubs more self contained and improves composition of the library.

Previously the platform stub library depended on `mbed-headers`, which is a collection of all available headers in mbed-os. To make it easier to separate the library, only depend on the headers we're actually using.

We have a central collection of "stub headers", which makes reasoning about dependencies rather difficult, as it forces every stub library to depend on all available stub headers. The standard approach would be for each stub library to expose its public headers, and its dependents to explicitly specify a dependency on the stub library containing the headers it needs. This is a more modular design than creating a header-only monolith library. Move the platform stub headers from this central library into the mbed-stubs-platform library to increase modularity. mbed-stubs-connectivity now depends on the mbed-stubs-platform because it requires some headers which were moved to mbed-stubs-platform.

We set the --coverage flag globally in UNITTESTS/CMakeLists.txt. We shouldn't need to also set it on mbed-stubs-platform, so remove it.

rwalton-arm · 2021-07-06T12:54:24Z

Rebased.

rwalton-arm · 2021-07-07T10:09:34Z

@0xc0170 Can we get this in today?

LDong-Arm · 2021-07-07T14:30:06Z

@ARMmbed/mbed-os-maintainers
This is our first PR to move unit test stubs, and even some later PRs that follow the same ideas are merged already. Also, "platform" stubs are used by all unit tests. So we'd better prioritise getting this in.

Patater · 2021-07-07T15:07:57Z

CI started

mbed-ci · 2021-07-07T15:14:40Z

Jenkins CI Test : ✔️ SUCCESS

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

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️

ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Jun 14, 2021

ciarmcom requested a review from a team June 14, 2021 14:00

ciarmcom added needs: review component: test component: core labels Jun 14, 2021

0xc0170 reviewed Jun 14, 2021

View reviewed changes

ciarmcom requested a review from a team June 14, 2021 15:30

LDong-Arm approved these changes Jun 15, 2021

View reviewed changes

0xc0170 previously approved these changes Jun 16, 2021

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Jun 16, 2021

rajkan01 reviewed Jun 16, 2021

View reviewed changes

This was referenced Jun 17, 2021

Move the rtos stubs to rtos/tests/UNITTESTS #14793

Closed

Move cellular stubs to Connectivity/cellular directory #14794

Closed

ciarmcom added the stale Stale Pull Request label Jun 17, 2021

ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jun 18, 2021

rajkan01 reviewed Jun 23, 2021

View reviewed changes

0xc0170 added needs: work and removed needs: CI labels Jun 24, 2021

ciarmcom removed the stale Stale Pull Request label Jun 24, 2021

This was referenced Jun 28, 2021

Move lorawan stubs to Connectivity/lorawan directory #14836

Closed

Move netsocket stubs to Connectivity/netsocket directory #14839

Closed

LDong-Arm mentioned this pull request Jul 5, 2021

Move events stubs to events dir #14823

Merged

LDong-Arm previously approved these changes Jul 5, 2021

View reviewed changes

rwalton-arm force-pushed the move_platform_stub branch from 50a6830 to 6c54ac1 Compare July 5, 2021 15:35

rwalton-arm added 6 commits July 6, 2021 13:48

CMake: unit-tests: CircularBuffer: Remove trailing whitespace

3abc788

CMake: Move platform stubs to the mbed-os/platform directory

40435fd

Move the platform stub library into the platform component directory. This change is so we can avoid duplicating the mbed-os source tree in a central UNITTESTS folder.

CMake: unit-tests: Move mbed-headers-platform to platform directory

e270474

Move the header-only mbed-headers-platform library the unit test stubs depend on into the platform component directory. This makes the platform stubs more self contained and improves composition of the library.

CMake: unit-tests: Make platform stub only depend on headers it uses

b73b38a

Previously the platform stub library depended on `mbed-headers`, which is a collection of all available headers in mbed-os. To make it easier to separate the library, only depend on the headers we're actually using.

CMake: unit-tests: Remove --coverage option from mbed-stubs-platform

55f77ea

We set the --coverage flag globally in UNITTESTS/CMakeLists.txt. We shouldn't need to also set it on mbed-stubs-platform, so remove it.

rwalton-arm force-pushed the move_platform_stub branch from 6c54ac1 to 55f77ea Compare July 6, 2021 12:53

LDong-Arm approved these changes Jul 6, 2021

View reviewed changes

mergify bot added needs: CI and removed needs: work labels Jul 6, 2021

mergify bot added ready for merge and removed needs: CI labels Jul 7, 2021

Patater merged commit 8287c52 into ARMmbed:master Jul 7, 2021

mergify bot removed the ready for merge label Jul 7, 2021

mbedmain added release-version: 6.13.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jul 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move mbed-stubs-platform to the platform directory #14773

Move mbed-stubs-platform to the platform directory #14773

rwalton-arm commented Jun 14, 2021 •

edited by rajkan01

Loading

ciarmcom commented Jun 14, 2021

0xc0170 Jun 14, 2021

rwalton-arm Jun 16, 2021

0xc0170 Jun 16, 2021

ciarmcom commented Jun 14, 2021

LDong-Arm left a comment

rajkan01 Jun 16, 2021 •

edited

Loading

ciarmcom commented Jun 17, 2021

rajkan01 Jun 23, 2021 •

edited

Loading

rwalton-arm Jun 30, 2021 •

edited

Loading

LDong-Arm Jun 30, 2021 •

edited

Loading

0xc0170 commented Jun 24, 2021

LDong-Arm commented Jul 5, 2021

mergify bot commented Jul 6, 2021

rwalton-arm commented Jul 6, 2021

rwalton-arm commented Jul 7, 2021

LDong-Arm commented Jul 7, 2021

Patater commented Jul 7, 2021

mbed-ci commented Jul 7, 2021


		add_library(mbed-stubs-platform)

Move mbed-stubs-platform to the platform directory #14773

Move mbed-stubs-platform to the platform directory #14773

Conversation

rwalton-arm commented Jun 14, 2021 • edited by rajkan01 Loading

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

ciarmcom commented Jun 14, 2021

0xc0170 Jun 14, 2021

Choose a reason for hiding this comment

rwalton-arm Jun 16, 2021

Choose a reason for hiding this comment

0xc0170 Jun 16, 2021

Choose a reason for hiding this comment

ciarmcom commented Jun 14, 2021

LDong-Arm left a comment

Choose a reason for hiding this comment

rajkan01 Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

ciarmcom commented Jun 17, 2021

rajkan01 Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

rwalton-arm Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

LDong-Arm Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

0xc0170 commented Jun 24, 2021

LDong-Arm commented Jul 5, 2021

mergify bot commented Jul 6, 2021

rwalton-arm commented Jul 6, 2021

rwalton-arm commented Jul 7, 2021

LDong-Arm commented Jul 7, 2021

Patater commented Jul 7, 2021

mbed-ci commented Jul 7, 2021

Jenkins CI Test : ✔️ SUCCESS

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

rwalton-arm commented Jun 14, 2021 •

edited by rajkan01

Loading

rajkan01 Jun 16, 2021 •

edited

Loading

rajkan01 Jun 23, 2021 •

edited

Loading

rwalton-arm Jun 30, 2021 •

edited

Loading

LDong-Arm Jun 30, 2021 •

edited

Loading