From f18006e9e1b0fc54e0952b9bf40fdbb88694acbf Mon Sep 17 00:00:00 2001 From: RF Simulator Bot Date: Mon, 10 Aug 2026 08:10:56 +0200 Subject: [PATCH] ci: run the test suite on every PR; add fs_Hz and S-param reload regression coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C2: the pull-request workflow built but NEVER ran a test — the entire suite ran only at minor/major release tags. Add a Test step (xvfb-run ctest) to ci.yml so regressions fail on every PR. T1: add fs_Hz propagation tests for combiner, attenuator (manual), equalizer, and ideal_filter — the pass-through engines the #54 fix touched but never covered — and fix the vacuous ideal_filter 'preserves fs_Hz' test that compared 0.0 == 0.0. T2: strengthen the #56 S-param reload test to drive a tone through the loaded amplifier and compare output against the interpolated S21 (catches the reloaded-but-ideal-gain bug the old flag-only assertions missed); fix mislabeled issue numbers. T3: root-cause the Windows 'ADC DDC output grid spans' exclusion: the bracket-paren '[...)' in the TEST_CASE name broke catch_discover_tests' unquoted list parsing on all platforms, merging it and every later test into one ';'-joined CTest entry that Catch2 rejects as Invalid Filter. Rename to 'from -Fs/4 to Fs/4' (engine math was correct) and remove the release.yml exclusion; document the remaining test_ui exclusion. Add a CI guard for the MinGW TEST_CASE registration ceiling (--list-tests count floor in release.yml) and update tests/AGENTS.md to the verified 223. --- .github/workflows/ci.yml | 7 +++- .github/workflows/release.yml | 30 ++++++++++++++- tests/AGENTS.md | 4 +- tests/test_adc.cpp | 8 +++- tests/test_ideal_filter.cpp | 18 ++++++--- tests/test_project_file.cpp | 33 +++++++++++++++- tests/test_signal_domain.cpp | 71 ++++++++++++++++++++++++++++++++++- 7 files changed, 157 insertions(+), 14 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 324b818..91aca6a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -33,7 +33,7 @@ jobs: - name: Install Linux dependencies run: | sudo apt-get update -qq - sudo apt-get install -y -qq ninja-build cmake pkg-config g++-14 \ + sudo apt-get install -y -qq ninja-build cmake pkg-config xvfb \ libxrandr-dev libxinerama-dev libxcursor-dev libxi-dev libxext-dev libgl1-mesa-dev - name: Configure @@ -46,3 +46,8 @@ jobs: - name: Build run: cmake --build build + + # Run the full suite on every PR (not just release tags). Mirrors the + # release.yml strict-build invocation; xvfb-run gives test_ui a display. + - name: Test + run: xvfb-run --auto-servernum ctest --test-dir build --output-on-failure -E "Benchmark" diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 4dd46ea..ef5210a 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -134,7 +134,35 @@ jobs: if [ "${{ runner.os }}" = "Linux" ]; then xvfb-run --auto-servernum ctest --test-dir build --output-on-failure -E "Benchmark" else - ctest --test-dir build --output-on-failure -E "Benchmark|test_ui|ADC DDC output grid spans" + # test_ui is excluded on Windows: it drives an interactive ImGui + # window and needs a display, so it cannot run headless in CI. + # ("ADC DDC output grid spans [-Fs/4, Fs/4)" was previously excluded + # by name too. Root cause: the bracket-paren pattern in that name + # broke catch_discover_tests' unquoted list parsing, merging it and + # every later test into one ';'-joined CTest entry that Catch2 + # rejects as "Invalid Filter". The test was renamed to "ADC DDC + # output grid spans from -Fs/4 to Fs/4" (tests/test_adc.cpp) and now + # registers as its own CTest entry, so the exclusion is removed.) + ctest --test-dir build --output-on-failure -E "Benchmark|test_ui" + fi + + - name: Verify MinGW TEST_CASE registration count + if: runner.os == 'Windows' + run: | + # The MinGW-w64 toolchain silently drops TEST_CASEs registered beyond + # a ceiling in the main `tests` binary (see tests/AGENTS.md). Count + # the registered cases (each emits one indented tag line) and fail if + # the count ever falls below the verified floor, so a silent drop is + # caught in CI instead of shipping unrun tests. Bump EXPECTED when the + # registered count changes; new coverage that must run on Windows goes + # in a standalone executable (e.g. test_signal_domain), not the main + # tests binary. + EXPECTED=223 + COUNT=$(build/bin/tests.exe --list-tests | grep -c '^ \[') + echo "registered TEST_CASEs: $COUNT (expected >= $EXPECTED)" + if [ "$COUNT" -lt "$EXPECTED" ]; then + echo "ERROR: tests.exe registered $COUNT TEST_CASEs, below the verified floor of $EXPECTED" + exit 1 fi sanitizer: diff --git a/tests/AGENTS.md b/tests/AGENTS.md index 1c1eed6..7e48743 100644 --- a/tests/AGENTS.md +++ b/tests/AGENTS.md @@ -20,9 +20,9 @@ Own the Catch2 v3.4.0 unit test suite and ImGui test engine UI tests. Verify all - Test files are named `test_.cpp` matching the module name - Build via `cmake --build build && ctest --test-dir build` or direct `build/bin/tests` - Adding a new module? Add its test source to `TEST_SOURCES` in `CMakeLists.txt` and link the library target -- **MinGW-w64 test-registration ceiling:** this toolchain silently drops any `TEST_CASE` registered beyond the ~217 already linked into the main `tests` executable (confirmed via a from-scratch clean rebuild; see the comment above `test_component_authoring` in `CMakeLists.txt`). Do not add new `TEST_CASE`s to `test_main.cpp` or any file already compiled into the `tests` target — give the new coverage its own standalone executable instead (`add_executable(test_ test_.cpp)` + `target_link_libraries` + `add_test`, following `test_attenuator`/`test_combiner`/`test_component_authoring`/`test_extensions`/`test_signal_domain`), and run it directly (`build/bin/test_.exe`) rather than relying on `ctest`. +- **MinGW-w64 test-registration ceiling:** this toolchain silently drops any `TEST_CASE` registered beyond the ~223 already linked into the main `tests` executable (verified 2026-08-09; the release.yml Windows job enforces the 223 floor with a `--list-tests` count guard). Do not add new `TEST_CASE`s to `test_main.cpp` or any file already compiled into the `tests` target — give the new coverage its own standalone executable instead (`add_executable(test_ test_.cpp)` + `target_link_libraries` + `add_test`, following `test_attenuator`/`test_combiner`/`test_component_authoring`/`test_extensions`/`test_signal_domain`), and run it directly (`build/bin/test_.exe`) rather than relying on `ctest`. - Platform-specific tests (e.g., Windows-only session state) are gated with `#ifdef WIN32` in CMakeLists.txt -- `test_component_authoring` and `test_tutorial_state` are standalone executables, not part of `TEST_SOURCES`: the MinGW-w64 toolchain silently drops `TEST_CASE`s registered beyond the ~217 already linked into `tests`. New test files that must run on Windows should follow that pattern. +- `test_component_authoring` and `test_tutorial_state` are standalone executables, not part of `TEST_SOURCES`: the MinGW-w64 toolchain silently drops `TEST_CASE`s registered beyond the ~223 already linked into `tests`. New test files that must run on Windows should follow that pattern. ## Work Guidance diff --git a/tests/test_adc.cpp b/tests/test_adc.cpp index 7375e23..bf13eb2 100644 --- a/tests/test_adc.cpp +++ b/tests/test_adc.cpp @@ -79,7 +79,13 @@ TEST_CASE("ADC DDC tone at 0 Hz maps to -Fs/4", "[adc]") { REQUIRE(out.tones[0].freq_Hz == Approx(-Fs / 4.0).margin(1.0)); } -TEST_CASE("ADC DDC output grid spans [-Fs/4, Fs/4)", "[adc]") { +// Name avoids the "[-Fs/4, Fs/4)" bracket-paren pattern: catch_discover_tests +// (Catch2 v3.4.0) expands the discovered name list unquoted, and an unclosed +// '[' followed by ')' breaks CMake's list splitting, merging this test and +// every name after it into one ';'-joined CTest entry that Catch2 rejects as +// an "Invalid Filter" (observed on CMake 4.x; the test otherwise passes on +// MinGW). +TEST_CASE("ADC DDC output grid spans from -Fs/4 to Fs/4", "[adc]") { NodeGraphEngine graph; AdcEngine adc(3, graph); adc.setFs_Hz(Fs); diff --git a/tests/test_ideal_filter.cpp b/tests/test_ideal_filter.cpp index 46aa2ac..fa4c764 100644 --- a/tests/test_ideal_filter.cpp +++ b/tests/test_ideal_filter.cpp @@ -141,17 +141,25 @@ TEST_CASE("IdealFilter passes noise density unchanged in passband", "[filter]") TEST_CASE("IdealFilter preserves fs_Hz from input", "[filter]") { NodeGraphEngine graph; - SignalGeneratorEngine gen(0, graph); - gen.addTone(100e6, -20.0); - gen.update(0.0); + + // Drive from a synthetic spectrum with a NONZERO fs_Hz. The generator's + // output fs_Hz is 0.0, so the old version of this test compared 0.0 == 0.0 + // and could never catch a dropped fs_Hz (issues #43/#54). + Spectrum in; + in.frequencies = {100e6, 200e6}; + in.tones = {{100e6, -20.0, 0.0}}; + in.noise_W.assign(2, 1e-20); + in.noise_added_W.assign(2, 0.0); + in.noise_total_W.assign(2, 1e-20); + in.fs_Hz = 500e6; IdealFilterEngine filt(0, graph); filt.setFilterType(FilterType::LPF); filt.setCutoff_Hz(200e6); - filt.node().inputs[0] = &gen.node().outputs[0]; + filt.node().inputs[0] = ∈ filt.update(0.0); - REQUIRE(filt.node().outputs[0].fs_Hz == gen.node().outputs[0].fs_Hz); + REQUIRE(filt.node().outputs[0].fs_Hz == Approx(500e6)); } TEST_CASE("IdealFilter dirty flag skips when input unchanged", "[filter]") { diff --git a/tests/test_project_file.cpp b/tests/test_project_file.cpp index 0cae55f..20c7282 100644 --- a/tests/test_project_file.cpp +++ b/tests/test_project_file.cpp @@ -6,10 +6,13 @@ #include "implot.h" #include #include +#include +#include #include #include #include #include +#include #include using Catch::Approx; @@ -440,7 +443,7 @@ TEST_CASE_METHOD(ImGuiFixture, "Round-trip: default component positions are (0,0 } // --------------------------------------------------------------------------- -// 13 — Issue #44: S-param mode survives save/load for amplifier, ideal filter, +// 13 — Issue #56: S-param mode survives save/load for amplifier, ideal filter, // equalizer, attenuator, and combiner. Previously deserialize() restored // sparam_mode/sparam_filepath but never reloaded the Touchstone file, so // a reloaded project silently fell back to ideal/manual mode. @@ -450,7 +453,7 @@ static std::string sparamFixturePath() { "/component_data/amplifiers/adm-3844psm/ADM-8344PSM_SM_A_25C_De_5V_5V_102mA.s2p"; } -TEST_CASE_METHOD(ImGuiFixture, "Round-trip: S-param mode survives save/load (issue #44)", +TEST_CASE_METHOD(ImGuiFixture, "Round-trip: S-param mode survives save/load (issue #56)", "[project_file][sparam]") { auto path = tempPath(); std::remove(path.c_str()); @@ -492,6 +495,32 @@ TEST_CASE_METHOD(ImGuiFixture, "Round-trip: S-param mode survives save/load (iss CHECK(amps[0]->sparamMode() == true); CHECK(amps[0]->sparamLoaded() == true); + // Issue #56 regression: sparamMode()/sparamLoaded() are necessary but + // not sufficient — the old deserialize() restored the mode flags and + // filepath without reloading the Touchstone file, so a reloaded + // amplifier reported sparamMode()==true but applied ideal gain. Drive + // a tone through the loaded amplifier via the app DSP chain and + // compare the output against the S21-derived expectation (mirrors + // tests/test_amplifier_sparam.cpp). + auto &gen = app.testComponents().add(20001, app.testGraphEngine()); + gen.addTone(1e9, -20.0); + gen.update(0.0); + + int gen_pin = gen.outputPinId(); + int amp_pin = amps[0]->inputPinId(); + int link_id = app.testGraphEngine().addLink(gen_pin, amp_pin); + REQUIRE(link_id > 0); + + app.update_dsp(); + + const auto &_out = amps[0]->node().outputs[0]; + REQUIRE(amp_out.tones.size() == 1); + auto S21 = amps[0]->sparamData().interpolate(1e9, 2); + double expected_gain = 20.0 * std::log10(std::abs(S21)); + REQUIRE(amp_out.tones[0].power_dBm == Approx(-20.0 + expected_gain).margin(0.5)); + double expected_phase = std::arg(S21) * 180.0 / std::numbers::pi; + REQUIRE(amp_out.tones[0].phase_deg == Approx(expected_phase).margin(1.0)); + auto flts = app.testComponents().byType(); REQUIRE(flts.size() == 1); CHECK(flts[0]->sparamMode() == true); diff --git a/tests/test_signal_domain.cpp b/tests/test_signal_domain.cpp index bad2786..a894f16 100644 --- a/tests/test_signal_domain.cpp +++ b/tests/test_signal_domain.cpp @@ -419,7 +419,7 @@ TEST_CASE("SpectrumAnalyzer: complex-baseband tone renders unchanged (no mirrori REQUIRE(trace[10] < -50.0); } -// ---- fs_Hz propagation (issue #43) ---- +// ---- fs_Hz propagation (issues #43/#54) ---- TEST_CASE("Mixer: propagates fs_Hz", "[domain][mixer]") { NodeGraphEngine graph; @@ -505,8 +505,75 @@ TEST_CASE("Amplifier: propagates fs_Hz (S-param mode)", "[domain][amplifier]") { REQUIRE(amp.node().outputs[0].fs_Hz == Approx(500e6)); } +TEST_CASE("Combiner: propagates fs_Hz", "[domain][combiner]") { + NodeGraphEngine graph; + CombinerEngine comb(0, graph); + + Spectrum in0, in1; + in0.frequencies = {1e9, 2e9}; + in0.noise_total_W.assign(2, 1e-21); + in0.fs_Hz = 500e6; + in1.frequencies = {1e9, 2e9}; + in1.noise_total_W.assign(2, 1e-21); + in1.fs_Hz = 500e6; + + comb.node().inputs[0] = &in0; + comb.node().inputs[1] = &in1; + comb.update(0.0); + + REQUIRE(comb.node().outputs[0].fs_Hz == Approx(500e6)); +} + +TEST_CASE("Attenuator: propagates fs_Hz (manual mode)", "[domain][attenuator]") { + NodeGraphEngine graph; + AttenuatorEngine atten(0, graph); + + Spectrum in; + in.frequencies = {1e9, 2e9}; + in.tones = {{1e9, -10.0, 0.0}}; + in.noise_total_W.assign(2, 1e-21); + in.fs_Hz = 500e6; + + atten.node().inputs[0] = ∈ + atten.update(0.0); + + REQUIRE(atten.node().outputs[0].fs_Hz == Approx(500e6)); +} + +TEST_CASE("Equalizer: propagates fs_Hz", "[domain][equalizer]") { + NodeGraphEngine graph; + EqualizerEngine eq(0, graph); + + Spectrum in; + in.frequencies = {1e9, 2e9}; + in.tones = {{1e9, -10.0, 0.0}}; + in.noise_total_W.assign(2, 1e-21); + in.fs_Hz = 500e6; + + eq.node().inputs[0] = ∈ + eq.update(0.0); + + REQUIRE(eq.node().outputs[0].fs_Hz == Approx(500e6)); +} + +TEST_CASE("IdealFilter: propagates fs_Hz", "[domain][ideal_filter]") { + NodeGraphEngine graph; + IdealFilterEngine flt(0, graph); + + Spectrum in; + in.frequencies = {1e9, 2e9}; + in.tones = {{1e9, -10.0, 0.0}}; + in.noise_total_W.assign(2, 1e-21); + in.fs_Hz = 500e6; + + flt.node().inputs[0] = ∈ + flt.update(0.0); + + REQUIRE(flt.node().outputs[0].fs_Hz == Approx(500e6)); +} + // Real multi-engine post-ADC chains: fs_Hz must reach the PFB and channels must be populated -// without any manual setFs_Hz() (issue #43 regression tests). +// without any manual setFs_Hz() (issues #43/#54 regression tests). static bool anyChannelHasContent(const PFBChannelizerEngine &pfb) { for (const auto &ch : pfb.channels()) {