Add CTest support for greentea tests: events #14986

hazzlim · 2021-08-06T16:52:20Z

Summary of changes

Fixes #14965

the MBED_EXTENDED_TESTS macro is removed and 'extended' tests are enabled by default
Multi-part FAT filesystem tests are refactored, splitting the test case with large ROM requirements into a separate suite in order to isolate it for skipping.
events/equeue tests that require RTOS are wrapped in checks for RTOS
events greentea tests are refactored to use std::chrono values and remove deprecated APIs
CMake support for events greentea tests is refactored, following pilot work, to build as part of global project when BUILD_GREENTEA_TESTS is set. Added checks for macros that indicate the test is not supported and conditionally set the TEST_SKIPPED argument accordingly.

Preceding:

Impact of changes

None

Migration actions required

None

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

Reviewers

@ARMmbed/mbed-os-core

ciarmcom · 2021-08-06T17:00:16Z

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

ciarmcom · 2021-08-11T19:50:13Z

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

events/tests/TESTS/events/equeue/CMakeLists.txt

LDong-Arm

CI failed because test_equeue_multithread and test_equeue_multithreaded_barrage use Thread which needs RTOS and doesn't work with bare metal. You can guard them with #ifdef MBED_CONF_RTOS_PRESENT.

Note: Thread requires RTOS, but ThisThread works fine with bare metal because bare metal is treated like only one thread (i.e. ThisThread).

It's nice that our CMake test includes a bare metal target.

events/tests/TESTS/events/equeue/CMakeLists.txt

hazzlim · 2021-08-13T17:08:22Z

CI failed because test_equeue_multithread and test_equeue_multithreaded_barrage use Thread which needs RTOS and doesn't work with bare metal. You can guard them with #ifdef MBED_CONF_RTOS_PRESENT.

Note: Thread requires RTOS, but ThisThread works fine with bare metal because bare metal is treated like only one thread (i.e. ThisThread).

It's nice that our CMake test includes a bare metal target.

Thanks Lingkai, I am guarding these tests and also refactoring all the events tests to use chrono to avoid deprecation warning.

Already shows that running these extended tests is a good thing! And a good reminder to me to test both profiles locally too.

hugueskamba · 2021-08-16T07:28:28Z

You can use bullet points in the PR description to list the PR that must be merged before this one. It shows icons that informs of the status of the PRs as well as their titles.

Example:

Patater · 2021-08-18T09:03:23Z

events/tests/TESTS/events/timing/main.cpp

@@ -128,7 +152,8 @@ void semaphore_timing_test()
 // Test setup
 utest::v1::status_t test_setup(const size_t number_of_cases)
 {
-    GREENTEA_SETUP((number_of_cases + 1)*TEST_EVENTS_TIMING_TIME / 1000, "default_auto");
+    GREENTEA_SETUP((number_of_cases + 1) * duration_cast<seconds>(TEST_EVENTS_TIMING_TIME_DURATION).count(),


Looks like we had some bad style before. Is events/tests/TESTS/events/timing somehow exclused from astyle checking?

It’s not excluded...but I just tried running astyle on it (the master branch version) and it returned “Unchanged”. Maybe .astylerc needs updating?

Patater · 2021-08-18T09:09:37Z

CI started

ciarmcom · 2021-09-01T15:50:25Z

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

hazzlim · 2021-09-03T15:33:42Z

ROM size seems semantically the clearest and the most direct property to query. If some targets don't define ROM size (either in config or directly in their linker scripts) we could use this opportunity to define it for all targets?

I've identified the targets that don't define ROM size in config - I take it I can just consult the TRM and add all the mbed_* memory size definitions?

Some targets seem slightly less straightforward, for example ARM_MPS2_* targets have no flash/rom, but the linker scripts define ZBTSRAM1 as flash, so do we think it would make sense to define this region as mbed_rom ?

rwalton-arm · 2021-09-22T15:28:08Z

storage/filesystem/fat/tests/TESTS/fat/multipart_fat_filesystem/main.cpp

@@ -18,6 +18,7 @@
 #include "greentea-client/test_env.h"
 #include "unity.h"
 #include "utest.h"
+#include <memory>


In commit message "Greentea: Refactor FAT Filesystem test cases to be independent" there is a typo: "propogated" should be "propagated"

rwalton-arm · 2021-09-22T15:32:17Z

storage/filesystem/fat/tests/TESTS/fat/multipart_fat_filesystem/main.cpp

+bool check_heap_allocation(size_t bytes)
+{
+    unique_ptr<uint8_t[]> dummy {new (std::nothrow) uint8_t[bytes]};
+    return (dummy != nullptr);


We should just be able to return dummy.get() here and a nullptr will be deduced as false

Good point! I have amended with this change.

rwalton-arm · 2021-09-22T15:32:58Z

storage/filesystem/fat/tests/TESTS/fat/multipart_fat_filesystem/main.cpp

+void format_partitions(HeapBlockDevice &bd)
+{
+    // Create two partitions splitting device in ~half
+    TEST_ASSERT_EQUAL(0, MBRBlockDevice::partition(&bd, 1, 0x83, 0, (BLOCK_COUNT / 2) * BLOCK_SIZE));


0x83 should be a named variable.

rwalton-arm · 2021-09-22T15:35:35Z

storage/filesystem/fat/tests/TESTS/fat/multipart_fat_filesystem/main.cpp

-    TEST_SKIP_UNLESS_MESSAGE(bd, "Not enough heap memory to run test. Test skipped.");
+    TEST_SKIP_UNLESS_MESSAGE(check_heap_allocation(mem_alloc_threshold), "Not enough heap memory to run test. Test skipped.");
+
+    HeapBlockDevice bd {BLOCK_COUNT * BLOCK_SIZE, BLOCK_SIZE};


This test case seems to make lots of assertions. Any way we can split it up into separate cases?

I agree it's a pretty huge number of assertions. I'm not really sure how to split it up however, as verifying the data read matches the data written requires all the steps in the case as far as I can see.

storage/filesystem/fat/tests/TESTS/fat/multipart_fat_filesystem/main.cpp

rwalton-arm · 2021-09-22T15:41:35Z

storage/filesystem/fat/tests/TESTS/fat/multipart_fat_filesystem_with_other_filesystem/main.cpp

+{
+    TEST_SKIP_UNLESS_MESSAGE(check_heap_allocation(mem_alloc_threshold), "Not enough heap memory to run test. Test skipped.");
+
+    // Stage 0 - LittleFS


Could these "Stages" be separate test cases? The loop looks pretty awkward at the moment with all this "if stage==1" stuff.

Yes it's not very readable! I have split the stages into three separate cases in the commit "Greentea: Refactor test with other filesystem".

rwalton-arm · 2021-09-22T15:53:50Z

events/tests/TESTS/events/timing/main.cpp

@@ -57,6 +62,16 @@ float chisq(float sigma)
    return pow(gauss(0, sqrt(sigma)), 2);
 }

+// Macro for random duration generation
+#define RANDOM_DURATION(sigma) \


Could this be a normal function instead of a macro?

Yes good point, I have replaced the macro with a function.

rwalton-arm · 2021-09-22T15:56:14Z

events/tests/TESTS/events/timing/main.cpp

-    unsigned start = equeue_tick();
-    int prev = 0;
+    // equeue_tick() returns an integral millisecond value
+    auto start = milliseconds{equeue_tick()};


Looks like start should be const?

rwalton-arm · 2021-09-22T15:56:44Z

events/tests/TESTS/events/timing/main.cpp

-    while (prev < TEST_EVENTS_TIMING_TIME) {
-        int next = equeue_tick() - start;
+    while (prev < TEST_EVENTS_TIMING_TIME_DURATION) {
+        auto next = milliseconds{equeue_tick()} - start;


Looks like next should be const?

rwalton-arm · 2021-09-22T15:58:39Z

events/tests/TESTS/events/timing/main.cpp

+        auto delay = RANDOM_DURATION(TEST_EVENTS_TIMING_MEAN_DURATION);

-        int start = timer.read_us();
-        equeue_sema_wait(&sema, delay);
-        int taken = timer.read_us() - start;
+        auto start = timer.elapsed_time();
+        equeue_sema_wait(&sema, duration_cast<milliseconds>(delay).count());
+        auto taken = timer.elapsed_time() - start;


Looks like most, if not all, of these variables should be const?

In the Greentea test suite `multipart_fat_filesystem`, all the test cases are dependent on setup that occurs in the first case. This case formats (with FAT) a HeapBlockDevice that is shared between cases. Alongside this, the first case checks that sufficient heap memory is available for the dynamic memory allocation required for the FAT format process. This check is performed by testing if we can successfully allocate the required number of bytes, and skipping the case if not. The check is then propagated to the other cases by skipping those if the HeapBlockDevice pointer is falsy, which it will be if the first test is skipped. We refactor the cases to be independent of one another. Now each test case checks that sufficient memory can be allocated for FAT formatting, performing said formatting or skipping as indicated by this check. In doing so we replace remove some raw pointers, and replace some C style arrays with std::array, in order to make memory management less error prone.

Pull request has been modified.

The test case `test_with_other_fs` uses a fairly awkward loop to test mounting different filesystems consecutively. We split the test case into seperate cases, using more descriptive names and removing the use of raw pointers.

The Greentea test suite multipart_fat_filesystem contains a test case that uses LittleFileSystem and FATFilesystem, and including this case results in a binary too large for the flash memory of some targets. We separate this test case into its own suite and guard this test suite with a check for the size of the target's ROM. This is chosen over simply guarding the individual test case inside the same suite as it will allow the newly created suite to be skipped from CMake when the FAT Filesystem tests are ported to CTest. This issue was previously unidentified because the test suite was disabled by default, requiring the `MBED_EXTENDED_TESTS` macro to be defined, and was therefore not run in CI.

Some events/equeue Greentea test cases use the Thread API and therefore require RTOS to run, but previously this was not checked in the code. We add guards with the macro MBED_CONF_RTOS_PRESENT.

MBED_EXTENDED_TESTS macro was previously required to enable certain Greentea tests, which were disabled by default in order to save CI time. We remove this macro because the time saved in finding bugs or issues with a PR prior to merging is worth the increased testing time.

References to time should do so using std::chrono. We reworked events tests to use std::chrono and new APIs in order to remove deprecation warnings resulting from deprecated API calls. This required addition of a macro for test assertions using std::chrono values. Calls to wait_us() (not deprecated) are replaced with ThisThread::sleep_for() as there is no reason to wait without sleeping. Certain API calls that involve UserAllocatedEvents and do not have std::chrono versions are left unchanged, for example EventQueue.time_left().

Following the approach defined in ARMmbed#14892 and ARMmbed#14902, events greentea tests are ported to enable running with CTest. events/queue and events/timing both require DEVICE_USTICKER.

0xc0170 · 2022-02-09T14:52:54Z

We are closing this pull request due to inactivity

ciarmcom added the release-type: patch label Aug 6, 2021

ciarmcom requested a review from a team August 6, 2021 17:00

ciarmcom added needs: review component: hal component: security component: tools component: core devices: nuvoton component: connectivity labels Aug 6, 2021

hazzlim force-pushed the ctest_support branch 3 times, most recently from e147d50 to eb8658b Compare August 11, 2021 13:46

ciarmcom added the stale label Aug 11, 2021

hazzlim force-pushed the ctest_support branch from eb8658b to bff47d4 Compare August 12, 2021 08:23

hazzlim marked this pull request as ready for review August 12, 2021 08:30

LDong-Arm reviewed Aug 12, 2021

View reviewed changes

events/tests/TESTS/events/equeue/CMakeLists.txt Outdated Show resolved Hide resolved

ciarmcom removed the stale label Aug 12, 2021

hazzlim force-pushed the ctest_support branch from bff47d4 to 3dbbbd1 Compare August 13, 2021 14:14

LDong-Arm reviewed Aug 13, 2021

View reviewed changes

events/tests/TESTS/events/equeue/CMakeLists.txt Outdated Show resolved Hide resolved

hazzlim force-pushed the ctest_support branch 3 times, most recently from 3d017fd to 4daeb86 Compare August 13, 2021 22:35

Patater reviewed Aug 18, 2021

View reviewed changes

LDong-Arm mentioned this pull request Aug 18, 2021

Add CMakeLists for USB greentea tests #14937

Closed

mergify bot added the needs: work label Aug 18, 2021

ciarmcom added the stale label Sep 1, 2021

ciarmcom added stale and removed stale labels Sep 3, 2021

ciarmcom added stale and removed stale labels Sep 12, 2021

hazzlim force-pushed the ctest_support branch from c370579 to cf4a565 Compare September 18, 2021 00:11

rwalton-arm previously requested changes Sep 22, 2021

View reviewed changes

hazzlim force-pushed the ctest_support branch from cf4a565 to bdc6d9f Compare September 27, 2021 07:38

hazzlim added 6 commits October 2, 2021 11:34

Greentea: Refactor test with other filesystem

679a8a2

The test case `test_with_other_fs` uses a fairly awkward loop to test mounting different filesystems consecutively. We split the test case into seperate cases, using more descriptive names and removing the use of raw pointers.

Greentea: Add macro check for RTOS in events/equeue

ace887f

Some events/equeue Greentea test cases use the Thread API and therefore require RTOS to run, but previously this was not checked in the code. We add guards with the macro MBED_CONF_RTOS_PRESENT.

CMake: Greentea: Port events tests to CTest

a3c30f7

Following the approach defined in ARMmbed#14892 and ARMmbed#14902, events greentea tests are ported to enable running with CTest. events/queue and events/timing both require DEVICE_USTICKER.

hazzlim force-pushed the ctest_support branch from bdc6d9f to a3c30f7 Compare October 2, 2021 10:48

0xc0170 closed this Feb 9, 2022

mergify bot removed needs: work devices: nuvoton component: connectivity component: hal component: tools component: security component: core release-type: patch labels Feb 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CTest support for greentea tests: events #14986

Add CTest support for greentea tests: events #14986

hazzlim commented Aug 6, 2021 •

edited

Loading

ciarmcom commented Aug 6, 2021

ciarmcom commented Aug 11, 2021

LDong-Arm left a comment •

edited

Loading

hazzlim commented Aug 13, 2021

hugueskamba commented Aug 16, 2021

Patater Aug 18, 2021

hazzlim Aug 18, 2021 •

edited

Loading

Patater commented Aug 18, 2021

ciarmcom commented Sep 1, 2021

hazzlim commented Sep 3, 2021

rwalton-arm Sep 22, 2021

hazzlim Sep 27, 2021

rwalton-arm Sep 22, 2021

hazzlim Sep 27, 2021

rwalton-arm Sep 22, 2021

hazzlim Sep 27, 2021

rwalton-arm Sep 22, 2021

hazzlim Oct 2, 2021

rwalton-arm Sep 22, 2021

hazzlim Oct 2, 2021

rwalton-arm Sep 22, 2021

hazzlim Sep 27, 2021

rwalton-arm Sep 22, 2021

hazzlim Sep 27, 2021

rwalton-arm Sep 22, 2021

hazzlim Sep 27, 2021

rwalton-arm Sep 22, 2021

hazzlim Sep 27, 2021

0xc0170 commented Feb 9, 2022

Add CTest support for greentea tests: events #14986

Add CTest support for greentea tests: events #14986

Conversation

hazzlim commented Aug 6, 2021 • edited Loading

Summary of changes

Impact of changes

Migration actions required

Documentation

None

Pull request type

Test results

Reviewers

@ARMmbed/mbed-os-core

ciarmcom commented Aug 6, 2021

ciarmcom commented Aug 11, 2021

LDong-Arm left a comment • edited Loading

Choose a reason for hiding this comment

hazzlim commented Aug 13, 2021

hugueskamba commented Aug 16, 2021

Choose a reason for hiding this comment

hazzlim Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Patater commented Aug 18, 2021

ciarmcom commented Sep 1, 2021

hazzlim commented Sep 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xc0170 commented Feb 9, 2022

hazzlim commented Aug 6, 2021 •

edited

Loading

LDong-Arm left a comment •

edited

Loading

hazzlim Aug 18, 2021 •

edited

Loading