Skip to content

Added fixme for the audit log tests#26020

Merged
chirag-madlani merged 2 commits intomainfrom
audit-log-e2e-fiz
Feb 20, 2026
Merged

Added fixme for the audit log tests#26020
chirag-madlani merged 2 commits intomainfrom
audit-log-e2e-fiz

Conversation

@Rohit0301
Copy link
Copy Markdown
Contributor

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.

});

test('should search audit logs', async ({ page }) => {
test.fixme('should search audit logs', async ({ page }) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Quality: Missing reason/tracking issue for fixme-marked tests

Four tests across two describe blocks are marked as fixme but there's no inline comment or linked issue explaining why they're failing. Playwright's fixme() accepts an optional description string (e.g., test.fixme('reason') or as a second argument) and the PR description uses a placeholder Fixes <issue-number>.

Without context, future developers won't know:

  • What failure these tests exhibit
  • Whether there's a tracking issue for re-enabling them
  • If the root cause is in the test code or the application

This risks the fixme markers becoming permanent technical debt.

Suggested fix:

  test.fixme('should search audit logs', async ({ page }) => {
    // TODO: Skipped due to <issue-link> - search API returns inconsistent results

Was this helpful? React with 👍 / 👎

@chirag-madlani chirag-madlani merged commit 07189f2 into main Feb 20, 2026
13 checks passed
@chirag-madlani chirag-madlani deleted the audit-log-e2e-fiz branch February 20, 2026 14:34
await redirectToHomePage(page);
await settingClick(page, GlobalSettingOptions.AUDIT_LOGS);
// Audit log search api has very high latency due to which the test is getting timeout
test.fixme(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Quality: Redundant test.fixme() inside test.describe.fixme() block

The test 'should include filters and search in export request' at line 816 is individually wrapped with test.fixme(), but it's already inside the test.describe.fixme() block (line 690). When a describe block is marked as fixme, all tests within it are automatically skipped. The inner test.fixme() wrapper (and the associated comment and indentation changes) are unnecessary.

This won't cause any functional issue, but removing the redundant wrapper would simplify the code and avoid the indentation churn (lines 815-882).

Suggested fix:

    test(
      'should include filters and search in export request',
      async ({ page }) => {

Was this helpful? React with 👍 / 👎

@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented Feb 20, 2026

Code Review 👍 Approved with suggestions 0 resolved / 2 findings

Test-only change correctly disables flaky tests, but still lacks a tracking issue for re-enablement, and one test.fixme() is redundant inside an already-fixme'd describe block.

💡 Quality: Redundant test.fixme() inside test.describe.fixme() block

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/AuditLogs.spec.ts:816

The test 'should include filters and search in export request' at line 816 is individually wrapped with test.fixme(), but it's already inside the test.describe.fixme() block (line 690). When a describe block is marked as fixme, all tests within it are automatically skipped. The inner test.fixme() wrapper (and the associated comment and indentation changes) are unnecessary.

This won't cause any functional issue, but removing the redundant wrapper would simplify the code and avoid the indentation churn (lines 815-882).

Suggested fix
    test(
      'should include filters and search in export request',
      async ({ page }) => {
💡 Quality: Missing reason/tracking issue for fixme-marked tests

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/AuditLogs.spec.ts:401 📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/AuditLogs.spec.ts:687 📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/AuditLogs.spec.ts:812

Four tests across two describe blocks are marked as fixme but there's no inline comment or linked issue explaining why they're failing. Playwright's fixme() accepts an optional description string (e.g., test.fixme('reason') or as a second argument) and the PR description uses a placeholder Fixes <issue-number>.

Without context, future developers won't know:

  • What failure these tests exhibit
  • Whether there's a tracking issue for re-enabling them
  • If the root cause is in the test code or the application

This risks the fixme markers becoming permanent technical debt.

Suggested fix
  test.fixme('should search audit logs', async ({ page }) => {
    // TODO: Skipped due to <issue-link> - search API returns inconsistent results
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

Rohit0301 added a commit that referenced this pull request Feb 20, 2026
* Added fixme for the audit log tests

* added comment
@github-actions
Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 65%
65.77% (56483/85884) 45.21% (29573/65419) 48.03% (8924/18581)

@sonarqubecloud
Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

2 participants