Skip to content

FINERACT-2746: migrate organisation tests to feign - #6246

Open
DeathGun44 wants to merge 3 commits into
apache:developfrom
DeathGun44:FINERACT-2746/migrate-organisation-tests-to-feign
Open

FINERACT-2746: migrate organisation tests to feign#6246
DeathGun44 wants to merge 3 commits into
apache:developfrom
DeathGun44:FINERACT-2746/migrate-organisation-tests-to-feign

Conversation

@DeathGun44

Copy link
Copy Markdown
Contributor

Description

Migrates five organisation and reference-data integration test classes from REST-assured onto the generated Feign client.

Class Tests
OfficeIntegrationTest 3
HookIntegrationTest 2 (1 disabled, see gaps)
ExternalEventConfigurationIntegrationTest 2
RatesTest 1
ProductMixIntegrationTest 1

No production code is changed. Every file is under integration-tests/, so the OpenAPI spec is untouched and checkBreakingChanges cannot regress. The PR shares no file with any other in-flight migration branch and can be merged in any order relative to them.

Helpers

  • HookHelper and RatesHelper are deleted; ProductMixHelper moves to client/feign/helpers/FeignProductMixHelper. None had a caller outside its own test.
  • ExternalEventConfigurationHelper loses 788 lines - a hand-maintained catalogue of the 146 default event configurations plus two helpers, all three with no callers left. Its two live methods stay; three other tests still use them.
  • OfficeHelper is untouched: 20 other tests still depend on it.
  • FeignExternalEventHelper now delegates its two configuration toggles to FeignExternalEventConfigurationHelper, so one class owns that endpoint.

Notable changes beyond the transport swap

  • The 146 expected defaults collapse from 788 lines of maps to a set of type names in ExternalEventConfigurationTestData. Keeping the catalogue duplicated is deliberate: adding or removing an event type should fail this test until the set is updated. (The previous list had drifted out of date against the Liquibase seed.)
  • The external-event update test re-reads the configuration afterwards, rather than trusting only the echoed changes map.
  • RatesTest additionally asserts the new rate appears in the listing, and checks the update's changes payload against both the requested percentage and the persisted one.
  • FeignHookHelper locates the payload URL by field name instead of by list position - the server promises no order there, so config.get(1) happened to work rather than being correct. Field names come from the server's own HookApiConstants, so a rename breaks the build instead of the test.
  • HookIntegrationTest deletes its hook in @AfterEach. Previously a failed assertion could leave an active OFFICE/CREATE webhook registered, making every later test that creates an office fire an outbound dispatch.

Known gaps left in place, each needing its own ticket

Pre-existing, called out rather than papered over:

  1. CashierSummaryAndTransactionsTest is not migrated. It was in scope until the typed client turned out to be unable to create a teller. TellerApiResourceSwagger.PostTellersRequest.status is declared as TellerStatus, so the generated model sends "ACTIVE", but Teller.fromJson reads the field with integerValueOfParameterNamed("status") and expects an int. The server answers 400 error.msg.invalid.json.data. Correcting the DTO is production code and a breaking OpenAPI change, so it does not belong here.
  2. ProductMixIntegrationTest can only assert that its two calls returned 200. The listing call does not send associations=productMixes, so it returns loan productsrather than product mixes; and with isProductMixTemplate=true the endpoint answers {"productOptions": [...]}, which the declared GetLoanProductsTemplateResponse has no field for, leaving every property null. Both are unchanged from before the migration and both need LoanProductsApiResourceSwagger changes to close.
  3. shouldSendOfficeCreationNotification is @Disabled. It polls echo-webhook.herokuapp.com, offline since Heroku retired free dynos in 2022, so the loop always timed out and the test passed having asserted nothing - while still registering a hook and making the server attempt an outbound POST to a dead host on every CI run. The code is kept for whoever supplies a replacement echo endpoint.

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Write the commit message as per our guidelines
  • 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 our 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". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.
  • 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.

OfficeIntegrationTest now uses the existing FeignOfficeHelper, which already
exposed every call the test makes. The legacy OfficeHelper it used is a near
duplicate of that helper, but it stays in place because eighteen other tests
still depend on it.

RatesHelper and ProductMixHelper were already Feign backed, but they sat outside
the client/feign tree and their tests still carried REST-assured scaffolding that
was never used. Rates moves to a conventional FeignRateHelper plus
RateRequestBuilders; product mix is two calls and is inlined. Both legacy helpers
are deleted, since neither had a caller other than its own test.

Two assertions were tightened while the tests were being rewritten. RatesTest now
checks that the rate it creates actually appears in the rate listing, rather than
only that the listing is non-null, and it compares the updated percentage against
the value its request carried instead of reading it back out of the untyped
changes map. ProductMixIntegrationTest asserts the template offers at least one
currency, because a bare non-null check on a deserialized response only proves the
call returned 200.

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
HookHelper was already Feign backed, so this mostly moves it into the client/feign
tree as FeignHookHelper, an instance helper built on the client like the others.
Its verify* methods were assertions rather than transport, so they move into the
test that owns them, and the legacy helper is deleted; HookIntegrationTest was its
only caller.

The payload URL is now located by field name rather than by position in the
configuration list. The server does not promise an order there, so config.get(1)
happened to work rather than being correct.

The echo-webhook.herokuapp.com endpoint that shouldSendOfficeCreationNotification
polls has been offline since Heroku retired free dynos in 2022, so the loop always
times out and the test finishes without asserting delivery. That is preserved
as-is: only the transport changes here, from REST-assured to the JDK HTTP client.
Restoring real coverage needs a replacement echo service and its own ticket.

CashierSummaryAndTransactionsTest was originally in scope for this commit and has
been left on REST-assured. Creating a teller through the typed client is not
currently possible: TellerApiResourceSwagger declares status as TellerStatus, so
the generated model sends "ACTIVE", while Teller.fromJson reads the field with
integerValueOfParameterNamed and wants 300. The server answers 400
error.msg.invalid.json.data. Correcting the DTO to Integer is production code and
would be a breaking OpenAPI change, so it needs a separate ticket.

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
ExternalEventConfigurationHelper spelled out its expected defaults as 146 maps
across 847 lines. Every one of those entries is disabled, so the whole expectation
collapses to a set of type names in ExternalEventConfigurationTestData. The legacy
helper stays where it is; four other tests still use it.

Keeping the catalogue duplicated here is deliberate: adding or removing an
external event type should fail this test until the set is updated to match.

The update test now re-reads the configuration afterwards to confirm the change
actually landed, instead of trusting only the changes map echoed back by the
command.

Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants