Skip to content

FINERACT-2494: Add unit tests for ApiParameterHelper in fineract-core.#5677

Open
San-43 wants to merge 1 commit intoapache:developfrom
San-43:FINERACT-2494-api-parameter-helper-tests
Open

FINERACT-2494: Add unit tests for ApiParameterHelper in fineract-core.#5677
San-43 wants to merge 1 commit intoapache:developfrom
San-43:FINERACT-2494-api-parameter-helper-tests

Conversation

@San-43
Copy link

@San-43 San-43 commented Mar 22, 2026

Description

This PR fixes FINERACT-2494.

New test coverage for API parameter extraction and parsing:

  • Added ApiParameterHelperTest with tests for extracting and validating commandId, fields, associations, and locale from query parameters, including handling of missing, blank, invalid, and duplicate values.
  • Included tests for boolean query parameter parsing for methods such as parameterType, template, makerCheckerable, includeJson, and genericResultSet, verifying correct handling of true, false, and missing values.
  • Provided tests for exclusion logic in associations and for presence checks on the genericResultSet flag.
  • Added a helper method to simplify test parameter creation, improving readability and maintainability of the test code.

Checklist

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

Your assigned reviewer(s) will follow our guidelines for code reviews.

@Ambika-Sony
Copy link
Contributor

Ambika-Sony commented Mar 23, 2026

Hi @San-43 , I see you're working on this issue!

I’ve already submitted PR #5665 addressing FINERACT-2494. My approach focuses on building a comprehensive unit test suite for ApiParameterHelper, including coverage for edge cases such as null handling, negative offsets, malformed inputs, and boundary conditions.

The tests are designed as isolated JUnit 5 unit tests to ensure reliability and maintainability within fineract-core, and I’m also aligning with static analysis improvements from FINERACT-2510.

Happy to align approaches or incorporate any feedback from the community.

@San-43
Copy link
Author

San-43 commented Mar 23, 2026

Hi @Ambika-Sony, thanks for the note and for your work on this. I actually hadn’t noticed #5665 when I started #5677. I saw a couple of closed PR targeting FINERACT-2494, but I missed #5665, apologies for that.

After comparing #5665 and #5677, my view is that #5677 is a bit cleaner for FINERACT-2494 because it keeps the change strictly tests-only, in line with the ticket scope. In #5665, there are also ApiParameterHelper production-code changes; those hardening updates are valuable, but they may be better handled in a separate PR/ticket so scope and review impact stay clear.

I’m happy to align and avoid duplication. If maintainers prefer a single path, I can either:

  1. close this PR, or
  2. keep this one for tests-only and leave behavior changes to a follow-up PR.

Happy to follow maintainer guidance either way.

@Ambika-Sony
Copy link
Contributor

Hi @Ambika-Sony, thanks for the note and for your work on this. I actually hadn’t noticed #5665 when I started #5677. I saw a couple of closed PR targeting FINERACT-2494, but I missed #5665, apologies for that.

After comparing #5665 and #5677, my view is that #5677 is a bit cleaner for FINERACT-2494 because it keeps the change strictly tests-only, in line with the ticket scope. In #5665, there are also ApiParameterHelper production-code changes; those hardening updates are valuable, but they may be better handled in a separate PR/ticket so scope and review impact stay clear.

I’m happy to align and avoid duplication. If maintainers prefer a single path, I can either:

  1. close this PR, or
  2. keep this one for tests-only and leave behavior changes to a follow-up PR.

Happy to follow maintainer guidance either way.

Hi @San-43,

Regarding the scope: the 'production-code changes' in #5665 weren't just extra additions; they are hardening fixes specifically required to address NullPointerExceptions and boundary errors that my 22+ test cases exposed. A tests-only PR (#5677) would pass on the current code but would leave these underlying stability issues unaddressed.

I believe it's critical for the project to not only have tests but to resolve the vulnerabilities those tests reveal. I'm happy to let the maintainers decide the best path forward, but I will be keeping #5665 active as a comprehensive solution for FINERACT-2494 that ensures both coverage and core stability.

@San-43 San-43 force-pushed the FINERACT-2494-api-parameter-helper-tests branch from 91aa040 to a8a81b2 Compare March 26, 2026 07:25
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