Skip to content

test(config): add reference.conf to bean parity gate#6803

Open
bladehan1 wants to merge 1 commit into
tronprotocol:release_v4.8.2from
bladehan1:feat/reference_test
Open

test(config): add reference.conf to bean parity gate#6803
bladehan1 wants to merge 1 commit into
tronprotocol:release_v4.8.2from
bladehan1:feat/reference_test

Conversation

@bladehan1
Copy link
Copy Markdown
Collaborator

What does this PR do?

Adds a single build-time parity gate between reference.conf and the *Config beans bound by ConfigBeanFactory. Two new files under common/src/test/java/org/tron/core/config/args/:

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

Five 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.
  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).

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

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."

This PR has been tested by:

  • Unit Tests:
    • 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
    • Full :common:test — 108 tests across 12 suites green, including all per-section *ConfigTest behavioural tests alongside the new parity gate.
    • Helper meta-tests — 37 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 this team CI gate dependency-free; a runner copies them in, executes Gradle, and tears down. Matrix:
      • assertNoHoconOrphans — 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 — happy path, property without key, level-2 bean orphan via recursion, shape mismatch.
      • assertDefaultValuesMatch — happy path, per-scalar drift (int / long / boolean / double / float / String / List), allowlist hit, nested recursion happy + drift, shape-mismatch guard (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 — 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 — config/checkstyle/checkStyleAll.xml passes on both new files, 0 violations.
  • Manual Testing — N/A (test-only change; no runtime code touched).

Follow up

  • 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.

Extra details

Design tradeoffs — five gate ideas were considered. We deliberately ship only the four shipped here and reject / defer 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 above.
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 behavioural 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 parity 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], [parity-sweep] 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.

Allowlists preserved (do not extend):

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

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.
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