Skip to content

feat(config,ci): add reference.conf to bean parity gate test#31

Closed
bladehan1 wants to merge 1 commit into
release_v4.8.2from
feat/referece_test
Closed

feat(config,ci): add reference.conf to bean parity gate test#31
bladehan1 wants to merge 1 commit into
release_v4.8.2from
feat/referece_test

Conversation

@bladehan1
Copy link
Copy Markdown
Owner

@bladehan1 bladehan1 commented May 26, 2026

What does this PR do?

Adds a single build-time parity gate between reference.conf and the *Config beans bound by ConfigBeanFactory. The gate lives in common/src/test/java/org/tron/core/config/args/ and consists of two files:

  • ConfigParityGateTest — JUnit class holding the Section registry (path → bean → per-section allowlists) and three test methods that iterate every registered section.
  • ConfigParityCheck — shared helpers: recursive HOCON / bean walkers, scalar type dispatcher, allowlist plumbing, and cross-section aggregate logging.

Four sub-gates run for every entry in the registry:

  1. hoconKeysAreBound — every HOCON key under <section>.* has a writable bean property (recursing into nested *Config beans) or is on the per-section HOCON-orphan allowlist with an inline rationale.
  2. beanPropertiesHaveHoconKeys — every writable bean property has a matching HOCON key (recursing the same way) or is on the per-section bean-orphan allowlist.
  3. defaultValuesMatch — every supported-type bean property's default value equals the reference.conf value. Nested *Config beans are recursed into; types outside the scalar matrix hard-fail with no allowlist escape (see "Design tradeoffs" below).
  4. allowlistEntriesAreLive — every per-section allowlist entry (hoconOrphans / beanOrphans / divergent) still resolves to a live HOCON path or writable bean property. Catches the rot mode where a key/property is renamed or removed but the grandfathering entry is left behind — without this gate, the rationale comment would silently outlive the underlying anomaly (the long-term failure mode of Cassandra-style PROPERTIES_TO_IGNORE blacklists).

A meta-gate (everyReferenceConfTopLevelKeyIsCovered) ensures every top-level reference.conf key is either covered by a registered Section or explicitly listed in TOP_LEVEL_NON_BEAN with a rationale (crypto, enery, localwitness, net, seed, trx).

Why are these changes required?

ConfigBeanFactory.create(...) silently drops HOCON keys that have no matching setter, and silently retains bean defaults that diverge from reference.conf. A unit-test gate is the cheapest place to catch this drift at PR time rather than at process startup (or worse, in production).

The gate's class Javadoc also documents two naming conventions required for new keys to bind without a normalizeNonStandardKeys() hook:

  • Pure lowerCamelCase — treat acronyms (PBFT, HTTP, JSON, RPC, …) as words: allowPbft, not allowPBFT; httpUrl, not HTTPURL. Avoids the "first two characters preserved on decapitalize" trap that produced pBFTExpireNum.
  • Avoid isXxx boolean keys — prefer enableXxx or xxxSwitch. The is prefix forces a manual rename hook.

The v4.8.2 baseline (acronym casing, isOpenFullTcpDisconnect, event.subscribe.native/topics) is grandfathered via per-section allowlists with inline rationale; the policy is "do not extend without explicit reason."

Design tradeoffs

Five gate ideas were considered. We deliberately ship only Gate 2 and rejected the rest:

# Idea Decision Reason
2 Bean ↔ key parity + default-value drift (this PR) Ship The drift modes here are silent and have already produced operator-visible incidents. The check is cheap (reflection + Config.get*) and the contract surface is small and explicit.
3a applyXxxConfig assigns every bean field to Parameter Reject Business logic — eyeballable at review time. AST-pattern-matching this is form-fitting, not value-adding.
3b Every getXxx has a production consumer Reject Equivalent to unused-symbol lint, already handled by IDE/SpotBugs/jdtls. Periodic dead-code cleanup, not a gate.
4 Inline comment coverage on leaf HOCON keys Deferred (follow-up) Gating today would force vacuous one-liners — reference.conf is commented by logical block, not per-leaf, so a coverage gate over the current file is noise. Plan is to first complete the inline comments (every leaf key gets a one-line description of intent / units / interaction), then land the gate. Tracked under Follow up below.
5 Every bean field is referenced by at least one assertion in *ConfigTest Reject Measures test shape, not test value. Would catalyze assertEquals(x, x) boilerplate. Coverage belongs to JaCoCo + review.

Scope discipline that fell out of this:

  • The gate validates the contract layer (key exists, field exists, default matches). It does not validate the behavior layer (the bound value flows to the right downstream consumer). That stays with *ConfigTest behavioral cases (alias fallback, clamp, fromConfig outputs) plus integration tests.
  • Manual-read configs (MiscConfig, LocalWitnessConfig) are intentionally out of scope. Their HOCON paths are scattered top-level keys with no 1:1 bean field, so any "parity gate" would mean asserting a hand-written mapping that duplicates the very thing it's supposed to protect, and could not catch the actual failure mode (a new manual-read key being added without sync). TOP_LEVEL_NON_BEAN accounts for them as "explicitly out-of-scope with rationale" instead.
  • Unsupported scalar types (Map, enum, Duration, MemorySize, custom classes) are a hard failure today rather than an allowlistable skip. None of the current *Config beans use such types; the comment in Section documents the path for re-introducing a typeSkip field if a future bean genuinely cannot be value-compared.

Implementation notes

  • Registry-driven: one Section row per (path, bean, hoconOrphans, beanOrphans, divergent). New *Config beans add a row here — no per-section test file.
  • Recursion: descends into a child *Config bean iff the property's Java type has a default constructor, lives in org.tron.*, and the HOCON value is an OBJECT. Lists / primitives / JDK types never recurse.
  • Logging: emits [parity-hocon], [parity-bean], [parity-default] per section, then a final [parity-summary] block with the file-coverage equation file-hoconKey = checkSection + cantCheckSection and the bean-coverage triple-equality registry = parity-bean = parity-default. When the gate runs without booting a tron main, the lines land in common/build/test-results/test/TEST-...ConfigParityGateTest.xml under <system-out>; once a main has touched logback in the same fork, they route to logs/ on disk — class Javadoc spells this out.

Scope (registered sections)

block, committee, event.subscribe, genesis.block, node, node.metrics, rate.limiter, storage, vm.

Allowlists preserved (do not extend):

  • HOCON orphans: committee.{allowPBFT,pBFTExpireNum}, node.{isOpenFullTcpDisconnect,metrics}, event.subscribe.{native,topics}.
  • Bean orphans: committee.{allowPbft,pbftExpireNum}, node.openFullTcpDisconnect, event.subscribe.nativeQueue.
  • Divergent defaults: genesis.block.{timestamp,parentHash,assets,witnesses}, node.fastForward, event.subscribe.filter.{contractAddress,contractTopic}.

This PR has been tested by:

  • End-to-end gate./gradlew :common:test --tests "*ConfigParityGateTest*" → all five sub-gates green (hoconKeysAreBound, beanPropertiesHaveHoconKeys, defaultValuesMatch, allowlistEntriesAreLive, everyReferenceConfTopLevelKeyIsCovered). The [parity-summary] block verifies the two coverage invariants:
    • HOCON alignment: file-hoconKey = checkSection + cantCheckSection
    • Bean alignment: registry-beanKey = parity-bean = parity-default
  • Helper meta-tests — 41 cases covering every branch of ConfigParityCheck's four assertion helpers (one fixture .conf + one synthetic *Config-shaped bean per branch). Sources live outside src/ to keep the team CI gate dependency-free; a runner copies them in, executes Gradle, and tears down. Matrix:
    • assertNoHoconOrphans (6) — happy path, top-level orphan, allowlist hit, level-2 orphan via recursion, shape mismatch (child is scalar), recursion-filter rejects non-org.tron.* type.
    • assertNoBeanOrphans (4) — happy path, property without key, level-2 bean orphan via recursion, shape mismatch.
    • assertDefaultValuesMatch (15) — happy path, per-scalar drift (int / long / boolean / double / float / String / List), allowlist hit, nested recursion happy + drift, shape-mismatch guard (the new branch this PR adds — recursing into a non-OBJECT HOCON value previously threw ConfigException.WrongType; now surfaces a clean field-pointing mismatch), unsupported Java type (Map), write-only property (setter, no getter), null nested field, type-coerce incompatible.
    • assertAllowlistEntriesAreLive (6) — every-entry-resolves, dead hoconOrphan, dead beanOrphan, divergent dead on hocon side, divergent dead on bean side, nested dotted ok + nested dotted bean-side dead (exercises beanPropertyExists' dotted-recursion path).
  • Checkstyle — verified the new files pass under config/checkstyle/checkStyleAll.xml.
  • Manual Testing — N/A (test-only change; no runtime code touched).

Follow up

Optional, not blocking this PR:

  • If a future *Config bean introduces a Map/enum/Duration/MemorySize field, re-introduce a Section.typeSkip field and the corresponding assertDefaultValuesMatch parameter following the comment block in Section.
  • Inline comment coverage gate on leaf HOCON keys (planned). Two ordered steps: (a) complete the inline // ... comments on reference.conf so every leaf key carries a one-line description of intent / units / interaction; (b) add a sub-gate that fails when a registered-section leaf key has no preceding comment. Step (b) is held until step (a) lands — gating an under-commented file would force vacuous one-liners and defeat the point.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 26, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: be2b1da7-2d14-4baf-8625-54fda0b0f227

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/referece_test

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 12 files

Re-trigger cubic

Copy link
Copy Markdown
Owner Author

@bladehan1 bladehan1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by AI pipeline (see .review-system/tasks/PR31 for full artifacts).

Decision: Approve
Findings: P0=0, P1=0, P2=1, nit=1

NOTE: This review was generated by Claude Code's /review pipeline. Comments are AI suggestions; human reviewers retain final judgment.

Comment thread common/src/test/java/org/tron/core/config/args/ConfigParityCheck.java Outdated
Comment thread common/src/test/java/org/tron/core/config/args/MiscConfigTest.java Outdated
Copy link
Copy Markdown
Owner Author

@bladehan1 bladehan1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated re-review by AI pipeline (PR version updated).

Decision: Approve
Findings: P0=0, P1=1, P2=1, nit=1

Key Update: The review now includes Gate 2b (Default Value Matching). One P1 finding regarding type boundary documentation was added.

NOTE: This review was generated by Claude Code's /review pipeline.

Comment thread common/src/test/java/org/tron/core/config/args/ConfigParityCheck.java Outdated
Comment thread common/src/test/java/org/tron/core/config/args/MiscConfigTest.java Outdated
@bladehan1 bladehan1 force-pushed the feat/referece_test branch from fde558c to a54bdcd Compare May 28, 2026 02:15
@github-actions
Copy link
Copy Markdown

❌ Math Usage Detection Results

Found forbidden usage of java.lang.Math in the following files:

./common/src/test/java/org/tron/core/config/args/ConfigParityCheck.java

Please review if this usage is intended.

Caution

Note: You should use org.tron.common.math.StrictMathWrapper.
If you need to use java.lang.Math, please provide a justification.

@bladehan1 bladehan1 force-pushed the feat/referece_test branch from a54bdcd to c164f4b Compare May 28, 2026 02:22
@github-actions
Copy link
Copy Markdown

❌ Math Usage Detection Results

Found forbidden usage of java.lang.Math in the following files:

./common/src/test/java/org/tron/core/config/args/ConfigParityCheck.java

Please review if this usage is intended.

Caution

Note: You should use org.tron.common.math.StrictMathWrapper.
If you need to use java.lang.Math, please provide a justification.

@bladehan1 bladehan1 changed the title test(config): add reference.conf <-> bean parity tests (Gate 2) test(config): add reference.conf to bean parity gate test May 28, 2026
@bladehan1 bladehan1 force-pushed the feat/referece_test branch 2 times, most recently from 8ba0599 to c69e256 Compare May 28, 2026 03:38
@bladehan1 bladehan1 changed the title test(config): add reference.conf to bean parity gate test feat(config): add reference.conf to bean parity gate test May 28, 2026
Copy link
Copy Markdown
Owner Author

@bladehan1 bladehan1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by AI pipeline (see .review-system/tasks/PR31 for full artifacts).

Decision: Approve
Findings: P0=0, P1=2, P2=0, nit=0

NOTE: This review was generated by the /review pipeline. Comments are AI suggestions; human reviewers retain final judgment.

Copy link
Copy Markdown
Owner Author

@bladehan1 bladehan1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated corrected review by AI pipeline (see .review-system/tasks/PR31 for full artifacts).

Correction: previous review was invalidated because origin/release_v4.8.2 was not synced with upstream/release_v4.8.2. This review uses upstream/release_v4.8.2...origin/feat/referece_test.

Decision: Approve
Findings: P0=0, P1=1, P2=0, nit=0

NOTE: Comments are AI suggestions; human reviewers retain final judgment.

checkSectionTopLevels, TOP_LEVEL_NON_BEAN);
}

@Test
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SHOULD] Run the allowlist liveness sweep from the gate

ConfigParityCheck.assertAllowlistEntriesAreLive(...) is implemented to catch stale hoconOrphans, beanOrphans, and divergent entries, but the gate only runs the orphan/default/top-level checks. As written, a future key or bean property can disappear while the old allowlist entry and rationale comment stay behind, and this test class will still pass.

Suggestion: Add a fourth gate test that iterates SECTIONS and calls assertAllowlistEntriesAreLive(...) for each section.

@bladehan1 bladehan1 force-pushed the feat/referece_test branch from 3675673 to f9351d8 Compare May 29, 2026 04:12
@bladehan1 bladehan1 changed the title feat(config): add reference.conf to bean parity gate test feat(config,ci): add reference.conf to bean parity gate test May 29, 2026
Add ConfigParityGateTest as the single build-time gate validating that
reference.conf and its *Config beans stay in lockstep. Five sub-gates
run against every entry in SECTIONS:

  1) hoconKeysAreBound — every HOCON key has a matching writable bean
     property or is in the per-section HOCON-orphan allowlist with a
     rationale comment.
  2) beanPropertiesHaveHoconKeys — every writable bean property has a
     matching HOCON key or is in the per-section bean-orphan allowlist.
  3) defaultValuesMatch — every supported-type bean property has a
     default value equal to the reference.conf value; types outside the
     dispatcher matrix are a hard failure (no silent escape).
  4) allowlistEntriesAreLive — every per-section allowlist entry still
     resolves to a live HOCON path or writable bean property; prevents
     allowlist rot when a key is renamed or removed.
  5) everyReferenceConfTopLevelKeyIsCovered — meta-gate closing the
     "new top-level section sneaks in" hole.

ConfigParityCheck holds the shared recursive walkers, type dispatcher,
shape-mismatch guard (a nested *Config property must see a HOCON
OBJECT), and per-section + cross-section accounting. Default-value
mismatch messages stamp both sides with the runtime type so that an
Integer(10) vs Long(10) divergence is no longer visually identical.

Scope: gates validate only the SECTIONS bucket. Top-level keys outside
SECTIONS (crypto, enery, localwitness, net, seed, trx) are counted for
file-coverage alignment but their subtrees are not value-validated —
they hang off MiscConfig manual-read paths or non-bean roots.

Each helper provides a (label, Config, ...) overload that walks a
caller-supplied Config without bumping the production AGGREGATES, so
unit tests of the gate itself stay isolated from the gate's own
coverage totals.
@bladehan1 bladehan1 force-pushed the feat/referece_test branch from f9351d8 to c022f3a Compare May 29, 2026 04:33
@bladehan1 bladehan1 closed this May 29, 2026
@bladehan1 bladehan1 deleted the feat/referece_test branch May 29, 2026 06:15
@bladehan1 bladehan1 restored the feat/referece_test branch May 29, 2026 06:16
@bladehan1 bladehan1 deleted the feat/referece_test branch May 29, 2026 06:16
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.

1 participant