JanssenProject / JanssenProject/jans

feat(jans-fido2): add unit tests for Fido2UserMetrics computed logic

Open
#14,133 1 comment 1 reaction 1 assignee Claimed by @imran-ishaq View on GitHub
comp-jans-fido2 enhancement kind-feature
Dominant language
Java
Stars
648
Forks
174
Avg merge
1d 18h
Merged PRs (30d)
110

Description

**Is your feature request related to a problem? Please describe.**
`Fido2UserMetrics` (`jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2UserMetrics.java`) is the per-user ORM entity backing every FIDO2 user-adoption metric stored under `ou=fido2-user-metrics,o=jans`. Unlike the earlier DTOs, this class carries real computed logic that downstream services and the REST analytics endpoints depend on:

- `incrementRegistrations(boolean)`, `incrementAuthentications(boolean)`, `incrementFallbackEvents()` — counter mutations with side effects on `lastActivityDate`/`lastUpdated`.
- `getRegistrationSuccessRate()`, `getAuthenticationSuccessRate()`, `getOverallSuccessRate()`, `getFallbackRate()` — null-safe ratio computations that must protect against divide-by-zero (otherwise `Double.NaN` / `Double.POSITIVE_INFINITY` leak into stored aggregates and JSON responses).
- `updateEngagementLevel()` — threshold-based bucketing into `HIGH` (≥50 ops), `MEDIUM` (≥10), `LOW` (otherwise). Off-by-one mistakes at the boundaries silently re-bucket every user.
- `updateAdoptionStage()` — threshold-based bucketing into `NEW` (0 regs), `LEARNING` (1), `ADOPTED` (2–5), `EXPERT` (>5).
- `isNewUser()`, `isActiveUser()` — 30-day window logic anchored to `System.currentTimeMillis()`, with null-date guards.
- Custom `equals(Object)` / `hashCode()` based on the (`userId`, `username`, `firstRegistrationDate`) triple — used by collections/dedup in `Fido2UserMetricsService`.
- Two constructors with non-trivial initialization: the no-arg constructor zeroes counters, sets `isActive=true`, and stamps `lastUpdated`; the 2-arg constructor additionally assigns a random `UUID` to `id`.

None of this is tested today. This is the first issue in the rollout that exercises real computation rather than getter/setter wiring, and the per-method consequences of a silent regression are large (corrupted stored aggregates, mis-bucketed engagement reports, NaN leakage through the JSON API).

This is step 5 of the FIDO2 metrics test rollout (step 1 added `Fido2MetricTypeTest`; step 2 added `Fido2MetricsConstantsTest`; step 3 added `Fido2MetricsDataTest`; step 4 added `UserMetricsUpdateRequestTest`).

**Describe the solution you'd like**
Add a new JUnit 5 test class:

- File: `jans-fido2/model/src/test/java/io/jans/fido2/model/metric/Fido2UserMetricsTest.java`

The test should cover:

- **Default constructor initial state:** all six counters (`totalRegistrations`, `totalAuthentications`, `successfulRegistrations`, `successfulAuthentications`, `failedRegistrations`, `failedAuthentications`, `fallbackEvents`) are `0`; `isActive` is `true`; `lastUpdated` is non-null and within ±1s of `System.currentTimeMillis()` at construction.
- **2-arg constructor:** `new Fido2UserMetrics("u-1", "alice")` sets `userId`/`username` correctly, generates a non-null `id` parseable as a UUID, and inherits the default-constructor initialization (counters zeroed, `isActive=true`).
- **`incrementRegistrations(true)`:** `totalRegistrations` goes 0→1, `successfulRegistrations` goes 0→1, `failedRegistrations` stays at 0, `lastActivityDate` and `lastUpdated` are both non-null and ~now.
- **`incrementRegistrations(false)`:** mirrors the above but increments `failedRegistrations`, not `successfulRegistrations`.
- **`incrementAuthentications(true/false)`:** symmetric to the registration tests, on the authentication counters.
- **`incrementFallbackEvents()`:** `fallbackEvents` 0→1, plus timestamp side effects.
- **`getRegistrationSuccessRate()` happy path:** with `totalRegistrations=10`, `successfulRegistrations=7`, returns `0.7` (delta `1e-9`).
- **`getRegistrationSuccessRate()` zero-and-null guards:** returns `0.0` (not `NaN`) when `totalRegistrations` is `null`, and also when it is `0`.
- **`getAuthenticationSuccessRate()` happy path + guards:** parallel to the registration cases.
- **`getOverallSuccessRate()` cross-counter math:** with `totalRegistrations=4`, `successfulRegistrations=2`, `totalAuthentications=6`, `successfulAuthentications=3`, returns `5/10 = 0.5`. With both totals null/zero, returns `0.0`. With one side null, treats it as `0` and computes from the other side.
- **`getFallbackRate()`:** happy path with mixed counters; `0.0` when both totals are zero/null; null `fallbackEvents` is treated as `0` (not NPE).
- **`updateEngagementLevel()` boundary check at 50:** totals 49 → `MEDIUM`, totals 50 → `HIGH`. Catches `>` vs `>=` regressions on the high threshold.
- **`updateEngagementLevel()` boundary check at 10:** totals 9 → `LOW`, totals 10 → `MEDIUM`.
- **`updateEngagementLevel()` null totals:** returns `LOW` (the null-safe branch must treat null as 0).
- **`updateAdoptionStage()` boundaries:** 0 regs → `NEW`, 1 → `LEARNING`, 5 → `ADOPTED`, 6 → `EXPERT`; `null` → `NEW`.
- **`isNewUser()`:** `true` when `firstRegistrationDate` is 29 days ago, `false` when 31 days ago, `false` when `null`. Use a fixed `Date(System.currentTimeMillis() - Nd)` rather than wallclock comparisons inside the assertion.
- **`isActiveUser()`:** mirrors `isNewUser()` against `lastActivityDate`.
- **`equals()` / `hashCode()` contract:** two instances with identical (`userId`, `username`, `firstRegistrationDate`) are equal and share a hash; differing in any of the three breaks equality; differing in unrelated fields (e.g. counters, `engagementLevel`) preserves equality (the equality is identity-tuple-only by design). Includes reflexive (`x.equals(x)`) and null/wrong-type (`x.equals(null)`, `x.equals("string")`) cases.

Conventions to follow (same as `Fido2MetricTypeTest`, `Fido2MetricsConstantsTest`, `Fido2MetricsDataTest`, `UserMetricsUpdateRequestTest`):
- JUnit 5 (`org.junit.jupiter.api`).
- No Mockito needed — pure entity behavior.
- No additional dependencies — `junit-jupiter-api` and `junit-jupiter-engine` test-scope deps are already declared in `jans-fido2/model/pom.xml`; `jans-orm-model` (for the `Entry` superclass) is already on the model module's compile classpath.
- Group related cases into one `@Test` per behavior (e.g., `testGetOverallSuccessRateHappyPath`, `testGetOverallSuccessRateZeroAndNullGuards`, `testUpdateEngagementLevelBoundary50`, `testEqualsContractForIdentityTuple`).
- For timestamp side-effect assertions, use a small wall-clock window (e.g., capture `before = System.currentTimeMillis()` before the call, `after = System.currentTimeMillis()` after, assert `lastUpdated.getTime()` falls in `[before, after]`). Avoid `Thread.sleep`.

Acceptance criteria:
- [ ] New test file exists at the path above.
- [ ] All behaviors listed are covered.
- [ ] `mvn -pl model -am test` from `jans-fido2/` passes locally.
- [ ] Test class runs in well under 1 second (no `Thread.sleep`, no I/O).
- [ ] No changes to production code under `src/main`.
- [ ] No pom changes required.

**Describe alternatives you've considered**
- Splitting this into smaller sub-issues (one per method group: counters, rates, levels/stages, equals): rejected — all the logic lives in one class with overlapping setup (the counters drive the rates which drive the levels), and splitting would either duplicate the setup or force artificial test-file fragmentation. The class file is one cohesive unit and should be covered as one.
- Skipping boundary tests and only covering happy paths: rejected — the entire risk profile here is off-by-one in the threshold ladders and `NaN` from unguarded division. Happy paths catch neither.
- Using a clock-injection abstraction to make `isNewUser()` / `isActiveUser()` deterministic: rejected as out of scope — the method takes its time from `System.currentTimeMillis()` directly and refactoring that signature is a separate change. The proposed approach (fixed `Date` offsets relative to current time) is robust enough for a unit test.
- Asserting on exact `lastUpdated == new Date()` equality: rejected — flaky; uses the small-window assertion described above instead.

**Additional context**
Roadmap position: step 5 of the FIDO2 metrics test rollout — and the first step that exercises computed behavior rather than value mapping. Next planned step is to strengthen the existing `MetricServiceTest` (`jans-fido2/server/src/test/java/io/jans/fido2/service/shared/MetricServiceTest.java`), replacing its `assertDoesNotThrow` smoke checks with real `verify()` / `ArgumentCaptor` assertions. After that, the rollout moves into the server-side service classes (`Fido2MetricsService` will be split across several issues — store, query, aggregate, analytics).

Same Maven module (`jans-fido2/model`) as issues #1–#4; with the surefire/JUnit alignment fix already in place, no further pom workarounds are required.

Suggested label: `kind-test`.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.