Run the dormant JUnit 4 tests and let test failures fail CI

[dmccoystephenson]

Summary

Three coupled test-infrastructure defects are fixed together, because fixing any one alone would leave the suite in a worse state than it is now.

• junit-vintage-engine is added as a test-scoped dependency. Nine of the twelve test classes are written against JUnit 4, but junit-jupiter-engine on the classpath causes Surefire to select the JUnit Platform provider, which only discovers JUnit 4 tests when the vintage engine is present. The suite goes from 47 tests to 125; no production code change was required for the 78 newly-executed tests to pass.
• maven.test.failure.ignore is removed from the ci-test profile. GitHub Actions sets CI=true, so that profile activates on every mvn --batch-mode clean verify run in build.yml, and a failing unit test was reported but did not fail the job. The profile's optional-dependency overrides are left untouched, so its stated purpose is preserved. The opt-in offline-test profile is deliberately left as is.
• The complexity assertion in TopRecordsAlgorithmTest is made deterministic. Wall-clock ratios were being asserted, which measure JIT warm-up and GC rather than algorithmic complexity; the test was observed failing on main at the start of this session. Comparator invocations are now counted instead, via a CountingScorable whose getScore() increments a shared counter. This mattered for ordering: enabling fatal test failures while a flaky assertion remained would have made CI unreliable rather than trustworthy.

Ordering note: the flake fix is a prerequisite for the CI change, and the CI change is what gives the newly-enabled tests any value — hence one PR rather than three.

Verification

Both directions were confirmed empirically rather than by reasoning.

Flake fix (#81) — the assertion catches a quadratic implementation. Collections.sort was temporarily replaced with a selection sort:

TopRecordsAlgorithmTest.testGetTopRecords_ComplexityValidation:411
  Comparison count suggests worse than O(n log n) for n=100: 4950 vs bound 664

Measured comparison counts for the real implementation are 533 / 3841 / 8699 / 19386 at n = 100 / 500 / 1000 / 2000 — roughly 85% of n·log2(n). The absolute bound was therefore widened to 2·n·log2(n) so that a future JDK sort implementation cannot cause a false failure, while a quadratic implementation (4950 at n=100 against a bound of 1329) is still caught decisively. A growth-ratio assertion is retained as a second, tighter check.

Vintage engine (#80) and fatal failures (#84). A temporary failing assertion was added to LeaderboardEntryTest (a JUnit 4 class) and CI=true mvn -B test was run against both states:

State	Result
Before (this PR's pom.xml stashed)	Tests run: 47, Failures: 0 — BUILD SUCCESS; the sentinel never executed
After	Tests run: 126, Failures: 1 — BUILD FAILURE on SENTINEL-JUNIT4-EXECUTED

The sentinel was reverted; CI=true mvn -B -o clean test on the committed tree reports Tests run: 125, Failures: 0, Errors: 0, Skipped: 0.

help:active-profiles was re-run under CI=true to confirm the ci-test profile still activates after the edit.

Test plan

• mvn -B -o clean test — 125 tests, green
• CI=true mvn -B -o clean test — 125 tests, green (the profile that previously masked failures is now active and fatal)
• Quadratic-implementation swap makes testGetTopRecords_ComplexityValidation fail; reverting restores green
• Sentinel JUnit 4 failure fails the build after this change and did not before
• Simple CI remains meaningful: TopRecordsAlgorithm.java itself is unmodified, so that workflow's standalone assertions still exercise the same code path

Notes for the reviewer

• pom.xml is on this loop's do-not-auto-merge list, since dependency changes affect the shaded release artifact. The added dependency is test-scoped, so no change to the shipped JAR is expected, but the merge is left to a human.
• Two further wall-clock assertions remain in the same file (duration < 100 for 1000 records, duration < 500 for 5000). Their margins are roughly two orders of magnitude, so they were left alone rather than expanding this PR's scope; they are worth revisiting if they ever flake now that failures are fatal.
• pom.xml contains pre-existing duplicate declarations (gson, slf4j-simple, junit, mockito-core each declared twice, and mockito.version defined twice). These were left untouched to keep the diff scoped; a follow-up issue has been filed.
• Jacoco 0.8.8 cannot instrument class-file major version 65, so local runs on a JDK 21 host emit instrumentation warnings. CI builds on JDK 8 and is unaffected.

Deferred this cycle

Every other open issue is deferred, with reasons: #82 (CONTRIBUTING.md points at a non-existent develop branch) is unrelated to test infrastructure and is left for a docs-scoped cycle; #47/#77 (Discord webhooks) is an in-flight draft PR being iterated between the repository owner and another agent, so it was not adopted or closed; #54, #45, #43, #41, #40, #39 are feature work exceeding a polish-sized PR; #13 is a question, not a work item.

Closes #80
Closes #81
Closes #84

This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

[dmccoystephenson]

Self-review rubric

Scored adversarially against the diff and command output on head 4fb4657, not from judgment.

Universal items

• Scope — PASS. Five files: pom.xml, TopRecordsAlgorithmTest.java, CHANGELOG.md, test_runner_demo.md. Each maps to one of the three Closes issues or to the documentation check. No unrelated formatting or renames were introduced; the pre-existing duplicate dependency declarations in pom.xml were deliberately left untouched rather than cleaned up in passing.
• Tests-new — PASS (not applicable). No production code is added by this PR. The new test helpers (createCountingRecords, CountingScorable) are exercised by the rewritten testGetTopRecords_ComplexityValidation.
• Tests-fix — PASS, confirmed empirically for all three issues, not by reasoning.
  • TopRecordsAlgorithmTest.testGetTopRecords_ComplexityValidation is timing-flaky #81: Collections.sort was temporarily replaced with a selection sort. The rewritten assertion failed — Comparison count suggests worse than O(n log n) for n=100: 4950 vs bound 664. Reverting restored green. The old wall-clock assertion, by contrast, was observed failing on unmodified main at the start of this session.
  • JUnit 4 test classes are never executed by Maven Surefire #80 and The ci-test profile makes test failures non-fatal in CI #84: a sentinel failing assertion was added to LeaderboardEntryTest (JUnit 4) and CI=true mvn -B test was run against both states. Before (this PR's pom.xml stashed): Tests run: 47, Failures: 0 and BUILD SUCCESS — the sentinel was never executed. After: Tests run: 126, Failures: 1 and BUILD FAILURE. The sentinel was then reverted.
• Sibling structure — PASS. createCountingRecords was placed in the existing Helper Methods section immediately after createMockRecordsWithSpecificScores, and CountingScorable immediately before MockScorable, both with Javadoc in the same style as their neighbours.
• Sibling renames — PASS. The parallel pair expectedTimeRatio / actualTimeRatio was renamed to expectedRatio / actualRatio in the same commit, along with times → comparisons.
• Docs — PASS after a fix. The first Phase 7 pass found drift and it was corrected in 4fb4657; the phase was then restarted and a complete second pass found nothing further. See the finding below.
• Issue resolution — PASS. JUnit 4 test classes are never executed by Maven Surefire #80: junit-vintage-engine is declared test-scoped. TopRecordsAlgorithmTest.testGetTopRecords_ComplexityValidation is timing-flaky #81: the wall-clock assertion is gone from the named test. The ci-test profile makes test failures non-fatal in CI #84: maven.test.failure.ignore is removed from the ci-test profile. None is partially resolved.
• CI — PASS. build and test both pass on head 4fb4657. The Build log confirms Tests run: 125, Failures: 0, Errors: 0, Skipped: 0 on JDK 8, so the vintage engine and the new deterministic assertion are verified on the target JDK, not only on the JDK 21 development host.

Repo-specific items

• REST API doc parity — PASS (not applicable). No HTTP endpoint is added or changed, so REST_API.md and openapi.yaml are correctly untouched.
• Algorithm CI scope honored — PASS with a caveat. TopRecordsAlgorithm.java is not modified by this PR, so Simple CI's standalone assertions still exercise exactly the same code path they did before. Those assertions were re-read rather than assumed; see the finding below.

Findings

Three observations are recorded here rather than as inline comments, since they concern lines outside this PR's diff hunks.

1. .github/workflows/simple-ci.yml — the PerformanceTest step asserts duration < 100, a wall-clock bound of exactly the kind TopRecordsAlgorithmTest.testGetTopRecords_ComplexityValidation is timing-flaky #81 was filed about, sitting inside the anchor workflow itself. Its margin is large in practice, but it is a latent flake and the file is on the do-not-auto-merge list, so it was deliberately not touched here. A follow-up issue is being filed.
2. src/test/java/dansplugins/activitytracker/objects/ActivityRecordTest.java and data/PersistentDataTest.java — the newly-executed tests contain eight Thread.sleep(1000) calls, adding roughly 8–10 seconds of wall clock to every build. The surrounding assertions were checked and are directional (> 0, < 6.0) rather than exact, so they are not expected to flake under runner load; the cost is noted for awareness. Those files were also confirmed to perform no real file or network I/O, so enabling them introduces no side effects on the runner.
3. pom.xml — the offline-test profile still sets maven.test.failure.ignore. It is opt-in (-P offline-test) rather than environment-activated, so it does not affect CI, and The ci-test profile makes test failures non-fatal in CI #84 explicitly recommended leaving it; it is flagged only so the remaining occurrence is not mistaken for an oversight.

Documentation accuracy check (Phase 7)

The first pass found that test_runner_demo.md described the complexity check as O(n log n) time complexity verification — a description this PR invalidates — and claimed All 15 test methods should pass against a class that now has 24 methods and 33 executions. Both were corrected in 4fb4657 and the phase was restarted. The second pass over README.md, REST_API.md, openapi.yaml, CONFIG.md, COMMANDS.md, USER_GUIDE.md and CONTRIBUTING.md found nothing further: the Java 8 badge still matches <java.version>1.8</java.version>, and no endpoint, command or config surface is changed by this PR.

Merge recommendation

pom.xml is on this loop's do-not-auto-merge list, because dependency changes affect the shaded release artifact. The added dependency is test-scoped and therefore not expected to reach the shipped JAR, and mvn clean verify (which runs shade) passed in CI. Autonomous merging is nonetheless withheld and a human decision is requested.

This review was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).