Skip to content

Refactor FormServiceTest by splitting shouldFormCreateUpdateDelete into isolated tests#6228

Open
Saimandsroy wants to merge 1 commit into
openmrs:masterfrom
Saimandsroy:refactor-form-service-tests
Open

Refactor FormServiceTest by splitting shouldFormCreateUpdateDelete into isolated tests#6228
Saimandsroy wants to merge 1 commit into
openmrs:masterfrom
Saimandsroy:refactor-form-service-tests

Conversation

@Saimandsroy

Copy link
Copy Markdown

Summary
This pull request refactors the existing shouldFormCreateUpdateDelete test in FormServiceTest into four focused and independent test methods, addressing an inline FIXME comment.

Changes

  • Added shouldCreateForm
  • Added shouldUpdateForm
  • Added shouldRetireAndUnretireForm
  • Added shouldDeleteForm
  • Removed the monolithic test method

Verification

  • Successfully built the project with Maven.
  • Ran the FormServiceTest test suite successfully.
  • All tests passed without failures.

@sonarqubecloud

Copy link
Copy Markdown

@jwnasambu jwnasambu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Saimandsroy ! Thanks for the contribution. Per our PR Tips, please update the PR description to include a link to the related Jira issue. This helps us track and review your work more effectively. Thanks!

@dkayiwa

dkayiwa commented Jul 7, 2026

Copy link
Copy Markdown
Member

@claude review

1 similar comment
@dkayiwa

dkayiwa commented Jul 7, 2026

Copy link
Copy Markdown
Member

@claude review

@dkayiwa

dkayiwa commented Jul 7, 2026

Copy link
Copy Markdown
Member

@claude review

*/
@Test
public void shouldFormCreateUpdateDelete() {
public void shouldCreateForm() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this lands, worth knowing that three of these four new tests recreate coverage that already exists as dedicated tests further down this same file, so they land as duplicates rather than new coverage:

  • shouldCreateForm overlaps saveForm_shouldSaveGivenFormSuccessfully (line 1017)
  • shouldUpdateForm overlaps saveForm_shouldUpdateAnExistingForm (line 1035)
  • shouldDeleteForm is essentially identical to purgeForm_shouldDeleteGivenFormSuccessfully (line 1098): the same getForm(1), purgeForm(form), assertNull(getForm(1)) sequence.

Only shouldRetireAndUnretireForm adds something not already covered by a standalone test, since retire/unretire was previously exercised only inside the monolithic test.

I'd suggest keeping shouldRetireAndUnretireForm and the removal of shouldFormCreateUpdateDelete, and dropping the other three, since the existing saveForm_* and purgeForm_* tests already cover create/update/delete (and follow the project's methodName_shouldExpectedBehavior naming). That still resolves the FIXME without adding redundant tests. This is a suggestion, not a blocker.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.41%. Comparing base (5167075) to head (ff47345).
⚠️ Report is 15 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #6228      +/-   ##
============================================
+ Coverage     59.37%   59.41%   +0.03%     
- Complexity     9353     9361       +8     
============================================
  Files           695      695              
  Lines         37471    37471              
  Branches       5521     5521              
============================================
+ Hits          22250    22264      +14     
+ Misses        13225    13199      -26     
- Partials       1996     2008      +12     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

4 participants