feat: add initial host based unittests #23

jedi7 · 2025-10-02T11:29:33Z

Description

Propose for host based unittesting.

ThrowTheSwitch/Unity and CMock were selected because tests are also planned on the target hardware.

There target tests are still quite experimental and have issues. Like they are not compatible with stub for now. And is hard to gather the results of the tests.

Testing

Checklist

Before submitting a Pull Request, please ensure the following:

🚨 This PR does not introduce breaking changes.
All CI checks (GH Actions) pass.
Documentation is updated as needed.
Tests are updated or added as necessary.
Code is well-commented, especially in complex areas.
Git history is clean — commits are squashed to the minimum necessary.

github-actions · 2025-10-02T11:30:08Z

	Messages
📖	This PR seems to be quite large (total lines of code: 2376), you might consider splitting it into smaller PRs

👋 Hello jedi7, we appreciate your contribution to this project!

Click to see more instructions ...

This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Addressing info messages (📖) is strongly recommended; they're less critical but valuable.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...

We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against 0a54e43

dobairoland

LGTM in general. Nice work!

Could you please also add a Github action which would build and run the unit tests at every push?

unittests/run-tests.sh

unittests/CMakeLists.txt

unittests/tests/CMakeLists.txt

unittests/CMakeLists.txt

unittests/run-tests.sh

unittests/generate_mocks.sh

Copilot

Pull Request Overview

This PR adds initial host-based unit testing infrastructure using the Unity and CMock frameworks for the ESP flasher stub project. The setup enables testing C code on the host system before deploying to target hardware.

Key changes:

Implementation of comprehensive SLIP protocol unit tests covering encoding, decoding, and error handling
CMake-based build system with automated mock generation for external dependencies
Shell scripts for automated test execution and mock file generation

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
unittests/tests/TestSlip.c	Comprehensive unit tests for SLIP protocol functions including frame delimiters, data encoding/escaping, and receive byte handling
unittests/tests/CMakeLists.txt	CMake configuration for building the SLIP test executable with proper dependencies
unittests/run-tests.sh	Main test runner script supporting both CMake and Rake build methods with dependency checking
unittests/generate_mocks.sh	Script to generate CMock files for rom_wrappers header with proper error handling
unittests/cmock_config.yml	CMock configuration file defining mock generation settings and behavior
unittests/CMakeLists.txt	Root CMake configuration for the unit test framework with Unity/CMock integration

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

unittests/generate_mocks.sh

jedi7 · 2025-10-03T07:02:45Z

@dobairoland Thank you for the review! I will incorporate everything. This was meant as a preview to see if I should continue in this way.

dobairoland · 2025-10-03T07:32:53Z

@jedi7 Yes, it look nice, please continue. Don't add new tests for now but make it easy for the guys to try. The instructions in the README will help.

unittests/generate_mocks.sh

radimkarnis · 2025-10-03T08:27:03Z

Nice job! After cloning the two repos, running this is very straightforward and easy. With some basic info in README, this is a great start, thank you.

jedi7 · 2025-10-03T14:42:33Z

the host based tests should be ok now. The target based tests are not currently running (they need to update ld scritps)

Dzarda7 · 2025-10-06T08:30:47Z

Nice job! Took a look at how tests can be done a ran the prepared one and seems like really nice addition, thanks.

jedi7 · 2025-10-07T08:54:03Z

There target tests are still quite experimental and have issues. Like they are not compatible with stub for now. And is hard to gather the results of the tests.

jedi7 · 2025-10-07T08:57:31Z

Are these initial tests ok for you? @Dzarda7 @dobairoland @radimkarnis
Thanks :)

Copilot

Pull Request Overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

unittests/target/run-tests.sh

unittests/target/minimal_system.c

unittests/target/TestTargetFlash.c

dobairoland

LGTM in general. Nice progress, thank you!

unittests/README.md

dobairoland · 2025-10-07T12:02:31Z

cmake/esp-build-config.cmake

@@ -0,0 +1,70 @@
+# ESP Build Configuration
+# Common build settings shared across the esp-flasher-stub project


I think these are just for unittests

currently yes, but I believe, we can update also the main stub cmake to use that.

Can you do it in this PR? Duplicating part of the main Cmake file here is dangerous in terms of that they can diverge if the follow-up is not done immediately. I think this would belong here because during this process you might identify other modifications needed to be done in the current PR.

unittests/target/CMakeLists.txt

jedi7 · 2025-10-09T10:51:01Z

Hi all (@radimkarnis @Dzarda7 @dobairoland ), the target tests are now ready. The supported chip for the target tests is currently only ESP32-S3, but we can add another.
Please look at it.
Thanks!

dobairoland · 2025-10-09T12:33:57Z

Could you please also add a Github action which would build and run the unit tests at every push?

Could this also be done in this PR? This shouldn't be too time consuming and ensures that these test don't broke just because someone forgot to run them locally before merging another PR.

dobairoland · 2025-10-09T12:35:03Z

Could someone test the target tests please? I don't have an ESP32-S3 with me right now.

dobairoland · 2025-10-09T12:48:04Z

cmake/esp-targets.cmake

@@ -0,0 +1,68 @@
+# ESP Target Definitions and Utilities
+# Shared across the entire esp-flasher-stub project


Please use this as well across the project, as the comment says.

dobairoland · 2025-10-09T12:49:07Z

unittests/host/CMakeLists.txt

+project(esp-flasher-stub-host-tests C)
+
+# Host-specific configuration (native build)
+set(CMAKE_C_FLAGS "-std=gnu17 -Wall -Wextra -Werror -g -O0 -fprofile-arcs -ftest-coverage")


Can we take part from the project's C flags?

dobairoland · 2025-10-09T12:55:02Z

unittests/target/CMakeLists.txt

+configure_esp_tests(${ESP_TARGET})
+
+# Check for Python and esptool module
+# First try ESP-IDF's Python environment if available


Please don't add dependency on ESP-IDF. Why would we want it here?

The main README suggests to create a venv. The tests could rely on this and extend if needed. Or for what else would we need the ESP-IDF environment? Toolchains? We are setting them up using the helper scripts.

We can assume that the usage for everyone is described in the main README. The unittests (host or target) are an extention to it. Not everyone will want to set up unit testing. But the unit testing setup should build on how the main project usage was set up.

dobairoland · 2025-10-09T12:56:14Z

unittests/target/CMakeLists.txt

+if(DEFINED ENV{IDF_PYTHON_ENV_PATH} AND EXISTS "$ENV{IDF_PYTHON_ENV_PATH}/bin/python")
+    set(PYTHON3 "$ENV{IDF_PYTHON_ENV_PATH}/bin/python")
+else()
+    find_program(PYTHON3 python3)


python is available as per instructions from the local venv. Esptool is already installed there.

Copilot

Pull Request Overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

unittests/target/run-tests.sh:1

Inconsistent default timeout values in usage help (10 seconds) versus actual default assignment (5 seconds on line 69). The help text should match the actual default value.

#!/bin/bash

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

unittests/target/TestTargetFlash.c

unittests/target/run-tests.sh

cmake/esp-targets.cmake

radimkarnis · 2025-10-09T14:41:16Z

@dobairoland tested now locally with Jarek, and after small changes, this works on MacOS ✅

jedi7 requested review from dobairoland, Dzarda7 and radimkarnis October 2, 2025 13:34

dobairoland requested changes Oct 2, 2025

View reviewed changes

dobairoland requested a review from Copilot October 2, 2025 14:33

Copilot AI reviewed Oct 2, 2025

View reviewed changes

unittests/generate_mocks.sh Outdated Show resolved Hide resolved

unittests/generate_mocks.sh Outdated Show resolved Hide resolved

radimkarnis reviewed Oct 3, 2025

View reviewed changes

unittests/generate_mocks.sh Outdated Show resolved Hide resolved

jedi7 force-pushed the feature/add_unity_unittests branch from 8a96916 to 5449ff2 Compare October 3, 2025 14:41

jedi7 changed the title ~~feat: add initial host based unittests~~ WIP feat: add initial host based unittests Oct 3, 2025

jedi7 force-pushed the feature/add_unity_unittests branch from 5449ff2 to a1c56d4 Compare October 7, 2025 08:51

jedi7 changed the title ~~WIP feat: add initial host based unittests~~ feat: add initial host based unittests Oct 7, 2025

dobairoland requested a review from Copilot October 7, 2025 11:46

Copilot AI reviewed Oct 7, 2025

View reviewed changes

unittests/target/run-tests.sh Outdated Show resolved Hide resolved

unittests/target/minimal_system.c Outdated Show resolved Hide resolved

jedi7 force-pushed the feature/add_unity_unittests branch from a1c56d4 to 8715ce5 Compare October 7, 2025 12:07

radimkarnis reviewed Oct 7, 2025

View reviewed changes

unittests/target/TestTargetFlash.c Show resolved Hide resolved

dobairoland requested changes Oct 7, 2025

View reviewed changes

jedi7 force-pushed the feature/add_unity_unittests branch 3 times, most recently from 2035a34 to 9b39f2f Compare October 9, 2025 10:48

dobairoland reviewed Oct 9, 2025

View reviewed changes

dobairoland requested a review from Copilot October 9, 2025 13:24

Copilot AI reviewed Oct 9, 2025

View reviewed changes

unittests/target/TestTargetFlash.c Show resolved Hide resolved

unittests/target/run-tests.sh Show resolved Hide resolved

cmake/esp-targets.cmake Show resolved Hide resolved

jedi7 force-pushed the feature/add_unity_unittests branch 4 times, most recently from e2c0c8a to 73a4f23 Compare October 9, 2025 14:38

feat: add initial host and target based unittests

0a54e43

jedi7 force-pushed the feature/add_unity_unittests branch 2 times, most recently from 5e589e2 to 0a54e43 Compare October 9, 2025 15:11

		@@ -0,0 +1,70 @@
		# ESP Build Configuration
		# Common build settings shared across the esp-flasher-stub project

		@@ -0,0 +1,68 @@
		# ESP Target Definitions and Utilities
		# Shared across the entire esp-flasher-stub project

feat: add initial host based unittests #23

Are you sure you want to change the base?

feat: add initial host based unittests #23

Uh oh!

Conversation

jedi7 commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related

Testing

Checklist

Uh oh!

github-actions bot commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dobairoland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

jedi7 commented Oct 3, 2025

Uh oh!

dobairoland commented Oct 3, 2025

Uh oh!

Uh oh!

radimkarnis commented Oct 3, 2025

Uh oh!

jedi7 commented Oct 3, 2025

Uh oh!

Dzarda7 commented Oct 6, 2025

Uh oh!

jedi7 commented Oct 7, 2025

Uh oh!

jedi7 commented Oct 7, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dobairoland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dobairoland Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

jedi7 Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

dobairoland Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jedi7 commented Oct 9, 2025

Uh oh!

dobairoland commented Oct 9, 2025

Uh oh!

dobairoland commented Oct 9, 2025

Uh oh!

dobairoland Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

jedi7 commented Oct 2, 2025 •

edited

Loading

github-actions bot commented Oct 2, 2025 •

edited

Loading

dobairoland Oct 9, 2025 •

edited

Loading