Skip to content

Conversation

@kshinde3110
Copy link

@kshinde3110 kshinde3110 commented Jan 22, 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

  • New E2E test suite:
    • ExplorePageRightPanel.spec.ts covers Overview, Schema, Lineage, Data Quality, and Custom Properties tabs
  • New Page Object classes:
    • OverviewPageObject, SchemaPageObject, LineagePageObject, DataQualityPageObject, CustomPropertiesPageObject, and RightPanelPageObject implement fluent API pattern
  • New utility function:
    • navigateToExploreAndSelectEntity in entityPanel.ts handles explore page navigation with asset type selection

This will update automatically on new commits.


@github-actions
Copy link
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@gitar-bot
Copy link

gitar-bot bot commented Jan 22, 2026

Code Review 👍 Approved with suggestions 0 resolved / 5 findings

New E2E test suite with well-structured Page Object pattern; has some robustness issues around error handling and locator scoping that should be addressed.

⚠️ Edge Case: Empty catch blocks silently swallow test failures

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/ExplorePageRightPanel.spec.ts:204

The test file has multiple empty catch blocks that silently swallow exceptions, which can hide actual test failures and make debugging difficult. In lines 204-208, 210-214, 240-242, 244-246, the try-catch blocks catch errors but do nothing with them.

For example:

try {
  await lineage.clickUpstreamButton();
  await lineage.shouldShowExpandUpstreamButton();
} catch {
  // Expected if no upstream lineage exists
  await lineage.shouldNotShowExpandUpstreamButton();
}

This pattern masks actual bugs - if clickUpstreamButton() fails due to an unexpected reason (like a selector bug or race condition), the test won't report the failure.

Suggested fix: Use explicit conditional checks instead of try-catch, or at minimum verify the expected state before attempting the action:

const hasUpstreamButton = await lineage.isUpstreamButtonVisible();
if (hasUpstreamButton) {
  await lineage.clickUpstreamButton();
  await lineage.shouldShowExpandUpstreamButton();
} else {
  await lineage.shouldNotShowExpandUpstreamButton();
}

Alternatively, if using try-catch, verify the error type matches expectations.

More details 💡 4 suggestions
💡 Quality: `valueElement` locator not scoped to specific property card

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/PageObject/Explore/OverviewPageObject.ts:119

In CustomPropertiesPageObject.shouldShowCustomPropertyWithValue(), the valueElement locator is not properly scoped to the found propertyCard. The code filters to find the correct property card but then uses this.valueElement which references ALL value elements, not the one inside the specific property card.

async shouldShowCustomPropertyWithValue(propertyName: string, expectedValue: string): Promise<void> {
  const propertyCard = this.propertyCard.filter({ hasText: propertyName });
  await propertyCard.waitFor({ state: 'visible' });

  await this.valueElement.waitFor({ state: 'visible' }); // Wrong: not scoped to propertyCard
  const actualValue = await this.valueElement.textContent();
  // ...
}

Suggested fix: Scope the value lookup to the found property card:

async shouldShowCustomPropertyWithValue(propertyName: string, expectedValue: string): Promise<void> {
  const propertyCard = this.propertyCard.filter({ hasText: propertyName });
  await propertyCard.waitFor({ state: 'visible' });

  const valueElement = propertyCard.locator('.value, [class*="value"], [data-testid="value"]');
  await valueElement.waitFor({ state: 'visible' });
  const actualValue = await valueElement.textContent();
  // ...
}
💡 Bug: `shouldShowSchemaFields` assertion mismatch with actual behavior

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/PageObject/Explore/SchemaPageObject.ts:100

The shouldShowSchemaFields() method in SchemaPageObject throws an error if there are no schema fields, but the test in ExplorePageRightPanel.spec.ts (line 176) uses it with the comment "Should not fail even if empty". This creates a contradiction - the method will throw if schema is empty, contradicting the test's intention.

Code:

// SchemaPageObject.ts
async shouldShowSchemaFields(): Promise<void> {
  const fieldCards = this.fieldCard;
  const count = await fieldCards.count();
  if (count === 0) {
    throw new Error('Should show at least one schema field but shows none');
  }
}

// ExplorePageRightPanel.spec.ts
await schema.shouldShowSchemaFields(); // Should not fail even if empty

Suggested fix: Either:

  1. Rename the method to clarify its behavior (e.g., shouldHaveAtLeastOneField)
  2. Update the test to use a different assertion method
  3. Create a separate maybeShowSchemaFields() method that doesn't throw on empty
💡 Quality: console.warn usage in production test code

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/PageObject/Explore/RightPanelPageObject.ts:228 📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/PageObject/Explore/RightPanelPageObject.ts:278

The runEntitySpecificVerification() method uses console.warn() which is typically discouraged by ESLint rules in this project (as mentioned in CLAUDE.md: "Follow ESLint rules strictly - the project enforces no-console").

public async runEntitySpecificVerification(): Promise<void> {
  // ...
  if (!exists) {
    console.warn(`Tab "${tab}" not found for entity type ${this.entityConfig?.entityType}`);
  }
}

Suggested fix: Either:

  1. Use the project's logging utilities if available
  2. Throw an error with proper test failure reporting
  3. Add an eslint-disable comment with justification if intentional
💡 Edge Case: Hard-coded wait time for entity indexing

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/ExplorePageRightPanel.spec.ts:73

The test uses a hard-coded setTimeout of 3000ms to wait for entities to be indexed. This is fragile - in slower CI environments or under load, indexing might take longer; in faster environments, it wastes time.

// Wait for entities to be indexed
await new Promise(resolve => setTimeout(resolve, 3000));

Suggested fix: Use a polling approach that waits for the entity to actually be indexed:

await page.waitForResponse(
  (response) => 
    response.url().includes('/api/v1/search/query') && 
    response.status() === 200,
  { timeout: 10000 }
);

Or implement a utility function that polls the search API until the entity appears.

What Works Well

Clean implementation of the Page Object Model pattern with fluent interface for method chaining. Comprehensive coverage of right panel tabs (Overview, Schema, Lineage, Data Quality, Custom Properties). Good configuration-driven approach for different entity types with DATA_ASSET_CONFIGS mapping.

Recommendations

Consider adding explicit visibility checks (e.g., isUpstreamButtonVisible()) to Page Objects to enable proper conditional test logic instead of relying on try-catch for control flow. This will make tests more deterministic and easier to debug.

Rules 🎸 1 action taken

Gitar Rules

🎸 Summary Enhancement: Added technical summary for new PR with template-only description

2 rules not applicable. Show all rules by commenting gitar display:verbose.

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


// Test upstream/downstream buttons (may not be visible if no lineage exists)
try {
await lineage.clickUpstreamButton();
Copy link

Choose a reason for hiding this comment

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

⚠️ Edge Case: Empty catch blocks silently swallow test failures

Details

The test file has multiple empty catch blocks that silently swallow exceptions, which can hide actual test failures and make debugging difficult. In lines 204-208, 210-214, 240-242, 244-246, the try-catch blocks catch errors but do nothing with them.

For example:

try {
  await lineage.clickUpstreamButton();
  await lineage.shouldShowExpandUpstreamButton();
} catch {
  // Expected if no upstream lineage exists
  await lineage.shouldNotShowExpandUpstreamButton();
}

This pattern masks actual bugs - if clickUpstreamButton() fails due to an unexpected reason (like a selector bug or race condition), the test won't report the failure.

Suggested fix: Use explicit conditional checks instead of try-catch, or at minimum verify the expected state before attempting the action:

const hasUpstreamButton = await lineage.isUpstreamButtonVisible();
if (hasUpstreamButton) {
  await lineage.clickUpstreamButton();
  await lineage.shouldShowExpandUpstreamButton();
} else {
  await lineage.shouldNotShowExpandUpstreamButton();
}

Alternatively, if using try-catch, verify the error type matches expectations.


Was this helpful? React with 👍 / 👎

@github-actions
Copy link
Contributor

github-actions bot commented Jan 22, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 65%
65.71% (55122/83885) 44.68% (28383/63531) 47.64% (8662/18181)

@sonarqubecloud
Copy link

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 UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants