FINERACT-2380: move retrofit client based test to feign #5164

budaidev · 2025-11-18T13:51:07Z

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 https://github.com/apache/fineract/#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 for verifying the changes made.
Follow coding conventions at 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
Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)

FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.

galovics · 2025-11-19T21:55:20Z

...src/main/java/org/apache/fineract/client/feign/services/ExternalAssetOwnersApiExtension.java

+
+    @RequestLine("POST /v1/external-asset-owners/transfers/{id}")
+    @Headers("Content-Type: application/json")
+    PostInitiateTransferResponse transferRequestWithIdWithBody(@Param("id") Long id, ExternalAssetOwnerRequest body,


Why are these manually created *Extension interfaces created?

Are they there because of the Map parameter at the end?

yes, openapi does not generate header parameter, so I needed a way to pass these

then I'd say don't do this but make the generation use a custom mustache template which generates the headers Map into the code. That's what we did for the retrofit clients too.

That`s also a good option, I was thinking about that. I had this solution because it was the least invasive, but in long term you are probably right about this

@budaidev are you planning to address this based on @galovics recommendations?

adamsaghy · 2025-11-21T13:07:28Z

...src/test/java/org/apache/fineract/integrationtests/client/feign/helpers/FeignLoanHelper.java


    public Long applyForLoan(PostLoansRequest request) {
-        PostLoansResponse response = ok(() -> fineractClient.loans().calculateLoanScheduleOrSubmitLoanApplication(request, null));
+        PostLoansResponse response = ok(() -> fineractClient.loans().calculateLoanScheduleOrSubmitLoanApplication(request, (String) null));


this is not required...right?

It's method overload issue unfortunately, it cannot figure the type of the second parameter

adamsaghy · 2025-11-21T13:07:59Z

.../java/org/apache/fineract/integrationtests/client/feign/helpers/FeignBusinessDateHelper.java

                .locale(LoanTestData.LOCALE);

-        ok(() -> fineractClient.businessDateManagement().updateBusinessDate(request, Collections.emptyMap()));
+        ok(() -> fineractClient.businessDateManagement().updateBusinessDate(null, request, Collections.emptyMap()));


what is the first param that to be null?

there is an idempotency key which is not needed here

adamsaghy · 2025-11-21T13:08:17Z

fineract-e2e-tests-runner/src/test/resources/features/LoanReschedule.feature

      | 16 January 2024    | 10 January 2024 | 31 January 2024 | 10 January 2024 | true                 |

-  @TestRailId:C3318 @AdvancedPaymentAllocation
+  @Skip @TestRailId:C3318 @AdvancedPaymentAllocation


Should not be skipped, no?

adamsaghy · 2025-11-21T13:08:37Z

fineract-e2e-tests-core/src/test/java/org/apache/fineract/test/api/ApiConfiguration.java

-    @Bean
-    public LoanProductsCustomApi loanProductsCustomApi() {
-        return fineractClient.createService(LoanProductsCustomApi.class);
-    }
-


Is this not used?

adamsaghy · 2025-11-24T11:30:31Z

fineract-e2e-tests-core/src/test/resources/fineract-test-application.properties

+fineract-test.messaging.jms.topic-name=${ACTIVEMQ_TOPIC_NAME:events}

-fineract-test.event.verification-enabled=${EVENT_VERIFICATION_ENABLED:false}
+fineract-test.event.verification-enabled=${EVENT_VERIFICATION_ENABLED:true}


This should be false default.

adamsaghy · 2025-11-24T11:30:44Z

fineract-e2e-tests-core/src/test/resources/fineract-test-application.properties

 fineract-test.testrail.run-id=${TESTRAIL_RUN_ID:0}

-fineract-test.messaging.jms.broker-url=${ACTIVEMQ_BROKER_URL:}
+fineract-test.messaging.jms.broker-url=${ACTIVEMQ_BROKER_URL:tcp://localhost:63000}


No need for default value.

adamsaghy · 2025-11-24T11:31:12Z

fineract-e2e-tests-core/src/test/resources/fineract-test-application.properties

 fineract-test.messaging.jms.broker-username=${ACTIVEMQ_BROKER_USERNAME:}
 fineract-test.messaging.jms.broker-password=${ACTIVEMQ_BROKER_PASSWORD:}
-fineract-test.messaging.jms.topic-name=${ACTIVEMQ_TOPIC_NAME:}
+fineract-test.messaging.jms.topic-name=${ACTIVEMQ_TOPIC_NAME:events}


No need for default value.

adamsaghy · 2025-11-24T13:36:44Z

...-feign/src/main/java/org/apache/fineract/client/feign/support/FeignResponseHeaderHolder.java

+ * Thread-local holder for the last Feign response headers. This is primarily used by E2E tests that need to assert on
+ * HTTP headers when using Feign clients that otherwise only expose the deserialized body.
+ */
+public final class FeignResponseHeaderHolder {


I’m concerned that using a ThreadLocal store may not be the right approach here. Since the header checks happen after the client call, the header values may already be removed or modified by that point.
It would be safer to use the endpoints that return ApiResponse via the withHttpInformation methods, so we can retrieve both the response data and the headers directly.

adamsaghy · 2025-11-24T13:40:48Z

...-e2e-tests-core/src/test/java/org/apache/fineract/test/messaging/event/EventCheckHelper.java

+    private void waitForTransactionCommit() {
+        if (eventProperties.isEventVerificationEnabled() && TRANSACTION_COMMIT_DELAY_MS > 0) {
+            try {
+                Thread.sleep(TRANSACTION_COMMIT_DELAY_MS);
+            } catch (InterruptedException e) {
+                Thread.currentThread().interrupt();
+                throw new RuntimeException("Interrupted while waiting for transaction commit", e);
+            }
+        }
+    }


Why do we need this? I believe the event checker by default wait till it got the required event.

adamsaghy · 2025-11-24T13:43:01Z

fineract-e2e-tests-core/src/test/java/org/apache/fineract/test/stepdef/hook/MessagingHook.java

+    private static final Duration STARTUP_TIMEOUT = Duration.ofSeconds(20);
+
+    @Before(order = 0)
+    public void waitForJmsListenerStartup() {


Do we need readiness check two different places? I think it was already done once in ExternalEventSuiteInitializer, no?

budaidev force-pushed the FINERACT-2380/e2e-to-feign branch 6 times, most recently from ebd35e9 to 975c81c Compare November 19, 2025 12:39

budaidev marked this pull request as ready for review November 19, 2025 13:46

budaidev force-pushed the FINERACT-2380/e2e-to-feign branch 3 times, most recently from e57c6b4 to dde008c Compare November 19, 2025 20:32

galovics reviewed Nov 19, 2025

View reviewed changes

budaidev force-pushed the FINERACT-2380/e2e-to-feign branch 2 times, most recently from 9698a3d to 475e56b Compare November 20, 2025 23:26

budaidev requested a review from galovics November 21, 2025 13:04

adamsaghy reviewed Nov 21, 2025

View reviewed changes

budaidev force-pushed the FINERACT-2380/e2e-to-feign branch 2 times, most recently from a7b9637 to acc2b50 Compare November 23, 2025 14:43

adamsaghy reviewed Nov 24, 2025

View reviewed changes

budaidev force-pushed the FINERACT-2380/e2e-to-feign branch from acc2b50 to c0ee888 Compare November 24, 2025 13:22

adamsaghy reviewed Nov 24, 2025

View reviewed changes

FINERACT-2380: move retrofit client based test to feign

d48889b

budaidev force-pushed the FINERACT-2380/e2e-to-feign branch from c0ee888 to d48889b Compare November 24, 2025 14:29

FINERACT-2380: move retrofit client based test to feign #5164

Are you sure you want to change the base?

FINERACT-2380: move retrofit client based test to feign #5164

Uh oh!

Conversation

budaidev commented Nov 18, 2025

Description

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsaghy Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adamsaghy Nov 24, 2025 •

edited

Loading