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

Fix lorawantimer unit test #14948

Merged
merged 8 commits into from
Jul 27, 2021

Conversation

Patater
Copy link
Contributor

@Patater Patater commented Jul 22, 2021

Summary of changes

Lorawan timer unit test currently fails because it failed to initialize the mock event queue.

In debugging and fixing this issue, many other broken windows were spotted and fixed, including a broken window whereby any failing MBED_ASSERT in a unit test would not bubble up to googletest which was masking that the lorawan timer unit test was at all broken. See commit messages for details on this and other fixes.

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)
[] Tests / results supplied as part of this PR

Reviewers


@mergify
Copy link

mergify bot commented Jul 22, 2021

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

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

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

Remove trailing whitespace from lorawantimer's CMakeLists.txt.
The header equeue_stub.h was missing header guards to enforce single
inclusion. Add some header guards.
Use the `--coverage` option instead of manually linking to gcov, as some
host platforms (like macOS, FreeBSD) don't have gcov by default and use
an llvm equivalent instead.
@Patater Patater force-pushed the lorawan-timer-unittest-fake-fix branch from dd26862 to 8994ba6 Compare July 22, 2021 14:43
@Patater
Copy link
Contributor Author

Patater commented Jul 22, 2021

Rebased on latest master

@ciarmcom
Copy link
Member

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

Add extern "C" to the equeue_stub declaration to avoid an error when a C
file implements the equeue_stub symbol (as we do in equeue_stub.c).

    Undefined symbols for architecture x86_64:
      "_equeue_stub", referenced from:
          Test_LoRaWANTimer_start_Test::TestBody() in Test_LoRaWANTimer.cpp.o
    ld: symbol(s) not found for architecture x86_64
    clang: error: linker command failed with exit code 1 (use -v to see invocation)
@Patater Patater force-pushed the lorawan-timer-unittest-fake-fix branch from 8994ba6 to 56af3f0 Compare July 22, 2021 17:04
rwalton-arm and others added 4 commits July 27, 2021 10:28
We weren't initialising the mocks correctly, so an MBED_ASSERT failed in
`LoRaWANTimer::start` because the "timer_id" was not set.
We previously made up an invalid message type, which was roughly
`MCPS_MULTICAST | 0x40`. This causes an MBED_ASSERT failure in the
handle_rx function, as it uses an assert to validate the message type
passed to it (in convert_to_msg_flag()). Use the valid message type
MCPS_MULTICAST instead in the handle_rx unit test.
The CircularBuffer doesn't allow pushing zero elements; you must push at
least one. Update the CircularBuffer unit test to avoid invalid use of
the CircularBuffer.
Make MBED_ASSERT failures fail the unit test case. Without this, we
might not notice the assertion failure as it wouldn't be bubbled up to
googletest.
@Patater Patater force-pushed the lorawan-timer-unittest-fake-fix branch from 56af3f0 to 51b81e0 Compare July 27, 2021 09:36
@Patater Patater added this to In progress (8) in Mbed Core via automation Jul 27, 2021
@Patater Patater moved this from In progress (8) to Awaiting review (3) in Mbed Core Jul 27, 2021
@Patater
Copy link
Contributor Author

Patater commented Jul 27, 2021

Rebased to remove my move from stub event queue to fake event queue, and to cherry-pick in mock initialization from Rob's PR (#14944).

@Patater
Copy link
Contributor Author

Patater commented Jul 27, 2021

CI started

@mbed-ci
Copy link

mbed-ci commented Jul 27, 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 ✔️

Mbed Core automation moved this from Awaiting review (3) to Reviewer approved & awaiting CI & merge (3) Jul 27, 2021
@Patater Patater merged commit 862a942 into ARMmbed:master Jul 27, 2021
Mbed Core automation moved this from Reviewer approved & awaiting CI & merge (3) to Done Jul 27, 2021
@mergify mergify bot removed the ready for merge label Jul 27, 2021
@Patater Patater moved this from Done to Reported in Mbed Core Jul 29, 2021
@mbedmain mbedmain added release-version: 6.14.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Aug 18, 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