unit_test: Restore 'waf test' functionality by Colin-Suckow · Pull Request #69 · pebble-dev/pebble-firmware

Colin-Suckow · 2025-03-05T02:51:48Z

Restores the waf test command, hopefully resolving #46.

Any tests failing to compile were commented out and appended with 'UNIT_TEST_DISABLED' for easy location in the future. Additionally, the entire fw/comm test suite was disabled because of missing bluetooth library headers (GAPAPI.h, GATTAPI.h).

I also added CI steps that run the test suite and parses the output junit.xml file. The test and parse steps are setup to not fail the entire build.

gmarull · 2025-03-05T09:36:25Z

.github/workflows/build.yml

+      - name: Run tests
+        run: ./waf test
+        continue-on-error: true


this will run on each matrix combination, feels like unnecessary. Maybe a new CI workflow for unit tests should be added?

Agreed. I have split the test runner out into a separate workflow. I also took the opportunity to add a step for uploading the failed visual diff result pngs.

gmarull · 2025-03-05T09:37:13Z

tests/fw/graphics/wscript

+    # clar(ctx,
+    #     sources_ant_glob = \
+    #         " src/fw/applib/graphics/gtypes.c" \
+    #         " src/fw/applib/graphics/graphics_circle.c",
+    #     test_sources_ant_glob = "test_graphics_circle.c")


isn't there a better way to disable a test that doesn't involve commenting out code? like a quarantine list

I refactored the test skipping into an exclusion list. Good call on this one. The new solution feels a lot cleaner.

Hexxeh · 2025-03-05T09:42:12Z

tests/fw/comm/wscript

 def build(bld):
+    # These tests don't compile because the bluetooth library needed was removed. The 'COMM_TESTS_DISABLED'
+    # flag has been added to temporarily disabled them.
+    if 'COMM_TESTS_DISABLED' in bld.env.DEFINES:


This feels like the wrong place to put this flag, since you're not. actually using it in C code. It should be just part of the waf env itself I think?

Agreed. This specific flag has been replaced by the test exclusion list, but I did add a new flag for skipping the python tool tests. That flag has been added directly to env.

Hexxeh

Can we create tickets for the things we had to disable to remember to go back and fix them?

Colin-Suckow · 2025-03-06T03:23:29Z

Can we create tickets for the things we had to disable to remember to go back and fix them?

Sounds like a good idea to me. How do you want to handle this? Should I go ahead make the issues now or after this closes?

Hexxeh · 2025-03-06T09:42:41Z

Can we create tickets for the things we had to disable to remember to go back and fix them?

Sounds like a good idea to me. How do you want to handle this? Should I go ahead make the issues now or after this closes?

Given they're broken already I think you could go ahead and create them now. Or we could just list them on the original ticket for this.

Colin-Suckow · 2025-03-07T01:21:55Z

Created #77 to track the disabled tests

Hexxeh · 2025-03-07T16:26:37Z

.github/workflows/test.yml

+  pull_request:
+    branches: [main]
+
+env:


Is there a way to re-use these between files (same with the first few steps below)?

I guess it's time to setup a CI Docker image :-)

How do we feel about making a new issue for that docker image? Outside of my wheel house and seems significant enough to deserve its own review.

Good with me :)

yep, can happen later

Created #82 for it

Hexxeh · 2025-03-07T16:27:03Z

tests/wscript

    # time_t is defined in sys/types in newlib, and time.h on recent Linux
    # so just force the defined type for testing time
-    bld.env.CFLAGS.append('-Dtime_t=__SYSCALL_SLONG_TYPE')
+    #bld.env.CFLAGS.append('-Dtime_t=__SYSCALL_SLONG_TYPE')


Can this be removed rather than just commenting, if it isn't needed anymore?

gmarull

could you squash changes or reduce diffs somehow? looks like you're undoing initial changes in later commits

Colin-Suckow · 2025-03-12T01:58:59Z

could you squash changes or reduce diffs somehow? looks like you're undoing initial changes in later commits

Done (and done a second time because I found a test I forgot to uncomment and add to BROKEN_TESTS :) )

Hexxeh · 2025-03-12T11:05:14Z

tools/pdc2png/wscript

    # time_t is defined in sys/types in newlib, and time.h on recent Linux
    # so just force the defined type for testing time
-    pdc2png_env.CFLAGS.append('-Dtime_t=__SYSCALL_SLONG_TYPE')
+    #pdc2png_env.CFLAGS.append('-Dtime_t=__SYSCALL_SLONG_TYPE')


Can this just be removed (I think this was commented before, was the change maybe just lost)?

@Colin-Suckow ping

Removed. This is a separate define from the last one. Forgot about it during the last fix.

Signed-off-by: Colin Suckow <colin@suckow.dev>

gmarull reviewed Mar 5, 2025

View reviewed changes

gmarull requested a review from Hexxeh March 5, 2025 09:37

Hexxeh reviewed Mar 5, 2025

View reviewed changes

gmarull requested a review from Hexxeh March 7, 2025 16:24

Hexxeh reviewed Mar 7, 2025

View reviewed changes

Colin-Suckow force-pushed the fix_tests branch from af898b4 to 577ecc1 Compare March 9, 2025 23:12

gmarull reviewed Mar 11, 2025

View reviewed changes

Colin-Suckow force-pushed the fix_tests branch 2 times, most recently from 5b83a1f to 40306fa Compare March 12, 2025 01:57

gmarull requested a review from Hexxeh March 12, 2025 11:02

Hexxeh reviewed Mar 12, 2025

View reviewed changes

Hexxeh approved these changes Mar 12, 2025

View reviewed changes

unit_test: Restore 'waf test' functionality

919baf0

Signed-off-by: Colin Suckow <colin@suckow.dev>

Colin-Suckow force-pushed the fix_tests branch from 40306fa to 919baf0 Compare March 15, 2025 15:31

gmarull merged commit 44e7092 into pebble-dev:main Mar 16, 2025
6 checks passed

waldyrious mentioned this pull request Mar 18, 2025

Fix unit tests that fail to compile #77

Open

26 tasks

Colin-Suckow mentioned this pull request Mar 19, 2025

Fix unit tests build #46

Closed

Conversation

Colin-Suckow commented Mar 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hexxeh left a comment

Choose a reason for hiding this comment

Uh oh!

Colin-Suckow commented Mar 6, 2025

Uh oh!

Hexxeh commented Mar 6, 2025

Uh oh!

Colin-Suckow commented Mar 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmarull left a comment

Choose a reason for hiding this comment

Uh oh!

Colin-Suckow commented Mar 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants