+1 This moves things along w test feign. Can we get a review this week?
(GSOC program is completing this week) Sent from Gmail Mobile On Mon, Aug 24, 2026 at 8:26 AM KRISHNA MEWARA <[email protected]> wrote: > Hi all, > > I have raised FINERACT-2781 [1] with a pull request [2] that adds a > documentation chapter for writing integration tests against the generated > Feign client, plus a Checkstyle rule to go with it. I would like feedback > before this is merged, particularly on the documentation. > Background > > Over the last few months, as a GSoC contributor, I have been working on > Feign migration, and several pull requests have migrated integration tests > from REST Assured to the generated Feign client [4]. During those reviews, > the same points came up repeatedly: fix a missing model field in the > Swagger DTO instead of working around it in the test, assert the specific > error code rather than just that a call failed, do not weaken a > verification because the generated model lacks a field, and so on. > > At the moment, that guidance exists only in pull request comments. A new > contributor has no way to find it, and reviewers have to repeat the same > explanations. > What the Pull Request Contains > > 1. > > *A new chapter, "Feign Integration Tests":* Covers how a test reaches > the client, how to assert failures, and what to do when a generated model > is missing a field. The conventions are written as a numbered list (IT-01 > to IT-20) so a review comment can point directly to a rule instead of > restating it. > 2. > > *A Checkstyle rule:* Rejects REST Assured imports in the > integrationtests.client.feign package, which holds the base classes > and helpers every migrated test inherits from. It reuses > ForbidCertainImportsCheck, which the project already uses twice, and > runs in the existing checkstyleTest job. There is no new tooling or > workflow required. > 3. > > *Corrections to the existing integration testing chapter:* While > writing the new one, I found that the current docs outline four base class > methods that do not exist, three Gradle options the test task does not > accept, and it still teaches the REST Assured setup block as the way to > start a new test. It also does not mention -PcargoDisabled, which is > required to run against your own instance. > > *Note: There are no production or test code changes in this PR.* > Please feel free to check it out and look into > > - > > *Rule accuracy:* Are the rules in the new chapter the right ones? I > have deliberately kept them to points that came out of actual review > feedback, but I may have included something that is only my own preference > or missed something that matters more. > - > > *Formatting:* Is the numbered rule format (IT-01, IT-02, etc.) useful, > or is it more structure than this needs? > - > > *Checkstyle scope:* The Checkstyle rule covers the shared base classes > and helpers, not the migrated tests themselves. Most of those live in the > root integrationtests package alongside the tests that still use REST > Assured, and Checkstyle can only scope by package, not by parent class. I > would rather guard what is realistic today than exclude files to make a > wider rule pass, but I am open to other suggestions. > > *One note on ordering:* The Checkstyle rule reports five violations > against develop today, all in FeignLoanHelper. Those are removed by #6321 > [3], which is already open. This pull request should go in after that one > is merged. > > Comments on the pull request are welcome and probably easier to follow > than the mailing list for the detailed points. > > Thanks, Krishna > > [1] https://issues.apache.org/jira/browse/FINERACT-2781 > [2] https://github.com/apache/fineract/pull/6324 > [3] https://github.com/apache/fineract/pull/6321 > [4] https://issues.apache.org/jira/browse/FINERACT-2609 >
