Skip to content

Add comprehensive unit testing infrastructure#85

Open
samjhill wants to merge 2 commits intoBenjaminPoilve:mainfrom
samjhill:feature/add-unit-testing
Open

Add comprehensive unit testing infrastructure#85
samjhill wants to merge 2 commits intoBenjaminPoilve:mainfrom
samjhill:feature/add-unit-testing

Conversation

@samjhill
Copy link
Contributor

@samjhill samjhill commented Oct 13, 2025

  • Add PlatformIO native test environment configuration
  • Extract chord calculation logic into testable modules
  • Create 15 unit tests (10 chord logic, 5 serialization)
  • Add GitHub Actions CI workflow for automated testing
  • Add comprehensive testing documentation (TESTING.md, README updates)
  • Fix typos in main.cpp (SOFWTARE -> SOFTWARE, adress -> address)

Tests cover:

  • Chord note calculations (major, minor, 7th, etc.)
  • Sharp/flat modifiers
  • Slash chords
  • Harp chromatic and chord modes
  • CSV serialization/deserialization
  • Edge cases and boundary conditions

All tests pass on native platform without hardware dependencies.

@samjhill samjhill marked this pull request as ready for review October 13, 2025 20:17
@samjhill
Copy link
Contributor Author

samjhill commented Oct 13, 2025

@BenjaminPoilve I am not able to add you as a reviewer. Let me know if you have any questions or suggestions

@samjhill samjhill force-pushed the feature/add-unit-testing branch from c14f5e5 to 17ff0c7 Compare October 13, 2025 20:27
@samjhill
Add comprehensive unit testing infrastructure
dd9d253 
Implements unit testing framework for firmware with 15 passing tests covering
core musical logic and parameter handling.

Testing Infrastructure:
- PlatformIO native test environment configuration
- Unity test framework integration
- GitHub Actions CI workflow for automated testing
- Comprehensive testing documentation

New Files:
- .github/workflows/firmware-tests.yml - CI automation
- TESTING_SUMMARY.md - Executive summary
- firmware/TESTING.md - Comprehensive testing guide (300+ lines)
- firmware/include/chord_logic.h - Testable chord calculation interface
- firmware/src/chord_logic.cpp - Pure function implementations
- firmware/test/test_chord_logic/test_chord_logic.cpp - 10 chord tests
- firmware/test/test_serialization/test_serialization.cpp - 5 serialization tests

Test Coverage:
- Chord note calculations (major, minor, 7th, augmented, diminished)
- Sharp/flat modifiers
- Slash chord functionality
- Harp chromatic and chord modes
- Different root notes (circle of fifths)
- CSV serialization/deserialization
- Edge cases and boundary conditions

Modified Files:
- firmware/README.md - Added testing section with quick start guide
- firmware/platformio.ini - Added [env:native] for testing
- firmware/src/main.cpp - Fixed typos (SOFWTARE→SOFTWARE, adress→address)

Benefits:
- Tests run without hardware (native platform)
- Faster development cycle
- Catch regressions early via CI
- Lower barrier for contributors
- Self-documenting code behavior

All 15 tests pass on native platform.
@samjhill samjhill force-pushed the feature/add-unit-testing branch from 17ff0c7 to dd9d253 Compare October 13, 2025 20:30
@BenjaminPoilve BenjaminPoilve self-requested a review October 14, 2025 06:33
@BenjaminPoilve
Copy link
Owner

BenjaminPoilve commented Oct 14, 2025

Thanks a lot for this important contribution! I'll need a bit of time to review this thoroughly as there are quite a lot of modifications. Did you design the test with the original code, have them pass, then do the refactoring?

@samjhill
Copy link
Contributor Author

samjhill commented Oct 15, 2025 •
edited
Loading

@BenjaminPoilve Initially, I wrote tests against a simplified version, not the original code. I've now fixed this. The extracted functions now match your original behavior exactly. All 15 tests pass and produce identical results to the original firmware.
The unit tests now properly validate the original musical behavior, so future changes won't break the chord calculations. Once this is all in place, we could decide to include the simplified & extracted versions I had before.

@samjhill samjhill force-pushed the feature/add-unit-testing branch 2 times, most recently from ecaf784 to 5ca789e Compare October 15, 2025 16:21
@samjhill
Fix chord logic to match original behavior exactly
5bb9540 
- Added complete get_root_button implementation with key signatures and frame shifts
- Added original calculate_note_chord and calculate_note_harp functions
- Updated wrapper functions to use original behavior with default parameters
- Fixed header file to avoid duplicate definitions
- All tests now pass with original firmware behavior
@samjhill samjhill force-pushed the feature/add-unit-testing branch from 5ca789e to 5bb9540 Compare October 15, 2025 16:50
@BenjaminPoilve
Copy link
Owner

BenjaminPoilve commented Oct 15, 2025

Thanks a lot! I'll have to dedicate a timeslot for a thorough review. But in any case thanks for bringing in good practices to this project :)

@terminalwaltz
Copy link
Contributor

terminalwaltz commented Oct 20, 2025

@samjhill This is incredible, thank you for taking the time to put this together -- I'll be sure to integrate this practice into my pull requests, this is exactly the kind of testing I need to keep from chasing my tail :D

@samjhill
Copy link
Contributor Author

samjhill commented Oct 21, 2025

@terminalwaltz @BenjaminPoilve glad you guys like this! happy to walk you through anything as you get into the review. I have some further enhancements in mind once we get this solid foundation laid

@BenjaminPoilve
Copy link
Owner

BenjaminPoilve commented Oct 24, 2025

I want to apologize for the delay: I'm in the middle of preparing my trip the Shenzhen markefaire, and in the middle of moving/home renovation which leaves me less time for testing. I'll get to it as soon as I get the time!

@samjhill
Copy link
Contributor Author

samjhill commented Oct 29, 2025

no rush at all. It will be here when you get more time. 🤗

@BenjaminPoilve
Copy link
Owner

BenjaminPoilve commented Nov 28, 2025

Hi there,
I'm testing it and all works fine. Especially the ability to test things on the teensy itself is great, I can't say that I was aware of that ability. That's great work, and I thank you for taking the time.

I apologise in advance if my questions are basic, but I'm really not familiar with testing in platformIO.

  • I personally don't have the test options in the platformIO side panel described in the readme, they are launched by using the beaker icon in the bottom bar. Is that a config issue on my side?
  • I understand that the [env:native] entry in the platformio.ini is the environment for the test, am I right? When compiling, I run into something weirds: it tries to compile both for the [env:teensy40] and for the [env:native], and that last one understandably throws errors (for one thing it does not know Arduino.h). Is that again a misconfiguration on my side?

Sorry again if it shows misunderstanding of the underlying testing tools !

@BenjaminPoilve
Copy link
Owner

BenjaminPoilve commented Dec 5, 2025

Let me know @samjhill if/when you have time to clarify those last points :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BenjaminPoilve BenjaminPoilve Awaiting requested review from BenjaminPoilve

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@samjhill @BenjaminPoilve @terminalwaltz