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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
