test: isolate entity-graph query-count assertion from parallel-execution pollution

[devondragon]

Summary

Fixes the flaky UserRepositoryEntityGraphTest.shouldLoadRolesAndPrivilegesInBoundedQueryCountViaEntityGraphFinder test reported in #337.

The test read the SessionFactory-global Hibernate Statistics.getPrepareStatementCount() counter to guard against N+1 query regressions. Under JUnit 5 parallel execution, @DataJpaTest classes share a single cached Spring context — and therefore one SessionFactory and one Statistics object. Tests running concurrently on other threads incremented that shared, non-transactional counter between stats.clear() and the assertion read, so the observed count intermittently exceeded the bound (e.g. 7 vs. ≤3) even though the actual per-query behavior was correct.

Changes

• New StatementCountInspector — a Hibernate StatementInspector that records prepared-statement counts in a ThreadLocal. Each parallel test runs on its own thread and JDBC connection, so the count is naturally isolated from concurrent pollution.
• Registered the inspector via spring.jpa.properties.hibernate.session_factory.statement_inspector in application-test.properties (co-located with the effective H2 JPA config).
• Updated the test to StatementCountInspector.reset() before the measured query and assert on the per-thread count.
• The assertion now uses isBetween(1, 3): the upper bound preserves the original N+1 regression guard; the new lower bound fails loudly if the inspector is ever unregistered instead of silently passing at 0.

Testing

• ./gradlew test — full suite green (987 tests, 0 failures).
• Verified the inspector genuinely counts (temporarily tightening the lower bound proved a per-thread count of 1–3, not a silent 0).
• Fix eliminates the flakiness by construction: a ThreadLocal counter cannot be mutated by tests on other threads.

Closes #337

[Copilot]

Pull request overview

This pull request fixes a flaky JPA repository test by replacing a SessionFactory-global Hibernate Statistics counter (polluted under JUnit 5 parallel execution) with a per-thread statement counter implemented via a Hibernate StatementInspector.

Changes:

• Add StatementCountInspector (ThreadLocal-backed) to count prepared statements per test thread.
• Register the inspector in the test Spring Boot configuration (application-test.properties).
• Update UserRepositoryEntityGraphTest to reset/assert against the per-thread counter (isBetween(1, 3)).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/test/resources/application-test.properties	Registers the Hibernate statement_inspector for the test profile.
src/test/java/com/digitalsanctuary/spring/user/test/config/StatementCountInspector.java	Introduces the per-thread statement-counting StatementInspector.
src/test/java/com/digitalsanctuary/spring/user/persistence/repository/UserRepositoryEntityGraphTest.java	Switches the bounded-query assertion to use the per-thread counter instead of global Statistics.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[claude]

Review

Nice, tightly-scoped fix for the flaky UserRepositoryEntityGraphTest test (#337). Root-causing the flakiness to a shared, non-transactional Statistics counter under JUnit 5 parallel execution is a good diagnosis, and switching to a ThreadLocal-backed StatementInspector is a sound, idiomatic fix for this class of problem.

What the PR does

• Adds StatementCountInspector (test-only Hibernate StatementInspector) that counts prepared statements per-thread instead of relying on the SessionFactory-global Statistics object.
• Registers it globally for the test profile via application-test.properties (hibernate.session_factory.statement_inspector).
• Updates the regression test to reset()/getCount() against the new inspector, and tightens the assertion to isBetween(1, 3) instead of isLessThanOrEqualTo(3).

Strengths

• The ThreadLocal approach directly addresses the root cause (cross-thread pollution of a shared counter) rather than papering over it with retries or @Execution(SAME_THREAD).
• Adding a lower bound (isBetween(1, 3)) is a genuinely good addition - it turns "inspector silently unregistered -> count reads 0 -> test passes vacuously" into a hard failure. That is a real improvement over the original assertion, not just a flakiness workaround.
• Serializable is correctly implemented (required by Hibernate for StatementInspector), and the javadoc explains the why behind the design (non-obvious concurrency reasoning), matching this repos comment conventions.
• Test-only code lives under test.config, consistent with the existing test infrastructure package layout.

Minor issues

1. Javadoc references the wrong file. StatementCountInspectors class javadoc says the property is registered "(see application-test.yml)", but it is actually added to application-test.properties. Both files exist and are loaded for the test profile, so this is a small but real doc/code mismatch worth fixing before merge.
2. Global registration side effect. The inspector is registered for the entire test Spring profile (not just this one @DataJpaTest), so every prepared statement across the whole suite now flows through a static method call incrementing a ThreadLocal. This is almost certainly negligible overhead, but it is a global, always-on side effect for the benefit of a single test - worth a one-line comment (or note in the PR description) confirming that was a deliberate tradeoff vs. e.g. scoping it via a test-specific @TestConfiguration/properties override on just UserRepositoryEntityGraphTest.
3. No test for the inspector itself. StatementCountInspector has non-trivial-enough logic (thread-local reset/count semantics) that a small direct unit test (no Spring context needed) would cheaply lock in its behavior independent of the one consumer test. Not a blocker given how simple the class is, but easy to add.
4. Thread reuse across test classes isnt fully ruled out. Since junit-platform.properties uses the dynamic parallel strategy with thread pooling, threads are reused across test classes, not just methods. That is fine here because reset() is called immediately before the measured operation, but it is worth double-checking there is no other place in the suite that reads StatementCountInspector.getCount() without a preceding reset() - otherwise it could pick up leftover counts from a prior test on the same pooled thread.

Security / performance
No security concerns - this is test-only infrastructure and is not shipped as part of the library artifact. Performance impact is negligible (an int increment per prepared statement, test-scope only).

Test coverage
The PR fixes the flaky tests isolation and adds a meaningful lower-bound assertion; overall this improves test coverage/robustness rather than just suppressing the flake. As above, a dedicated unit test for StatementCountInspector would be a nice-to-have but is not required.

Overall: solid, well-reasoned fix. Recommend fixing the .yml/.properties doc reference before merge; the rest are non-blocking suggestions.

[claude]

Review

Overview

Fixes the flaky UserRepositoryEntityGraphTest.shouldLoadRolesAndPrivilegesInBoundedQueryCountViaEntityGraphFinder test (#337) by replacing the shared, SessionFactory-global Hibernate Statistics.getPrepareStatementCount() read with a custom per-thread StatementInspector (StatementCountInspector). Since @DataJpaTest classes share a cached Spring context (and thus one SessionFactory), the old approach was vulnerable to cross-thread pollution under JUnit 5 parallel execution. The new ThreadLocal-backed counter is naturally isolated per test thread/connection. Test-only change (3 files), no production code touched.

Code quality

• Root-cause diagnosis is correct and well explained in the PR description and in-code comments.
• StatementCountInspector is clean, minimal, and well-documented (Javadoc matches the project's convention of documenting public classes/methods).
• Good defensive touch: switching the assertion from isLessThanOrEqualTo(3) to isBetween(1, 3) so a broken/unregistered inspector (which would silently read 0) fails loudly instead of producing a false green.
• Removal of the now-unused Statistics/SessionFactory/EntityManager imports and the statistics() helper is good cleanup.
• Imports remain alphabetical/no-wildcards per project convention.

Potential issues / suggestions

1. Thread-local not cleaned up — COUNT is ThreadLocal.withInitial(...) but nothing ever calls COUNT.remove(). Since JUnit 5 parallel execution reuses a fixed worker thread pool, this isn't a practical leak (one boxed Integer per pool thread, for the life of the JVM), but it's slightly sloppy for a class explicitly designed around per-thread state. Consider remove() in reset() or leave a short comment noting it's intentionally left set for the thread's lifetime.
2. Global registration, single consumer — the inspector is registered for the whole test SessionFactory via application-test.properties, so it now runs for every JPA test (per grep, this properties file backs @DatabaseTest, several @SecurityTest/@IntegrationTest classes, etc.), even though only one test currently reads the counter. This is harmless (a counter increment per prepared statement) and arguably fine for a shared test properties file, but worth a one-line comment noting other tests may pay a negligible, inert overhead — mostly just flagging it so it's a conscious tradeoff rather than a surprise later.
3. Serializable on the inspector — marked Serializable with a serialVersionUID; Hibernate's StatementInspector doesn't require this for the session_factory.statement_inspector property path (it's constructed via reflection, not deserialized), so this appears to be defensive/copy-paste boilerplate. Not harmful, just unnecessary — could be dropped for a slightly smaller surface area, but not worth blocking on.
4. Semantic parity with the old check — worth double-checking (if not already) that StatementInspector.inspect(...) fires at the same granularity as Statistics.getPrepareStatementCount() did (i.e., once per actual JDBC PreparedStatement, not once per cached/reused query plan) so the isBetween(1, 3) bound continues to reflect genuine N+1 behavior rather than an artifact of the new measurement mechanism. The PR description says this was manually verified (count of 1–3 observed), which is reassuring.

Security

No concerns — this is test-only infrastructure, not shipped in the published library artifact (src/test/java), and touches no production/auth code paths.

Test coverage

This is the test fix; no additional coverage needed. The approach (fixing flakiness via a mechanism that's structurally immune to cross-thread pollution, rather than e.g. adding retries or @Isolated) is the right call and should generalize if other tests ever need similar guards.

Overall: solid, well-reasoned fix for a real flakiness root cause. The two items above (remove() on the ThreadLocal, and possibly dropping Serializable) are minor nitpicks, not blockers.

[devondragon]

Thanks for the thorough reviews. Addressed the feedback in 80e0cdd (and the javadoc reference earlier in a4904f6):

Fixed

• Javadoc .yml → .properties (Copilot, claude[bot]): the inspector's javadoc now correctly points at application-test.properties.
• Global-registration tradeoff (claude[bot] ×2): added a comment in application-test.properties noting the profile-wide registration is deliberate and that the per-statement ThreadLocal increment is inert/negligible for tests that never read the counter.
• ThreadLocal lifetime (claude[bot]): added a comment explaining the value is intentionally left set (not remove()d) for the pooled worker thread's lifetime, since every measurement calls reset() first.
• Unit test (claude[bot]): added StatementCountInspectorTest covering reset/count/inspect semantics plus an interleaved two-thread isolation scenario that proves one thread's reset/count cannot perturb another's.

Verified (no change needed)

• No other unreset getCount() reader: UserRepositoryEntityGraphTest is the only consumer and always calls reset() immediately before getCount(), so pooled-thread reuse cannot leak a stale count.
• Measurement parity: inspect() fires per prepared statement, matching the old getPrepareStatementCount() granularity (empirically 1–3 for the entity-graph fetch).

Declined (with rationale)

• Drop Serializable/serialVersionUID: StatementInspector extends Serializable, so the class is unavoidably serializable; the serialVersionUID suppresses the -Xlint:serial warning. Keeping it is correct rather than boilerplate.

Full suite green (991 tests, 0 failures).

[claude]

Review: test: isolate entity-graph query-count assertion from parallel-execution pollution

Nice root-cause fix — moving from the SessionFactory-global Statistics counter to a per-thread StatementInspector correctly targets the actual problem (a non-transactional, JVM-wide counter being polluted by concurrent @DataJpaTest classes sharing one cached context/SessionFactory under JUnit 5 parallel execution). A few notes, nothing blocking:

Strengths

• Correct fix for the right reason: ThreadLocal isolation is the right tool here since each parallel JUnit test runs its own thread + JDBC connection — this eliminates the flake by construction rather than papering over it with a wider tolerance or @Execution(SAME_THREAD).
• isBetween(1, 3) lower bound is a nice touch — turns 'inspector silently unregistered' from a false-green pass into a loud failure. Good defensive test design.
• StatementCountInspectorTest is a solid addition — it locks in reset/count/inspect semantics independently of the one consumer test, and the interleaved two-thread test (shouldIsolateCountsBetweenThreads) directly exercises the property that actually matters (one thread's reset()/counting can't perturb another's).
• Good javadoc explaining why the global Statistics counter was unsafe and why the ThreadLocal approach is safe — this will save the next person from re-litigating the root cause.

Minor points worth considering

1. Footgun for future consumers: the ThreadLocal is never cleared (only reset), and JUnit's parallel executor reuses a bounded worker pool. That's safe today because the sole consumer always calls reset() before measuring — but if a second test later starts reading getCount() without resetting first, it could silently inherit a stale value from whatever ran previously on that pooled thread. The javadoc already documents the 'always reset() first' contract, so this is a documentation/discipline issue rather than a bug — might be worth a one-line reminder directly on getCount()'s javadoc too, since that's the method a future caller is more likely to read in isolation.
2. Profile-wide registration: registering the inspector via application-test.properties means every JPA test in the suite now pays a ThreadLocal get/set per prepared statement, not just UserRepositoryEntityGraphTest. The PR description already calls this out as intentional and negligible, which I agree with — just flagging it's a deliberate blast-radius expansion in case future contributors are surprised to find this wired in globally when grepping for it.
3. Instantiation path: since the inspector is wired via the hibernate.session_factory.statement_inspector property (a class-name string), Hibernate instantiates it via the implicit no-arg constructor. Fine here since the class has no dependencies, but worth double-checking on the Spring Boot 3.5/Java 17 test matrix too (testJdk17/testAll) since Hibernate ORM versions can differ slightly in how this property is resolved.

Test coverage
Coverage looks good — the fix itself, the regression test, and a standalone unit test for the new inspector's concurrency contract. No gaps I'd block on.

Security / performance
No security concerns (test-only infrastructure, not shipped to consumers of the library). Performance impact is negligible and confined to the test suite, as discussed above.

Overall: solid, well-reasoned flaky-test fix with good accompanying test coverage. Approving from a code-quality/correctness standpoint.