Skip to content

feat(jans-fido2): add Fido2MetricsAggregation unit tests#14219

Draft
imran-ishaq wants to merge 1 commit into
mainfrom
feat/fido2-metrics-aggregation-unit-tests
Draft

feat(jans-fido2): add Fido2MetricsAggregation unit tests#14219
imran-ishaq wants to merge 1 commit into
mainfrom
feat/fido2-metrics-aggregation-unit-tests

Conversation

@imran-ishaq

@imran-ishaq imran-ishaq commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Prepare


Description

Adds JUnit 5 test coverage for Fido2MetricsAggregation — the ORM entity that stores hourly/daily/weekly/monthly summary rows for the FIDO2 metrics feature. Most of the class is plain getters/setters, but the small computed surface (4-arg constructor id composition, getPeriod() derivation, null-safe Number coercion, Integer→Long map widening, incrementMetric accumulator, and the equals/hashCode contract) was completely untested. The new test class locks each of those behaviors. Test-only change; no production code or pom.xml changes. Step 7 of the FIDO2 metrics test rollout.

Target issue

Fido2MetricsAggregation (jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsAggregation.java) carries six small bits of computed behavior that downstream aggregation/analytics code silently depends on, and none of them is tested today:

  • A 4-arg constructor that builds the composite id as aggregationType + "_" + period and stamps lastUpdated in UTC.
  • getPeriod() — derives the period string back out of id by splitting on the FIRST underscore.
  • getLongMetric(key) / getDoubleMetric(key) — null-safe Number coercion that returns null (not NaN, not NPE) when the underlying map entry is absent or non-numeric.
  • getDeviceTypes() / getErrorCounts() — Map accessors that coerce Integer values to Long for JSON-serialization compatibility (the exact shape Jackson hands back for JSON numbers).
  • incrementMetric(key, increment) — null-safe accumulator (treats both nulls as 0).
  • A custom equals/hashCode based on (id, aggregationType, startTime, endTime), ignoring uniqueUsers and metricsData.

Silent failure modes — wrong aggregation totals, dropped Integer→Long conversions, NPE against partially-loaded ORM entries, or a refactor of getPeriod() that swaps indexOf("") for split("") or lastIndexOf("_") — would corrupt every downstream summary without any compile-time signal.

This PR is step 7 of the FIDO2 metrics test rollout.

closes #14220

Implementation Details

Adds a single new JUnit 5 test class — jans-fido2/model/src/test/java/io/jans/fido2/model/metric/Fido2MetricsAggregationTest.java — with 12 grouped @test methods, one per spec'd behavior group:

Behavior Test method
Default ctor — metricsData is non-null empty map, other fields null testDefaultConstructorInitializesEmptyMetricsData
4-arg ctor — composes id, wires aggregationType/startTime/endTime, stamps lastUpdated within ±1s of now, preserves default-ctor invariant via this() testFourArgConstructorComposesIdAndStampsLastUpdated
getPeriod() — single-underscore, multi-underscore (split on FIRST _), no-underscore, null-id testGetPeriodDerivesFromIdOnFirstUnderscore
getLongMetric / getDoubleMetric — null metricsData and absent key both return null testGetLongMetricAndGetDoubleMetricNullSafety
getLongMetric — Long pass-through, Integer widened to Long, Double truncated toward zero testGetLongMetricReturnsValueAndWidensIntegerToLong
getDoubleMetric — Double pass-through, Integer accepted via Number path testGetDoubleMetricReturnsValueForIntegerAndDouble
getLongMetric / getDoubleMetric — String value returns null, not ClassCastException testGetLongMetricReturnsNullForNonNumberValue
Convenience setters — setRegistrationAttempts stores under REGISTRATION_ATTEMPTS; setAuthenticationSuccessRate proves the Double path under AUTHENTICATION_SUCCESS_RATE testConvenienceSettersWireToConstantKeys
getDeviceTypes — inner Integer values widened to Long (Jackson-compat) testGetDeviceTypesCoercesIntegerToLong
getDeviceTypes / getErrorCounts — empty map (not null) when the key is missing testGetDeviceTypesReturnsEmptyMapWhenAbsent
incrementMetric — absent→5, 5+3=8, null increment keeps 8, both-null on fresh instance → 0 testIncrementMetricIsNullSafeOnBothSides
equals / hashCode — identity-4-tuple equality, reflexive, null/other-type, each of the 4 fields differing breaks equality, unrelated fields (uniqueUsers, metricsData) preserve it testEqualsAndHashCodeContract

Design choices worth flagging:

  • Multi-underscore id in the getPeriod() test. Asserts "HOURLY_2026-05-22_14" → "2026-05-22_14", not "2026-05-22". This is the load-bearing detail of the implementation (substring after the FIRST underscore) and catches a refactor that switches to split("") or lastIndexOf("") — both compile fine and pass a naive single-underscore test.
  • ±1s tolerance window on lastUpdated rather than exact equality. The test snapshots System.currentTimeMillis() before and after the ctor call and asserts stamped ∈ [before − 1000, after + 1000]. Tight enough to catch a missing stamp; loose enough to absorb scheduling jitter on slow CI.
  • Integer values (not Long) in the getDeviceTypes() Integer→Long coercion test. The whole reason getMapMetric exists is that Jackson hands back JSON numbers as Integer by default; using Integer in the test setup mirrors the actual production code path, not a hand-crafted Long map.
  • assertNotEquals(null, a) AND assertFalse(a.equals(null)) in the equals contract test. JUnit's assertNotEquals can be implemented in ways that don't always exercise a.equals(null) directly; the explicit a.equals(null) call locks the documented Object#equals contract.
  • Sampled (not exhaustive) convenience getter/setter coverage. One Long pair (setRegistrationAttempts) and one Double pair (setAuthenticationSuccessRate) prove the wiring; the remaining ~12 pairs are mechanical wrappers around setMetric(constant, value) and would just be near-identical assertions.

Scope:

  • Test-only addition under src/test.
  • No production-code changes under src/main.
  • No pom.xml changes (the JUnit 5 test-scope dependencies are already declared from earlier rollout steps).

Local verification: mvn -pl model -am test from jans-fido2/ runs the new class alongside the existing two (Fido2MetricsConstantsTest and Fido2UserMetricsRateCalculationsTest). Test class runtime is well under 1 second — no I/O, no Thread.sleep, no async.


Test and Document the changes

  • Static code analysis has been run locally and issues have been fixed

  • Relevant unit and integration tests have been added/updated

  • Relevant documentation has been updated if any (i.e. user guides, installation and configuration guides, technical design docs etc)

  • I confirm that there is no impact on the docs due to the code changes in this PR.

Closes #14220,

Signed-off-by: imran <imranishaq7071@gmail.com>
@imran-ishaq imran-ishaq added this to the 2.3.0 milestone Jun 5, 2026
@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 75023edc-bb49-423d-8f57-d3aa7c209539

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/fido2-metrics-aggregation-unit-tests

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.

@mo-auto

mo-auto commented Jun 5, 2026

Copy link
Copy Markdown
Member

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@mo-auto mo-auto added comp-jans-fido2 Component affected by issue or PR kind-feature Issue or PR is a new feature request labels Jun 5, 2026
@mo-auto

mo-auto commented Jun 5, 2026

Copy link
Copy Markdown
Member

Error: Hi @imran-ishaq, You did not reference an open issue in your PR. I attempted to create an issue for you.
Please update that issues' title and body and make sure I correctly referenced it in the above PRs body.

@sonarqubecloud

sonarqubecloud Bot commented Jun 5, 2026

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp-jans-fido2 Component affected by issue or PR kind-feature Issue or PR is a new feature request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: feat(jans-fido2): add Fido2MetricsAggregation unit tests -autocreated

2 participants