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

✅ Add unit testing framework #26948

Conversation

sjasonsmith
Copy link
Contributor

@sjasonsmith sjasonsmith commented Apr 8, 2024

Description

This revives and builds upon an effort from @costas-basdekis several years ago, which provided a way to execute unit tests against various Marlin configurations.

This uses Platform IO's unit testing mechanisms to build and execute tests on a Linux machine.

How to use this feature

This is used through the top-level makefile. It likely only works on Linux or a Linux VM (including WSL).
Execute by running make tests-code-all-local or make tests-code-all-local-docker.

Overall summary of strategy:

  1. Unique config.ini files describing test configurations are placed in sub-folders beneath test. PlatformIO recognizes this folder structure and will build one test executable for each folder present.

  2. extra_script tooling adds extra commands which will apply the config.ini prior to building each test.

  3. Tests are automatically discovered from test files inside the Marlin/tests directory. The Marlin configuration is available inside these tests, so typical pre-processor mechanisms can be used to determine whether a configuration is suitable to run a test, or to alter the behavior of a test.

  4. Will run automatically with our existing CI tests. This was tested in my developer branch, but will have to be verified as soon as this is merged.

How were the initial tests selected?

Some of these tests were simple sanity checks originally added by Costas when he initially prototyped the feature. The types tests were added to specifically test a recently fixed Boolean casting problem which caused major issues across several Marlin subsystems.

Possible future improvements

  • Currently it isn't easy to run a single configuration or apply a test filter. This is supported by platformio, but additional work will be needed to document it and make it work through makefiles.
  • I'd like to consider switching to googletest, which is a much more robust testing framework if we will only be using this on Linux. I'm currently prevented from doing this due to name conflicts between TEST macros.

Requirements

Linux development environment. This can be executed inside Windows Subsystem for Linux (I've only tested with docker). It may be possible to get it working on other platforms, but this has not been attempted.

Benefits

Provides a mechanism to write tests which can be easily executed and automated by any developer in the world. These can verify correct behavior, as well as protect against future regression.

Configurations

N/A

Related Issues

N/A

costas-basdekis and others added 5 commits April 6, 2024 09:44
By using exec_test_failing, we can test that if we omit some
configuration, or if we mismatch it, it will be caught.

You can even test that certain strings appear in the output, and if not,
the tests fail. To do that pass them as extra arguments.
Add some first unit tests

Fix `~Timer` if it hadn't initialised

Amend `restore_configs` to reset all pins files

It would only do it for ramps/RAMPS, but that's an implicit assumption
that it's the only board we will ever change.

Add test documentation

Use macros to auto-collect code tests

Use platformio custom actions to run unit tests

Avoid a warning
This was rebased and may not be completely functional without the commits that follow.
@sjasonsmith sjasonsmith added Needs: Work More work is needed Needs: Discussion Discussion is needed C: Build / Toolchain labels Apr 8, 2024
@sjasonsmith
Copy link
Contributor Author

That appears to have been relatively trivial to get running in a GitHub workflow. I have several temporary commits (which will be reverted) to allow them to run in my personal repo/branch for testing purposes.

Output with all tests passing

Output with the previous types.h fix reverted, causing tests to fail

Error messages that produced:

 linux_native_test:test_default_config_with_paren_comments [FAILED] Took 34.76 seconds 

=================================== SUMMARY ===================================
Environment        Test                                     Status    Duration
-----------------  ---------------------------------------  --------  ------------
linux_native_test  test_default_config_with_paren_comments  FAILED    00:00:34.758

__________ linux_native_test:test_default_config_with_paren_comments __________
test/marlin_tests.cpp:19:XYval_non_const_as_bools:FAIL: Expected FALSE Was TRUE

test/marlin_tests.cpp:37:XYZval_non_const_as_bools:FAIL: Expected FALSE Was TRUE

test/marlin_tests.cpp:55:XYZEval_non_const_as_bools:FAIL: Expected FALSE Was TRUE

@sjasonsmith sjasonsmith force-pushed the InWork/costas/2024_better-tests-rebased-squashed2 branch from b40d3a6 to ea5076d Compare April 9, 2024 02:18
name: Run All Unit Tests
# These tests will only be able to run on the bugfix-2.1.x branch, until the next release
# pulls them into additional branches.
if: github.repository == 'MarlinFirmware/Marlin' && github.ref == 'refs/heads/bugfix-2.1.x'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I tested that the workflow worked on my own repo/branch, I didn't test this github.ref check. I'm attempting to prevent this from running on the 2.1.x branch, since it won't work yet.

If we don't want to gamble whether this will work once merged, we could split this into a new YML file with different branch requirements. I thought it made sense to add it to this file because then it can re-use all the trigger configuration regarding file paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That github.ref syntax came from GitHub Copilot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also move this to its own action, if that helps to make things cleaner.

@sjasonsmith sjasonsmith marked this pull request as ready for review April 9, 2024 02:26
Copy link
Member

@thinkyhead thinkyhead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally speaking it looks good. I just have one cleanup commit to push. Let's try replacing all the test_*.cpp files with INI files (based on config.ini). I've been planning to do the same thing for our CI tests, which should make them easier to maintain.

name: Run All Unit Tests
# These tests will only be able to run on the bugfix-2.1.x branch, until the next release
# pulls them into additional branches.
if: github.repository == 'MarlinFirmware/Marlin' && github.ref == 'refs/heads/bugfix-2.1.x'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also move this to its own action, if that helps to make things cleaner.

Marlin/src/gcode/queue.h Outdated Show resolved Hide resolved
test/test_3_extruders_with_runout_sensors/test_all.cpp Outdated Show resolved Hide resolved
@sjasonsmith sjasonsmith force-pushed the InWork/costas/2024_better-tests-rebased-squashed2 branch from d37450d to 5514777 Compare April 9, 2024 06:11
@sjasonsmith
Copy link
Contributor Author

@thinkyhead I believe it will be automatic since I retained his name on commits in the history, but I'd like to be sure that @costas-basdekis is attributed as one of the authors when it goes in.

@sjasonsmith sjasonsmith dismissed thinkyhead’s stale review April 10, 2024 04:50

The requested changes have been completed.

@sjasonsmith sjasonsmith removed Needs: Work More work is needed Needs: Discussion Discussion is needed labels Apr 10, 2024
@sjasonsmith sjasonsmith changed the title Add unit testing framework ✅ Add unit testing framework Apr 10, 2024
@thinkyhead thinkyhead removed their request for review April 10, 2024 21:09
@thinkyhead
Copy link
Member

This seems to be in good shape as a starting point for a more complete set of tests. After this is merged we can try asking GitHub Copilot to generate more complete tests for our core data type classes and other things.

@sjasonsmith
Copy link
Contributor Author

This seems to be in good shape as a starting point for a more complete set of tests. After this is merged we can try asking GitHub Copilot to generate more complete tests for our core data type classes and other things.

Core data types and verifying complex macro behavior are good places to start.

I’d like to see us then expand to some of the more complex logic. I believe I have tests in some experiments branches for bilinear subdivision and classic jerk calculations that I may be able to revive.

@sjasonsmith
Copy link
Contributor Author

Based on the last comment from @thinkyhead, I m going to go ahead and merge this so that we can start adding additional tests.

@sjasonsmith sjasonsmith merged commit d10861e into MarlinFirmware:bugfix-2.1.x Apr 13, 2024
62 checks passed
blu28 added a commit to blu28/Marlin-blu that referenced this pull request Apr 20, 2024
commit 02ba6f9
Author: thinkyhead <thinkyhead@users.noreply.github.com>
Date:   Fri Apr 19 00:21:25 2024 +0000

    [cron] Bump distribution date (2024-04-19)

commit dba0010
Author: Andrew <18502096+classicrocker883@users.noreply.github.com>
Date:   Thu Apr 18 19:04:03 2024 -0400

    🎨 Rename some G-code files (MarlinFirmware#26981)

commit 90667f6
Author: I3DBeeTech <129617321+I3DBeeTech@users.noreply.github.com>
Date:   Fri Apr 19 02:24:17 2024 +0530

    🐛 Fix BLACKBEEZMINI fan, info (MarlinFirmware#26983)

commit d6961b2
Author: thinkyhead <thinkyhead@users.noreply.github.com>
Date:   Wed Apr 17 06:06:51 2024 +0000

    [cron] Bump distribution date (2024-04-17)

commit 07ebb81
Author: Javlon Sodikov <5047093+javlonsodikov@users.noreply.github.com>
Date:   Wed Apr 17 10:25:22 2024 +0500

    🩹Fix ProUI error when !CASELIGHT_USES_BRIGHTNESS (MarlinFirmware#26976)

    * Fix the compile error with the case light menu

    Fix the compile error with the case light menu

    * Add failing test

    ---------

    Co-authored-by: Jason Smith <jason.inet@gmail.com>

commit 245db73
Author: thinkyhead <thinkyhead@users.noreply.github.com>
Date:   Tue Apr 16 18:06:16 2024 +0000

    [cron] Bump distribution date (2024-04-16)

commit 9342dae
Author: Scott Lahteine <thinkyhead@users.noreply.github.com>
Date:   Tue Apr 16 12:17:47 2024 -0500

    📝 Remove dead PDF links

commit 1f84f50
Author: thinkyhead <thinkyhead@users.noreply.github.com>
Date:   Mon Apr 15 02:38:10 2024 +0000

    [cron] Bump distribution date (2024-04-15)

commit 3326c74
Author: Scott Lahteine <thinkyhead@users.noreply.github.com>
Date:   Sun Apr 14 16:26:16 2024 -0500

    📝 Minor README changes

commit 0269106
Author: Scott Lahteine <thinkyhead@users.noreply.github.com>
Date:   Sun Apr 14 16:24:14 2024 -0500

    🎨 Dagoma D6 followup

commit 95d38a8
Author: Sophist <3001893+Sophist-UK@users.noreply.github.com>
Date:   Sun Apr 14 21:04:52 2024 +0100

    ✨ Add Dagoma D6 as found in DiscoUltimate v2 TMC (MarlinFirmware#26874)

    * Add Dagoma D6 board as used in their DiscoUltimate v2 TMC.

    Taken from the Dagoma fork of Marlin DU_MC branch where it is called FYSETC_DAGOMA_F5 and explicitly confirmed by Dagoma as being the D6:

    "the BOARD_FYSETC_DAGOMA_F5 is effectively the definition for the D6"

    ---------

    Co-authored-by: thisiskeithb <13375512+thisiskeithb@users.noreply.github.com>
    Co-authored-by: Orel <37673727+0r31@users.noreply.github.com>

commit dca6afc
Author: Chris <52449218+shadow578@users.noreply.github.com>
Date:   Sun Apr 14 20:42:57 2024 +0200

    ✨🐛 HC32 - Add SERIAL_DMA, fix SDIO and MEATPACK (MarlinFirmware#26845)

    * fix meatpack on hc32

    * add support for SERIAL_DMA on HC32

    * add additional checks in HC32 HAL

    * migrate HC32 HAL to use app_config.h

    * fix memory leak in HC32 sdio HAL
    MarlinFirmware#26845 (comment)

    * hc32: fail if both EMERGENCY_PARSER and SERIAL_DMA are enabled

commit 19684f2
Author: Jason Smith <jason.inet@gmail.com>
Date:   Sat Apr 13 17:49:08 2024 -0700

    ✅ Add unit tests for macros.h (MarlinFirmware#26968)

commit 52a5613
Author: Keith Bennett <13375512+thisiskeithb@users.noreply.github.com>
Date:   Sat Apr 13 17:47:16 2024 -0700

    ⏪️ Revert unintended README changes (MarlinFirmware#26967)

    * Revert all the changes that went in with the unit test framework
    This restored broken links and other changes. Restoring to the prior revision seems the most appropriate action until the intentions of those file changes are known.
    ---------

    Co-authored-by: Jason Smith <jason.inet@gmail.com>

commit 0683e8a
Author: thinkyhead <thinkyhead@users.noreply.github.com>
Date:   Sun Apr 14 00:24:15 2024 +0000

    [cron] Bump distribution date (2024-04-14)

commit 1bb4a04
Author: Jason Smith <jason.inet@gmail.com>
Date:   Sat Apr 13 14:11:51 2024 -0700

    ✅Unit test improvements (MarlinFirmware#26965)

    * Do not warn about display in unit tests

    * Treat warnings as errors in unit tests

    * Report actual filenames with unit tests

commit d10861e
Author: Jason Smith <jason.inet@gmail.com>
Date:   Sat Apr 13 12:06:08 2024 -0700

    ✅ Add unit testing framework (MarlinFirmware#26948)

    - Add a framework to build and execute unit tests for Marlin.
    - Enable unit test execution as part of PR checks.

    ---------

    Co-authored-by: Costas Basdekis <costas.basdekis@gmail.com>
    Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>

commit cf7c86d
Author: Andrew <18502096+classicrocker883@users.noreply.github.com>
Date:   Sat Apr 13 14:59:59 2024 -0400

    🔧Fix M936 in features.ini (MarlinFirmware#26957)

commit d99e150
Author: David Buezas <dbuezas@users.noreply.github.com>
Date:   Sat Apr 13 18:54:25 2024 +0200

    ⚡️Reduce DISPLAY_SLEEP_MINUTES overhead (MarlinFirmware#26964)
@sjasonsmith sjasonsmith deleted the InWork/costas/2024_better-tests-rebased-squashed2 branch April 21, 2024 16:44
@thisiskeithb thisiskeithb mentioned this pull request May 7, 2024
RPGFabi pushed a commit to RPGFabi/Marlin that referenced this pull request Jun 15, 2024
- Add a framework to build and execute unit tests for Marlin.
- Enable unit test execution as part of PR checks.

---------

Co-authored-by: Costas Basdekis <costas.basdekis@gmail.com>
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants