Skip to content

Conversation

@mohityadav766
Copy link
Member

@mohityadav766 mohityadav766 commented Jan 26, 2026

Describe your changes:

Fixes

I worked on ... because ...

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

Summary by Gitar

  • Fixed flaky tests:
    • Enhanced test_listUsersWithAdminFilter in UserResourceIT.java with pagination logic to search across all pages
    • Created findUserInPaginatedResults helper method to centralize pagination filtering
  • Applied pagination fix:
    • test_listUsersWithBotFilter, test_listUsersWithTeamFilter, and test_listUsersWithDomainFilter now use centralized pagination
  • Enhanced diagnostics:
    • Added direct fetch validation and detailed failure messages with pagination metadata for admin filter test
  • New boundary test:
    • test_listUsersWithAdminFilter_paginationBoundary creates 101 users to verify filter works across page boundaries

This will update automatically on new commits.


pmbrull
pmbrull previously approved these changes Jan 26, 2026
@gitar-bot
Copy link

gitar-bot bot commented Jan 26, 2026

Code Review 👍 Approved with suggestions 0 resolved / 2 findings

Solid pagination improvements for flaky tests. Two minor issues from previous review remain: redundant assertTrue(true) and test cleanup concerns.

💡 Quality: Redundant assertTrue(true) assertion

📄 openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/UserResourceIT.java:165

The assertion assertTrue(true, "User correctly found via pagination as expected") at lines 165-168 is a no-op that always passes. This provides no test value and is essentially dead code.

Suggestion: Either remove this block entirely, or if you want to log a message when the expected pagination scenario occurs, use a logger or add a test comment. The conditional block itself provides no meaningful assertion:

// Remove this entire block:
if (!foundInFirstPage && totalPagesSearched > 1) {
  assertTrue(true, "User correctly found via pagination as expected");
}

Or if you want to explicitly document the expected behavior in test output, consider using a logging statement or JUnit's System.out.println for test debugging.

💡 Performance: Test creates 101 users without explicit cleanup

📄 openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/UserResourceIT.java:83

The new pagination boundary test creates 101 users in a loop (lines 83-92), but there's no cleanup of these users after the test completes. While the TestNamespace provides unique prefixes to avoid name collisions, leaving behind 101 test users per test run could:

  1. Slow down the test database over time
  2. Affect future test runs if tests query for total user counts
  3. Cause resource buildup in CI environments

Suggestion: Consider adding cleanup in a @AfterEach method or wrap the user creation in a try-finally block that cleans up the created users. Alternatively, if the test framework handles cleanup automatically (e.g., transaction rollback), add a comment documenting this.

// Example: Track created users for cleanup
List<UUID> createdUserIds = new ArrayList<>();
for (int i = 0; i < usersToCreate; i++) {
    User created = createEntity(createRequest);
    createdUserIds.add(created.getId());
}
// ... in cleanup or finally block:
// createdUserIds.forEach(id -> deleteEntity(id.toString()));
Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@mohityadav766 mohityadav766 merged commit 8145593 into main Jan 26, 2026
22 checks passed
@mohityadav766 mohityadav766 deleted the fix-ci-failure-user-test branch January 26, 2026 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants