You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fineract.apache.org by "adamsaghy (via GitHub)" <gi...@apache.org> on 2026/08/14 11:05:55 UTC
[PR] FINERACT-2684: Order integration tests: Run first tests which executes long running jobs [fineract]
adamsaghy opened a new pull request, #6271:
URL: https://github.com/apache/fineract/pull/6271
## 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!
- [ ] Write the commit message as per [our guidelines](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#pull-requests)
- [ ] Acknowledge that we will not review PRs that are not passing the build _("green")_ - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
- [ ] Create/update [unit or integration tests](https://fineract.apache.org/docs/current/#_testing) for verifying the changes made.
- [ ] Follow our [coding conventions](https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions).
- [ ] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
- [ ] [This PR must not be a "code dump"](https://cwiki.apache.org/confluence/display/FINERACT/Pull+Request+Size+Limit). Large changes can be made in a branch, with assistance. Ask for help on the [developer mailing list](https://fineract.apache.org/#contribute).
- [ ] If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.
Your assigned reviewer(s) will follow our [guidelines for code reviews](https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2684: Order integration tests: Run tests first which executes long running jobs [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #6271:
URL: https://github.com/apache/fineract/pull/6271#discussion_r3795192756
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/bulkimport/populator/savings/SavingsWorkbookPopulateTest.java:
##########
@@ -36,12 +36,15 @@
import org.apache.fineract.integrationtests.common.organisation.StaffHelper;
import org.apache.fineract.integrationtests.common.savings.SavingsAccountHelper;
import org.apache.fineract.integrationtests.common.savings.SavingsProductHelper;
+import org.apache.fineract.integrationtests.common.savings.SavingsTestLifecycleExtension;
import org.apache.poi.ss.usermodel.Row;
import org.apache.poi.ss.usermodel.Sheet;
import org.apache.poi.ss.usermodel.Workbook;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+@ExtendWith({ SavingsTestLifecycleExtension.class })
Review Comment:
Nice catch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2684: Order integration tests: Run tests first which executes long running jobs [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #6271:
URL: https://github.com/apache/fineract/pull/6271#discussion_r3795164811
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/loan/penalty/LoanPenaltyBackdatedTransactionTest.java:
##########
@@ -35,8 +35,10 @@
import org.apache.fineract.integrationtests.common.charges.ChargesHelper;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Order;
import org.junit.jupiter.api.Test;
+@Order(2)
Review Comment:
Marked with @Order(2) all integration tests which executes:
-POST_INTEREST_FOR_SAVINGS
-ADD_ACCRUAL_ENTRIES
-ADD_PERIODIC_ACCRUAL_ENTRIES
-ADD_PERIODIC_ACCRUAL_ENTRIES_FOR_LOANS_WITH_INCOME_POSTED_AS_TRANSACTIONS
-ADD_PERIODIC_ACCRUAL_ENTRIES_FOR_SAVINGS_WITH_INCOME_POSTED_AS_TRANSACTIONS
Marked with @Order(1) all integration tests which executes:
- LOAN_COB
- WORKING_CAPITAL_LOAN_COB_JOB
- SavingsInterestPostingTest
These tests to be executed before the tests that are not marked with order.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2684: Order integration tests: Run tests first which executes long running jobs [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #6271:
URL: https://github.com/apache/fineract/pull/6271#discussion_r3795248631
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/client/feign/tests/FeignTrialBalanceSummaryReportTest.java:
##########
@@ -68,6 +68,7 @@
@Slf4j
@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
+@Order(1)
Review Comment:
https://github.com/apache/fineract/pull/6271#discussion_r3795164811
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2684: Order integration tests: Run tests first which executes long running jobs [fineract]
Posted by "Aman-Mittal (via GitHub)" <gi...@apache.org>.
Aman-Mittal commented on PR #6271:
URL: https://github.com/apache/fineract/pull/6271#issuecomment-5296803969
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 `@Order` goes from 11 to 30 classes across three tiers, and `scripts/split-tests.sh` now depends on it for shard balancing), rolls `SavingsTestLifecycleExtension` out to roughly ten additional classes, and carries a behavioural fix to the `enable-auto-generated-external-id` handling in `LoanManualInterestRefundResponseStructureTest`.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2684: Order integration tests: Run tests first which executes long running jobs [fineract]
Posted by "Aman-Mittal (via GitHub)" <gi...@apache.org>.
Aman-Mittal commented on code in PR #6271:
URL: https://github.com/apache/fineract/pull/6271#discussion_r3786234360
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/loan/penalty/LoanPenaltyBackdatedTransactionTest.java:
##########
@@ -35,8 +35,10 @@
import org.apache.fineract.integrationtests.common.charges.ChargesHelper;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Order;
import org.junit.jupiter.api.Test;
+@Order(2)
Review 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?
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/LoanManualInterestRefundResponseStructureTest.java:
##########
@@ -59,48 +61,53 @@ public static void beforeAll() {
@Test
public void testManualInterestRefundResponseStructureWithoutExternalIds() {
AtomicReference<Long> loanIdRef = new AtomicReference<>();
- AtomicReference<Long> targetTransactionIdRef = new AtomicReference<>();
- runAt("01 January 2024", () -> {
- Long loanProductId = createLoanProduct(
- create4IProgressive().daysInMonthType(DaysInMonthType.ACTUAL).daysInYearType(DaysInYearType.ACTUAL)
- .addSupportedInterestRefundTypesItem(SupportedInterestRefundTypesItem.MERCHANT_ISSUED_REFUND)
- .recalculationRestFrequencyType(RecalculationRestFrequencyType.DAILY));
+ try {
+ globalConfigurationHelper.updateGlobalConfiguration(GlobalConfigurationConstants.ENABLE_AUTO_GENERATED_EXTERNAL_ID,
Review 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?
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/savings/base/BaseSavingsIntegrationTest.java:
##########
@@ -53,12 +53,15 @@
import org.apache.fineract.integrationtests.common.GlobalConfigurationHelper;
import org.apache.fineract.integrationtests.common.Utils;
import org.apache.fineract.integrationtests.common.accounting.JournalEntryHelper;
+import org.apache.fineract.integrationtests.common.savings.SavingsTestLifecycleExtension;
import org.apache.fineract.integrationtests.common.system.CodeHelper;
import org.hamcrest.Matcher;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.extension.ExtendWith;
@Slf4j
+@ExtendWith({ SavingsTestLifecycleExtension.class })
Review 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:
- `closeSavingsAccount` takes 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?
- `SavingsTestLifecycleExtension` is `AfterAllCallback` only, whereas `LoanTestLifecycleExtension` is `beforeEach`/`afterEach`. Is there a reason for the two models to differ, or is converging them worth a follow-up ticket?
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/AccountingScenarioIntegrationTest.java:
##########
@@ -79,12 +79,14 @@
import org.apache.fineract.integrationtests.common.shares.ShareProductTransactionHelper;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Order;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@SuppressWarnings({ "unchecked" })
+@Order(2)
Review 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?
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/bulkimport/populator/savings/SavingsWorkbookPopulateTest.java:
##########
@@ -36,12 +36,15 @@
import org.apache.fineract.integrationtests.common.organisation.StaffHelper;
import org.apache.fineract.integrationtests.common.savings.SavingsAccountHelper;
import org.apache.fineract.integrationtests.common.savings.SavingsProductHelper;
+import org.apache.fineract.integrationtests.common.savings.SavingsTestLifecycleExtension;
import org.apache.poi.ss.usermodel.Row;
import org.apache.poi.ss.usermodel.Sheet;
import org.apache.poi.ss.usermodel.Workbook;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+@ExtendWith({ SavingsTestLifecycleExtension.class })
Review 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?
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/LoanManualInterestRefundResponseStructureTest.java:
##########
@@ -59,48 +61,53 @@ public static void beforeAll() {
@Test
public void testManualInterestRefundResponseStructureWithoutExternalIds() {
AtomicReference<Long> loanIdRef = new AtomicReference<>();
- AtomicReference<Long> targetTransactionIdRef = new AtomicReference<>();
- runAt("01 January 2024", () -> {
- Long loanProductId = createLoanProduct(
- create4IProgressive().daysInMonthType(DaysInMonthType.ACTUAL).daysInYearType(DaysInYearType.ACTUAL)
- .addSupportedInterestRefundTypesItem(SupportedInterestRefundTypesItem.MERCHANT_ISSUED_REFUND)
- .recalculationRestFrequencyType(RecalculationRestFrequencyType.DAILY));
+ try {
+ globalConfigurationHelper.updateGlobalConfiguration(GlobalConfigurationConstants.ENABLE_AUTO_GENERATED_EXTERNAL_ID,
+ new PutGlobalConfigurationsRequest().enabled(false));
+ runAt("01 January 2024", () -> {
+ Long loanProductId = createLoanProduct(
+ create4IProgressive().daysInMonthType(DaysInMonthType.ACTUAL).daysInYearType(DaysInYearType.ACTUAL)
+ .addSupportedInterestRefundTypesItem(SupportedInterestRefundTypesItem.MERCHANT_ISSUED_REFUND)
+ .recalculationRestFrequencyType(RecalculationRestFrequencyType.DAILY));
- Long loanId = applyAndApproveProgressiveLoan(client.getClientId(), loanProductId, "01 January 2024", 1000.0, 9.9, 12, null);
- assertNotNull(loanId);
- loanIdRef.set(loanId);
+ Long loanId = applyAndApproveProgressiveLoan(client.getClientId(), loanProductId, "01 January 2024", 1000.0, 9.9, 12, null);
+ assertNotNull(loanId);
+ loanIdRef.set(loanId);
- disburseLoan(loanId, BigDecimal.valueOf(1000), "01 January 2024");
- });
+ disburseLoan(loanId, BigDecimal.valueOf(1000), "01 January 2024");
+ });
- runAt("15 January 2024", () -> {
- Long loanId = loanIdRef.get();
+ runAt("15 January 2024", () -> {
+ Long loanId = loanIdRef.get();
- PostLoansLoanIdTransactionsResponse refundResponse = makeLoanMerchantIssuedRefund(loanId, "15 January 2024", 100.0);
- assertNotNull(refundResponse);
- assertNotNull(refundResponse.getResourceId());
- targetTransactionIdRef.set(refundResponse.getResourceId());
+ PostLoansLoanIdTransactionsResponse refundResponse = makeLoanMerchantIssuedRefund(loanId, "15 January 2024", 100.0);
+ assertNotNull(refundResponse);
+ assertNotNull(refundResponse.getResourceId());
- PostLoansLoanIdTransactionsResponse interestRefundResponse = createManualInterestRefund(loanId, refundResponse.getResourceId(),
- "15 January 2024", 5.0, null);
+ PostLoansLoanIdTransactionsResponse interestRefundResponse = createManualInterestRefund(loanId,
+ refundResponse.getResourceId(), "15 January 2024", 5.0, null);
- assertNotNull(interestRefundResponse, "Interest refund response should not be null");
- assertNotNull(interestRefundResponse.getResourceId(), "Interest refund resource ID should not be null");
+ assertNotNull(interestRefundResponse, "Interest refund response should not be null");
+ assertNotNull(interestRefundResponse.getResourceId(), "Interest refund resource ID should not be null");
- GetLoansLoanIdResponse loanDetails = getLoanDetails(loanId);
- GetLoansLoanIdTransactions interestRefundTransaction = findTransactionByType(loanDetails, "Interest Refund");
- assertNotNull(interestRefundTransaction, "Interest Refund transaction should exist");
+ GetLoansLoanIdResponse loanDetails = getLoanDetails(loanId);
+ GetLoansLoanIdTransactions interestRefundTransaction = findTransactionByType(loanDetails, "Interest Refund");
+ assertNotNull(interestRefundTransaction, "Interest Refund transaction should exist");
- assertEquals(interestRefundTransaction.getId(), interestRefundResponse.getResourceId(),
- "Response entityId should be the Interest Refund transaction ID");
+ assertEquals(interestRefundTransaction.getId(), interestRefundResponse.getResourceId(),
+ "Response entityId should be the Interest Refund transaction ID");
- assertNull(interestRefundResponse.getResourceExternalId(), "entityExternalId should be null when no external ID provided");
+ assertNull(interestRefundResponse.getResourceExternalId(), "entityExternalId should be null when no external ID provided");
- assertNull(interestRefundResponse.getSubResourceId(), "subEntityId should be null");
+ assertNull(interestRefundResponse.getSubResourceId(), "subEntityId should be null");
- assertNull(interestRefundResponse.getSubResourceExternalId(), "subEntityExternalId should be null");
- });
+ assertNull(interestRefundResponse.getSubResourceExternalId(), "subEntityExternalId should be null");
+ });
+ } finally {
+ globalConfigurationHelper.updateGlobalConfiguration(GlobalConfigurationConstants.ENABLE_AUTO_GENERATED_EXTERNAL_ID,
Review 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.
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/client/feign/tests/FeignTrialBalanceSummaryReportTest.java:
##########
@@ -68,6 +68,7 @@
@Slf4j
@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
+@Order(1)
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2684: Order integration tests: Run tests first which executes long running jobs [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy merged PR #6271:
URL: https://github.com/apache/fineract/pull/6271
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2684: Order integration tests: Run tests first which executes long running jobs [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6271:
URL: https://github.com/apache/fineract/pull/6271#issuecomment-5314236337
> 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](https://issues.apache.org/jira/browse/FINERACT-2684) — please disregard that part.
>
> My understanding is that [FINERACT-2684](https://issues.apache.org/jira/browse/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 `@Order` goes from 11 to 30 classes across three tiers, and `scripts/split-tests.sh` now depends on it for shard balancing), rolls `SavingsTestLifecycleExtension` out to roughly ten additional classes, and carries a behavioural fix to the `enable-auto-generated-external-id` handling in `LoanManualInterestRefundResponseStructureTest`.
>
> 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](https://issues.apache.org/jira/browse/FINERACT-2684) didn't look like the right long-term home for it.
Since it is not changing any business logic, it is minor and qualify as enhancement which this story is for...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2684: Order integration tests: Run tests first which executes long running jobs [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #6271:
URL: https://github.com/apache/fineract/pull/6271#discussion_r3795183554
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/LoanManualInterestRefundResponseStructureTest.java:
##########
@@ -59,48 +61,53 @@ public static void beforeAll() {
@Test
public void testManualInterestRefundResponseStructureWithoutExternalIds() {
AtomicReference<Long> loanIdRef = new AtomicReference<>();
- AtomicReference<Long> targetTransactionIdRef = new AtomicReference<>();
- runAt("01 January 2024", () -> {
- Long loanProductId = createLoanProduct(
- create4IProgressive().daysInMonthType(DaysInMonthType.ACTUAL).daysInYearType(DaysInYearType.ACTUAL)
- .addSupportedInterestRefundTypesItem(SupportedInterestRefundTypesItem.MERCHANT_ISSUED_REFUND)
- .recalculationRestFrequencyType(RecalculationRestFrequencyType.DAILY));
+ try {
+ globalConfigurationHelper.updateGlobalConfiguration(GlobalConfigurationConstants.ENABLE_AUTO_GENERATED_EXTERNAL_ID,
Review Comment:
These issues were noticed during the reordering and blocks the PR till fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2684: Order integration tests: Run tests first which executes long running jobs [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #6271:
URL: https://github.com/apache/fineract/pull/6271#discussion_r3795196932
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/savings/base/BaseSavingsIntegrationTest.java:
##########
@@ -53,12 +53,15 @@
import org.apache.fineract.integrationtests.common.GlobalConfigurationHelper;
import org.apache.fineract.integrationtests.common.Utils;
import org.apache.fineract.integrationtests.common.accounting.JournalEntryHelper;
+import org.apache.fineract.integrationtests.common.savings.SavingsTestLifecycleExtension;
import org.apache.fineract.integrationtests.common.system.CodeHelper;
import org.hamcrest.Matcher;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.extension.ExtendWith;
@Slf4j
+@ExtendWith({ SavingsTestLifecycleExtension.class })
Review Comment:
I like the idea of centralizing this. Even if there are not open savings accounts, it makes easier the maintenance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2684: Order integration tests: Run tests first which executes long running jobs [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6271:
URL: https://github.com/apache/fineract/pull/6271#issuecomment-5314185767
> 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:
>
> 1. **Measured effect** — could [FINERACT-2684](https://issues.apache.org/jira/browse/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.
> 2. **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_shards` changes? Would it be worth having `split-tests.sh` print the ordered-class distribution so drift becomes visible in the logs?
> 3. **Parsing robustness** — the awk in `split-tests.sh` matches 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?
> 4. **Scope** — would splitting this into three PRs be practical (the `@Order` annotations / the `SavingsTestLifecycleExtension` rollout / the `enable-auto-generated-external-id` fix in `LoanManualInterestRefundResponseStructureTest`)? The third is a behavioural change that's easy to miss inside a test-infra change.
>
> Nothing blocking from me — just comments. Thanks again!
1.Execution times of the chunks are always logged
2. Over scaling does not bring any value + github and Apache infra has their own limitations.
3. let me review whether we can enhance further the logic
4. I would not split them. These issues were bring into light during the reordering.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2684: Order integration tests: Run tests first which executes long running jobs [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #6271:
URL: https://github.com/apache/fineract/pull/6271#discussion_r3795180546
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/AccountingScenarioIntegrationTest.java:
##########
@@ -79,12 +79,14 @@
import org.apache.fineract.integrationtests.common.shares.ShareProductTransactionHelper;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Order;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@SuppressWarnings({ "unchecked" })
+@Order(2)
Review Comment:
Asked an answered: https://github.com/apache/fineract/pull/6271#discussion_r3795164811
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2684: Order integration tests: Run tests first which executes long running jobs [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #6271:
URL: https://github.com/apache/fineract/pull/6271#discussion_r3795246717
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/LoanManualInterestRefundResponseStructureTest.java:
##########
@@ -59,48 +61,53 @@ public static void beforeAll() {
@Test
public void testManualInterestRefundResponseStructureWithoutExternalIds() {
AtomicReference<Long> loanIdRef = new AtomicReference<>();
- AtomicReference<Long> targetTransactionIdRef = new AtomicReference<>();
- runAt("01 January 2024", () -> {
- Long loanProductId = createLoanProduct(
- create4IProgressive().daysInMonthType(DaysInMonthType.ACTUAL).daysInYearType(DaysInYearType.ACTUAL)
- .addSupportedInterestRefundTypesItem(SupportedInterestRefundTypesItem.MERCHANT_ISSUED_REFUND)
- .recalculationRestFrequencyType(RecalculationRestFrequencyType.DAILY));
+ try {
+ globalConfigurationHelper.updateGlobalConfiguration(GlobalConfigurationConstants.ENABLE_AUTO_GENERATED_EXTERNAL_ID,
+ new PutGlobalConfigurationsRequest().enabled(false));
+ runAt("01 January 2024", () -> {
+ Long loanProductId = createLoanProduct(
+ create4IProgressive().daysInMonthType(DaysInMonthType.ACTUAL).daysInYearType(DaysInYearType.ACTUAL)
+ .addSupportedInterestRefundTypesItem(SupportedInterestRefundTypesItem.MERCHANT_ISSUED_REFUND)
+ .recalculationRestFrequencyType(RecalculationRestFrequencyType.DAILY));
- Long loanId = applyAndApproveProgressiveLoan(client.getClientId(), loanProductId, "01 January 2024", 1000.0, 9.9, 12, null);
- assertNotNull(loanId);
- loanIdRef.set(loanId);
+ Long loanId = applyAndApproveProgressiveLoan(client.getClientId(), loanProductId, "01 January 2024", 1000.0, 9.9, 12, null);
+ assertNotNull(loanId);
+ loanIdRef.set(loanId);
- disburseLoan(loanId, BigDecimal.valueOf(1000), "01 January 2024");
- });
+ disburseLoan(loanId, BigDecimal.valueOf(1000), "01 January 2024");
+ });
- runAt("15 January 2024", () -> {
- Long loanId = loanIdRef.get();
+ runAt("15 January 2024", () -> {
+ Long loanId = loanIdRef.get();
- PostLoansLoanIdTransactionsResponse refundResponse = makeLoanMerchantIssuedRefund(loanId, "15 January 2024", 100.0);
- assertNotNull(refundResponse);
- assertNotNull(refundResponse.getResourceId());
- targetTransactionIdRef.set(refundResponse.getResourceId());
+ PostLoansLoanIdTransactionsResponse refundResponse = makeLoanMerchantIssuedRefund(loanId, "15 January 2024", 100.0);
+ assertNotNull(refundResponse);
+ assertNotNull(refundResponse.getResourceId());
- PostLoansLoanIdTransactionsResponse interestRefundResponse = createManualInterestRefund(loanId, refundResponse.getResourceId(),
- "15 January 2024", 5.0, null);
+ PostLoansLoanIdTransactionsResponse interestRefundResponse = createManualInterestRefund(loanId,
+ refundResponse.getResourceId(), "15 January 2024", 5.0, null);
- assertNotNull(interestRefundResponse, "Interest refund response should not be null");
- assertNotNull(interestRefundResponse.getResourceId(), "Interest refund resource ID should not be null");
+ assertNotNull(interestRefundResponse, "Interest refund response should not be null");
+ assertNotNull(interestRefundResponse.getResourceId(), "Interest refund resource ID should not be null");
- GetLoansLoanIdResponse loanDetails = getLoanDetails(loanId);
- GetLoansLoanIdTransactions interestRefundTransaction = findTransactionByType(loanDetails, "Interest Refund");
- assertNotNull(interestRefundTransaction, "Interest Refund transaction should exist");
+ GetLoansLoanIdResponse loanDetails = getLoanDetails(loanId);
+ GetLoansLoanIdTransactions interestRefundTransaction = findTransactionByType(loanDetails, "Interest Refund");
+ assertNotNull(interestRefundTransaction, "Interest Refund transaction should exist");
- assertEquals(interestRefundTransaction.getId(), interestRefundResponse.getResourceId(),
- "Response entityId should be the Interest Refund transaction ID");
+ assertEquals(interestRefundTransaction.getId(), interestRefundResponse.getResourceId(),
+ "Response entityId should be the Interest Refund transaction ID");
- assertNull(interestRefundResponse.getResourceExternalId(), "entityExternalId should be null when no external ID provided");
+ assertNull(interestRefundResponse.getResourceExternalId(), "entityExternalId should be null when no external ID provided");
- assertNull(interestRefundResponse.getSubResourceId(), "subEntityId should be null");
+ assertNull(interestRefundResponse.getSubResourceId(), "subEntityId should be null");
- assertNull(interestRefundResponse.getSubResourceExternalId(), "subEntityExternalId should be null");
- });
+ assertNull(interestRefundResponse.getSubResourceExternalId(), "subEntityExternalId should be null");
+ });
+ } finally {
+ globalConfigurationHelper.updateGlobalConfiguration(GlobalConfigurationConstants.ENABLE_AUTO_GENERATED_EXTERNAL_ID,
Review Comment:
You are right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org