Skip to content

Conversation

@peter-lawrey
Copy link
Member

@peter-lawrey peter-lawrey commented Oct 28, 2025

This PR brings Chronicle-Test-Framework in line with current Chronicle engineering standards. It introduces an opt-in -P code-review quality gate, updates contributor guidance to ISO-8859-1 text policy, moves project docs under src/main/docs/, and makes a series of small but meaningful robustness and hygiene fixes across the codebase. No public APIs are changed.


Why

  • Consistent quality bar across OpenHFT repos (Checkstyle/SpotBugs/PMD/JaCoCo with shared rules).
  • Clearer contributor guidance (ISO-8859-1 text policy; corrected doc links).
  • More robust test helpers (safer logging, locale-safe formatting, correct stream usage).
  • Housekeeping (copyright years, remove stale backup).

What changed

1) Contributor docs

  • AGENTS.md

    • Switch language policy from ASCII-7ISO-8859-1 (retain “no smart quotes / NBSP” guidance).
    • Update references to src/main/docs/decision-log.adoc and src/main/docs/project-requirements.adoc.
  • README.adoc

    • New “Quality Checks” section with mvn -q -P-code-review verify usage.
    • Minor formatting clean-ups (consistent comment spacing; wrapped long lines).

2) Build / Quality gate

  • New opt-in profile code-review (kept off by default):

    • maven-checkstyle-plugin 3.6.0 + com.puppycrawl.tools:checkstyle 8.45.1 with shared net/openhft/quality/checkstyle/checkstyle.xml.
    • spotbugs-maven-plugin 4.8.6.6 + com.h3xstream:findsecbugs-plugin 1.14.0; repository-local src/main/config/spotbugs-exclude.xml scaffold.
    • maven-pmd-plugin 3.28.0 with src/main/config/pmd-exclude.properties.
    • jacoco-maven-plugin 0.8.14 wired for prepare-agent, report, and check with project thresholds (line 0.846, branch 0.707) in default build; same goals run under code-review profile.
    • Add com.github.spotbugs:spotbugs-annotations (provided) to enable precise suppressions.
  • Remove obsolete pom.xml.versionsBackup.

3) Documentation layout

  • Move src/main/adoc/project-requirements.adocsrc/main/docs/project-requirements.adoc (no content changes).

4) Code hygiene & robustness (no API surface changes)

  • Log sanitisation to avoid CR/LF injection in test utilities:

    • VanillaExceptionTracker, CodeStructureVerifier, ProxyConnection, TcpProxy, MappedFileUtil, InternalJavaProcessBuilder: strip \r/\n from values used in log messages.
  • Locale-stable formatting & casing:

    • Use Locale.ROOT in Standard*Accumulator formatting; replace %n / explicit padding; use Locale.ROOT for case transforms where relevant.
  • Correct stream usage & diagnostics:

    • InternalJavaProcessBuilder#getProcessStdOut now reads from getInputStream() (was incorrectly reading getErrorStream()).
    • Safer ProcessBuilder invocation with pre-split args (no shell expansion) and improved logging of child process output.
  • Static-analysis suppressions where intentional:

    • Add targeted @SuppressFBWarnings and PMD suppressions with justifications (e.g. deliberate System.gc() in test utilities; exposing provided maps for test tracking; path parsing in ArchUnit integration).
  • Minor clean-ups / correctness nits:

    • Fix lambda parentheses in ProductUtil to avoid PMD false positives.
    • Adjust SeriesUtil#isPrime cast order.
    • Use @since 2006-2025 copyright headers.
    • Small readability tweaks (padRight helpers, clearer local variables, remove dead locals, tighten visibilities, use try-with-resources).

How to use

# Normal CI-equivalent build (unchanged)
mvn -q verify

# Run pre-merge quality gate locally (strongly recommended)
mvn -q -Pcode-review verify

Compatibility & Impact

  • Source/ABI: No public API changes; all modifications are internal or documentation-only.
  • Build: New profile is opt-in; default mvn verify still works. Adds a provided SpotBugs annotations dep (no runtime impact).
  • Text policy: The ISO-8859-1 guidance affects documentation and contributor content. Source code continues to enforce ASCII in string literals per our Checkstyle rules.

Verification

  • Full mvn -q -Pcode-review verify green locally (unit tests, Checkstyle, PMD, SpotBugs+FindSecBugs, JaCoCo thresholds).
  • Smoke-tested InternalJavaProcessBuilder logging improvements.
  • Confirmed moved docs are referenced correctly from AGENTS.md.

Risks & Mitigations

  • Stricter checks may fail downstream forks. The code-review profile is optional; downstreams can opt-in when ready.
  • Policy mismatch (ISO-8859-1 vs ASCII source). Intentionally kept; we’ll clarify in a follow-up doc that source files remain ASCII-only while prose may use Latin-1.

Follow-ups

  • Populate src/main/config/spotbugs-exclude.xml with targeted, justified entries as needed.
  • Extend docs with a minimal “Getting Started with code-review” section, and a short note on encoding policy for docs vs source.

- Root cause: module lacked the shared Chronicle quality tooling and kept docs under src/main/adoc.

- Fix: add the code-review Maven profile, seed SpotBugs/PMD config placeholders, and move requirements docs plus README/AGENTS references into src/main/docs.

- Impact: mvn -q verify still passes; run mvn -q -Pcode-review verify on Java 11+ to satisfy the new coverage gates (line >= 0.80, branch >= 0.70).
Remove the base JaCoCo prepare-agent execution so only the profile attaches the agent, preventing duplicate instrumentation when running mvn -Pcode-review verify.
Set the shared spotbugs.version property to 4.8.6.6 and keep the annotations jar on 4.8.6 so the Java 8 build can resolve it.
Raise the line threshold to 0.846 and branch to 0.707 so the code-review profile enforces the module's current coverage baseline.
Sanitize logging, tighten GC and socket suppressions, and adjust utilities so the code-review profile passes on Java 21 without static-analysis regressions.
Cast to Buffer before calling clear/flip so the bytecode links against Java 8 APIs, restoring mvn verify on the legacy runtime.
@sonarqubecloud
Copy link

@peter-lawrey peter-lawrey changed the title Adv/code review Adopt code-review profile, align contributor policy to ISO-8859-1, harden test utilities, and tidy docs Oct 30, 2025
@peter-lawrey peter-lawrey marked this pull request as draft November 3, 2025 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants