FINERACT-2684: Order integration tests: Run tests first which executes long running jobs - #6271
Conversation
adamsaghy
changed the title
…s long running jobs
a27c0e4 to
56833ff
Compare
Aman-Mittal
left a comment
Aman-Mittal
left a comment
There was a problem hiding this comment.
Thanks for picking this up, @adamsaghy — front-loading the job-heavy classes is a sensible lever, and it's nice that scripts/split-tests.sh already understands class-level @Order so the annotated classes get spread round-robin across the 15 shards.
I've left some questions inline, mostly trying to understand the intent rather than pushing for changes. A few that don't attach to a particular line:
- Measured effect — could FINERACT-2684 record the before/after numbers? Which shard was the bottleneck, and how much wall-clock does this save? The green matrix here doesn't really demonstrate it, since each shard runs serially either way.
- Scaling — ordering only takes effect within a shard's JVM. At 30 ordered classes over 15 shards it happens to land at exactly 2 per shard; what happens at 31, or if
total_shardschanges? Would it be worth havingsplit-tests.shprint the ordered-class distribution so drift becomes visible in the logs? - Parsing robustness — the awk in
split-tests.shmatches the literal@Order(<digits>)form. If someone writes@Order(value = 1), JUnit still honours it but the splitter silently treats the class as unordered. Worth a guard, or is the shorthand form enough of a convention? - Scope — would splitting this into three PRs be practical (the
@Orderannotations / theSavingsTestLifecycleExtensionrollout / theenable-auto-generated-external-idfix inLoanManualInterestRefundResponseStructureTest)? The third is a behavioural change that's easy to miss inside a test-infra change.
Nothing blocking from me — just comments. Thanks again!
| import org.junit.jupiter.api.Order; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| @Order(2) |
There was a problem hiding this comment.
Could you help me understand the criterion for tier 1 vs tier 2? Reading the PR as "run the long-job tests first", I'd have expected this class to be in the earliest tier — it has by far the most scheduler/inline-COB call sites of the classes touched here (~13), yet it's @Order(2), while e.g. LoanRepaymentScheduleWithDownPaymentTest has a single one and is @Order(1).
Related: with this PR the repo goes from 11 to 30 class-level @Order annotations across three tiers (1, 2, and LoanImportHandlerTest's 3), and I couldn't find the convention written down anywhere — not in CONTRIBUTING.md, junit-platform.properties, or scripts/split-tests.sh. Could FINERACT-2684 capture what each tier is meant to represent (ideally mirrored as a short comment next to junit.jupiter.testclass.order.default), so the next contributor knows which value to pick?
| import org.slf4j.LoggerFactory; | ||
|
|
||
| @SuppressWarnings({ "unchecked" }) | ||
| @Order(2) |
There was a problem hiding this comment.
Just double-checking the semantics here: ClassOrderer.OrderAnnotation assigns unannotated classes Integer.MAX_VALUE / 2, so @Order(2) still places this class ahead of the entire unannotated bulk rather than after it. Is that the intent (tier 2 = "second wave of long-job tests"), or was the idea to push these towards the end?
Also, several classes now share the same order value — for ties JUnit falls back to discovery order. Is that stable enough across the MariaDB / MySQL / PostgreSQL matrices, or should the values be unique where the relative order actually matters?
|
|
||
| @Slf4j | ||
| @TestMethodOrder(MethodOrderer.OrderAnnotation.class) | ||
| @Order(1) |
There was a problem hiding this comment.
This one gave me a bit of pause. This class asserts on what look like whole-ledger properties — beginBal equal to ZERO (~L237) and "Report should not contain Retained Earnings rows when no annual summary data exists" (~L257). Those hold only while the ledger is still (near) empty.
Is @Order(1) here about long-running jobs, or is it effectively guaranteeing this test runs before any other accounting test in its shard so those assertions keep passing? If it's the latter, would it be preferable to scope the test to its own GL accounts / product instead? My worry is that the dependency becomes invisible — the test would start failing the moment anything else lands in tier 1 ahead of it, with nothing in the code to hint why.
| .addSupportedInterestRefundTypesItem(SupportedInterestRefundTypesItem.MERCHANT_ISSUED_REFUND) | ||
| .recalculationRestFrequencyType(RecalculationRestFrequencyType.DAILY)); | ||
| try { | ||
| globalConfigurationHelper.updateGlobalConfiguration(GlobalConfigurationConstants.ENABLE_AUTO_GENERATED_EXTERNAL_ID, |
There was a problem hiding this comment.
This change reads as a functional test fix rather than an ordering one, and I couldn't find it mentioned anywhere — could a note go on FINERACT-2684 (or the PR description) explaining it?
If I'm reading it right, the test was relying on enable-auto-generated-external-id being off, and something else in the suite leaves it on. If so, is the leaking test the actual root cause worth fixing? My concern is that class ordering makes such a leak reproducible rather than absent, so the next test that depends on this config could still be affected depending on which shard it lands in.
Small related question: testManualInterestRefundResponseStructureWithExternalIds below doesn't set the flag and shares the client created in @BeforeAll — is it now implicitly relying on this method having run and restored the config first?
| assertNull(interestRefundResponse.getSubResourceExternalId(), "subEntityExternalId should be null"); | ||
| }); | ||
| } finally { | ||
| globalConfigurationHelper.updateGlobalConfiguration(GlobalConfigurationConstants.ENABLE_AUTO_GENERATED_EXTERNAL_ID, |
There was a problem hiding this comment.
Should the finally restore to false rather than true? The seeded default for this configuration is disabled — db/changelog/tenant/parts/0071_add_external_id_support_for_loan_transaction.xml inserts enable-auto-generated-external-id with enabled=false. As written, the test leaves the server with the flag flipped relative to the default, which is the same class of leak the ordering work is trying to contain.
The codebase seems split on this too: ClientExternalIdTest uses the disable → enable pattern, while ClientLoanIntegrationTest (L2579 / L2777) uses enable → disable. Would reading the current value up front and restoring that be more robust than hardcoding either way? Happy to be corrected if true is the effective default in the integration-test environment for some reason I've missed.
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.extension.ExtendWith; | ||
|
|
||
| @ExtendWith({ SavingsTestLifecycleExtension.class }) |
There was a problem hiding this comment.
A question about the cost side of this rollout. SavingsTestLifecycleExtension.afterAll unconditionally runs the Post Interest For Savings scheduler job before closing accounts. This PR adds the extension to ~10 more classes (5 directly, plus BaseSavingsIntegrationTest's subclasses), so that's ~10 additional executions of a long-running job — which feels like it pulls against the PR's goal.
For this class in particular: does it create active savings accounts at all, or does it only populate the template workbook? If the latter, the job run and the status-300 scan would be pure overhead. Would gating the job on "this class actually created savings accounts" be feasible?
| import org.junit.jupiter.api.extension.ExtendWith; | ||
|
|
||
| @Slf4j | ||
| @ExtendWith({ SavingsTestLifecycleExtension.class }) |
There was a problem hiding this comment.
Registering the extension on the base class also applies it to the four subclasses — including ShareAccountChargeRoundingTest and the account-transfer tests — and the teardown closes every savings account in status 300 on the server, not only the ones the class created. Is that broad a teardown safe for whichever class runs next in the same shard?
Two smaller things while we're here, both pre-existing but amplified by the wider rollout:
closeSavingsAccounttakes neither branch when the balance is exactly zero, so those accounts stay active and get re-scanned by every subsequent class's teardown. Is that deliberate?SavingsTestLifecycleExtensionisAfterAllCallbackonly, whereasLoanTestLifecycleExtensionisbeforeEach/afterEach. Is there a reason for the two models to differ, or is converging them worth a follow-up ticket?
|
One follow-up to my review, and an amendment to it: in a couple of the inline comments I suggested recording the rationale on FINERACT-2684 — please disregard that part. My understanding is that FINERACT-2684 is scoped for small changes, and this PR is somewhat more than that. It establishes a repo-wide test execution convention (class-level Would you be open to raising a dedicated ticket for this? It would give the tier definitions, the before/after timings, and the shard-balancing assumptions a durable home that future contributors can find — which is really what my inline questions were reaching for. If the split I mentioned in the review appeals, separate tickets for the savings cleanup rollout and the external-ID fix would work nicely too, but a single dedicated ticket covering this PR would already address my concern. Happy to defer to your judgement on the ticket layout — just flagging that FINERACT-2684 didn't look like the right long-term home for it. |
2 participants
Description
Describe the changes made and why they were made. (Ignore if these details are present on the associated Apache Fineract JIRA ticket.)
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Your assigned reviewer(s) will follow our guidelines for code reviews.