Phase 6: Testing + Polish

[user1303836]

Summary

• Add Catch2 v3.12.0 test suite with 26 tests across 4 files covering all DSP classes and parameter IDs
• Add CTest step to CI workflow (runs on both macOS and Windows)
• Add GPLv3 LICENSE
• Update README with CI badge, build instructions (macOS/Windows), and parameter reference table
• Refactor ParameterIDs.h from juce::String to constexpr const char* (removes juce_core dependency from the header, enables test target without JUCE plugin infrastructure)

Test coverage

File	Tests	Coverage
TestOscillator	7	Waveform bounds, frequency accuracy, phase reset, PolyBLEP dt=0 guard
TestYinPitchDetector	8	Detection at 44.1/48/96kHz, silence handling, state reset, range rejection, confidence bounds
TestPitchSmoother	8	Convergence, confidence gating, log-domain smoothing, sensitivity threshold, state reset
TestParameters	3	ID string values, uniqueness, non-empty

Test plan

• All 26 tests pass locally
• CI builds and tests pass on macOS and Windows
• Pluginval still passes on both platforms

[user1303836]

Build System / Packaging / Integration Review

Verdict: Looks good -- The build system, packaging, and integration aspects of this PR are well-structured.

Summary of findings:

CMakeLists.txt / Catch2 Integration

• Clean opt-in test infrastructure via HDN_BUILD_TESTS (defaults OFF). No impact on normal plugin builds.
• FetchContent with GIT_SHALLOW TRUE and pinned tag v3.12.0 is correct and reproducible.
• Test target compiles independently from JUCE -- only links Catch2::Catch2WithMain and pulls specific DSP source files.

ParameterIDs.h Refactor

• juce::String to constexpr const char* is safe -- all call sites accept const char* via implicit conversion to juce::String.
• This is the key enabler for JUCE-free test compilation. Matches existing codebase style.

CI Workflow

• Test step correctly placed (build -> test -> pluginval).
• ctest flags handle both single-config (Ninja/macOS) and multi-config (MSVC/Windows) generators.

LICENSE

• GPLv3 is correct given JUCE's dual licensing (GPL/commercial). Catch2's BSL-1.0 is compatible.

README

• All parameter values verified correct against PluginProcessor.cpp::createParameterLayout().
• Build instructions are accurate. Minor nit: Ninja listed as macOS requirement but not used in default build instructions.

No build artifacts or IDE files committed. .gitignore is well-configured.

[user1303836]

Bounds check for non-sine waveforms is too loose ([-1.5, 1.5])

The sine test correctly asserts [-1, 1], but the "all waveforms" test uses [-1.5, 1.5]. Looking at the Oscillator implementation, polyBLEP-corrected Square and Saw should also stay within [-1, 1] (polyBLEP only reduces overshoot, it doesn't add it). Triangle is computed as 2.0 * abs(2.0 * phase - 1.0) - 1.0 which is also bounded to [-1, 1].

This tolerance of 0.5 is unnecessarily loose -- it would mask a real regression where waveforms start producing overshooting samples. Consider tightening to [-1.01, 1.01] or at most [-1.05, 1.05] to account for floating-point edge cases while still catching meaningful problems.

[user1303836]

Build system: Clean Catch2 integration via FetchContent. GIT_SHALLOW TRUE is good for CI speed. The test target correctly only links Catch2::Catch2WithMain (no JUCE dependencies), and pulls in only Oscillator.cpp and YinPitchDetector.cpp as needed sources.

One note: PitchSmoother.h is header-only so it doesn't need a .cpp in target_sources -- that's correctly handled here. The include(Catch) + catch_discover_tests() pattern is the canonical Catch2 v3 CTest integration approach.

Minor consideration: the ${CMAKE_CURRENT_SOURCE_DIR}/../source path references work but are a bit fragile if the directory structure changes. Since this is a small project, this is fine -- just noting it.

[user1303836]

Missing coverage: NaN / infinity input to setFrequency

The polyBLEP guard handles zero dt test is good for the zero-frequency edge case, but there's no test for NaN or Infinity frequency input. In a real-time audio context, upstream pitch detection can produce NaN (e.g. division by zero in edge cases). Consider adding:

TEST_CASE("Oscillator: NaN frequency produces finite output")
{
    Oscillator osc;
    osc.prepare(kSampleRate);
    osc.setWaveform(Oscillator::Waveform::Sine);
    osc.setFrequency(std::numeric_limits<float>::quiet_NaN());

    float sample = osc.nextSample();
    REQUIRE(std::isfinite(sample));
}

Note: this test may well fail against the current implementation (since updateIncrement doesn't guard against NaN), which would be a valuable finding.

[user1303836]

Good: HDN_BUILD_TESTS defaults to OFF, so the test infrastructure (Catch2 FetchContent download, test compilation) is completely opt-in and won't affect normal plugin builds. The enable_testing() is correctly scoped inside the conditional.

This cleanly separates test concerns from the plugin build -- downstream users or CI without -DHDN_BUILD_TESTS=ON won't pull Catch2 at all.

[user1303836]

Unused variable before suppressed with (void)before

The (void)before cast suggests the variable was only generated to advance the oscillator state, but this makes the test harder to read. The intent could be clearer if you simply called generate(osc, 1) instead of capturing a named-but-unused variable. Minor nit, but reduces noise.

[user1303836]

Missing coverage: pitch detection near buffer boundaries / short buffers

The tests all feed a full second of audio (44100/48000/96000 samples), which guarantees multiple full analysis windows. Consider testing behavior with fewer samples -- e.g., feeding exactly one window's worth (2048 samples before decimation = 4096 samples at 44100 Hz). This would catch off-by-one bugs in the bufferFull / writePos logic.

Also missing: testing what happens when prepare() is called with an unusual sample rate (e.g., very low like 8000 Hz, which would produce decimation=2 and analysisSR=4000, limiting the detectable range).

[user1303836]

README review:

Accurate:

• Build instructions match the actual CMakeLists.txt setup (CMake 3.25+ requirement, --recursive for JUCE submodule)
• -DHDN_BUILD_TESTS=ON flag matches the HDN_BUILD_TESTS option name
• VST3 output path build/HdnRingmod_artefacts/Release/VST3/ is consistent with JUCE's default artifact layout

Parameter table verification against PluginProcessor.cpp:

• Mix: 0-100%, default 50% -- matches NormalisableRange<float>(0.0f, 100.0f, 0.1f), default 50.0f
• Rate Multiplier: 0.1-8.0x, default 1.0x -- matches NormalisableRange<float>(0.1f, 8.0f, 0.01f, 0.5f), default 1.0f
• Manual Rate: 20-5000 Hz, default 440 Hz -- matches NormalisableRange<float>(20.0f, 5000.0f, 0.1f, 0.3f), default 440.0f
• Mode: Pitch Track / Manual -- matches StringArray{ "Pitch Track", "Manual" }, default index 0
• Smoothing: 0-100%, default 50% -- matches range and default
• Sensitivity: 0-100%, default 50% -- matches range and default
• Waveform: Sine/Triangle/Square/Saw -- matches StringArray{ "Sine", "Triangle", "Square", "Saw" }, default index 0

All parameter values verified correct against the source code.

Minor: The macOS requirements mention Ninja (brew install ninja) but the basic build instructions don't use -G Ninja. This is fine since the CI uses Ninja but the basic instructions use the default generator -- just a small inconsistency that could confuse users who wonder why Ninja is listed as a requirement if they don't need it for the default build flow.

[user1303836]

Good test, but the "holds last frequency" assertion depends on implementation details

This test checks that when confidence drops to 0, the smoother returns the last valid frequency (~440 Hz). Looking at the PitchSmoother implementation, the process() method returns std::exp2(smoothed) when confidence is below threshold and hasValue is true.

The test works because setSmoothingAmount(0.0f) makes alpha = 1.0f, so the first valid sample sets smoothed exactly. But if you ever change the smoothing behavior at zero, this test would silently break. The tolerance of 0.1 Hz is appropriate for this case though.

Suggestion: Consider adding a test where you send multiple valid samples, then drop confidence, to verify hold behavior after converging through the smoother (not just after a single sample with zero smoothing).

[user1303836]

Missing coverage: PitchSmoother with extreme/degenerate frequency values

No tests for:

1. Very high frequency input (e.g., 20000 Hz) -- log2(20000) is ~14.3, should still work
2. Very low frequency input near 0 (e.g., 1 Hz) -- log2(1) = 0, which could interact unexpectedly with the hasValue / smoothed = 0.0f initial state
3. Rapid frequency jumps (e.g., 100 Hz -> 4000 Hz in one step) to verify the log-domain smoothing doesn't produce weird intermediate values

The log-domain implementation is correct in principle, but edge case #2 is notable: if smoothed starts at 0.0 and the first valid input is 1 Hz (log2(1) = 0), then hasValue gets set to true with smoothed = 0.0, which is the same as the initial value. This wouldn't cause a bug per se, but testing it would document the behavior.

[user1303836]

These tests are extremely low-value -- consider whether they're worth keeping

The three tests here essentially verify that compile-time string literals:

1. Are non-empty (can't fail unless someone assigns "")
2. Have the exact values they were assigned (test literally duplicates the source)
3. Are unique (which the compiler would catch as duplicate symbols if they were constexpr variables, though not for string content)

Test #2 ("IDs have expected values") is particularly fragile: if someone renames a parameter ID (e.g., "mix" -> "dryWet"), they'd also need to update this test, making it a maintenance burden with no safety benefit. The rename itself would be caught by APVTS failing to find the parameter at runtime.

The uniqueness test (#3) provides some minor value as a guardrail against copy-paste errors when adding new parameters. Consider keeping only that one and dropping the first two, or consolidating into a single test.

[user1303836]

Test Quality & Coverage Review Summary

Overall Assessment

The 26 tests are well-structured and cover the core DSP paths meaningfully. This is a solid foundation for a test suite. The Catch2 integration is clean, and the ParameterIDs refactor is backwards-compatible. However, there are notable coverage gaps and a few assertion-quality issues worth addressing.

What's Strong

• Oscillator tests (7): Good coverage of waveform bounds, frequency accuracy via zero crossings, phase reset, and the polyBLEP dt=0 guard. These test real DSP properties, not implementation details.
• YIN tests (8): Multi-sample-rate coverage (44.1k/48k/96k) is excellent. Silence handling, state reset, range rejection, and confidence bounds are all important edge cases.
• PitchSmoother tests (8): Convergence, confidence gating, log-domain verification, and the comparative smoothing-speed test are all well-designed behavioral tests.
• ParameterIDs refactor: constexpr const char* is the right move. Removes juce_core dependency from the header, enables JUCE-free test compilation, and is fully compatible with juce::ParameterID(const char*, int).

Issues Found

Assertion quality:

1. Waveform bounds test uses [-1.5, 1.5] -- too loose. All waveforms (including polyBLEP) should stay in [-1, 1]. This tolerance would mask real regressions.
2. Tolerance inconsistency in YIN tests: WithinAbs(440.0, 3.0) vs WithinRel(880.0, 0.03). Should pick one approach and use it consistently. Relative tolerance is more appropriate for pitch detection.

Coverage gaps (important scenarios NOT tested):

1. NaN/Infinity handling: No tests for NaN frequency input to Oscillator, NaN sample input to YIN, or NaN frequency input to PitchSmoother. These are realistic in audio processing (upstream division-by-zero, buffer corruption).
2. Negative frequency: Oscillator accepts any float but negative frequency is untested.
3. Buffer boundary behavior: All YIN tests feed a full second of audio. No test with minimal sample counts (e.g., exactly one analysis window worth) to catch off-by-one in writePos/bufferFull logic.
4. Low/unusual sample rates: No YIN test with 8000 Hz or 22050 Hz sample rates.

Test value:

• TestParameters.cpp provides minimal value. The "expected values" test literally duplicates the source constants. The uniqueness test is the only one with real guardrail value.

Catch2 idiom:

• Waveform loop in TestOscillator should use DYNAMIC_SECTION for per-waveform failure reporting.

Build/CI Notes

• tests/CMakeLists.txt correctly uses catch_discover_tests() for CTest integration
• PitchSmoother.cpp is absent from target_sources because it's header-only; this is correct but should be documented

Verdict

The test suite is a good v1. The tolerance issue on waveform bounds and the NaN coverage gaps are the most actionable items. Everything else is polish.

[user1303836]

constexpr const char is correct and APVTS-compatible*

This refactor is clean. juce::ParameterID constructor accepts const juce::String&, and juce::String has an implicit constructor from const char*, so this is fully backwards-compatible with all APVTS call sites (confirmed by reading PluginProcessor.cpp).

The benefit of removing the juce_core dependency from this header is meaningful -- it enables the test target to compile DSP + parameter code without pulling in JUCE plugin infrastructure.

[user1303836]

Missing: PitchSmoother.cpp not listed in target_sources

The test target compiles Oscillator.cpp and YinPitchDetector.cpp but not PitchSmoother.cpp. This works because PitchSmoother is a header-only class (all methods are inline in the header). This is fine but fragile -- if someone moves a method out-of-line into a .cpp file, the test build will break with linker errors and no obvious explanation.

Consider adding a comment to the target_sources block to make this explicit for future contributors.

[user1303836]

Catch2 usage: consider using DYNAMIC_SECTION for waveform loop

If one waveform fails here, the error message won't indicate which waveform caused the failure. Idiomatic Catch2 would use DYNAMIC_SECTION:

for (auto wf : waveforms)
{
    DYNAMIC_SECTION("waveform " << static_cast<int>(wf))
    {
        // test body
    }
}

This gives per-waveform failure reporting with minimal code change.