Add: verification for GitHub Actions integration as part of the CI in the flow #191
Conversation
WalkthroughThis PR introduces GitHub Actions CI support to the testing framework by adding a new GithubActionsPlugin, exporting a page object for the Actions tab selector, integrating GitHub Actions case handling in the CI factory, and improving duplicate heading detection in the waitForPageLoad utility function. Changes
Sequence DiagramsequenceDiagram
participant Test as Test Suite
participant Factory as CIFactory
participant Plugin as GithubActionsPlugin
participant Page as Playwright Page
Test->>Factory: createCiPlugin(name, registryOrg, GITHUB_ACTIONS)
Factory->>Plugin: new GithubActionsPlugin(name, registryOrg)
Plugin-->>Factory: instance
Factory-->>Test: GithubActionsPlugin
Test->>Plugin: checkPipelineRunsTable(page)
activate Plugin
Plugin->>Page: navigate to View Source link
Plugin->>Page: open repository in new tab
Plugin->>Plugin: buildActionsUrl(repoUrl)
Plugin->>Page: verify/navigate to Actions URL
Plugin->>Page: assert URL matches expected
Plugin->>Page: close tab
deactivate Plugin
Plugin-->>Test: verification complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes span four related files introducing a new CI plugin feature. The factory and page object changes are straightforward, but the GithubActionsPlugin implementation requires careful review of the Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
BohdanMar
force-pushed
the
feature/github-actions-ci-verification
branch
from
6c3c9ef to
07f715b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/ui/commonUi.ts (1)
50-55: Simplify the redundant filter in heading visibility check.The condition handles multiple headings, but line 52's
.filter({ hasText: name })is redundant—getByRole('heading', { name })already matches by accessible name, which includes the text content. The filter doesn't narrow the selection further.More importantly, when multiple headings exist with the same name, silently selecting
.first()might mask a UI issue or select an unintended element (e.g., a hidden or off-screen heading). Consider whether the presence of duplicate headings indicates a problem that should be investigated.Apply this diff to remove the redundant filter:
const heading = page.getByRole('heading', { name }); if ((await heading.count()) > 1) { - await expect(heading.filter({ hasText: name }).first()).toBeVisible({ timeout: 20000 }); + await expect(heading.first()).toBeVisible({ timeout: 20000 }); } else { await expect(heading).toBeVisible({ timeout: 20000 }); }Alternatively, if duplicate headings are unexpected, consider logging a warning or failing the test when
count > 1to catch potential UI regressions.src/ui/plugins/ci/githubActionsPlugin.ts (3)
10-10: Document why checkCIHeading is empty.The base class
BaseCIPluginprovides an implementation that verifies the heading visibility. Overriding it with an empty method removes that functionality without explanation. If GitHub Actions integration doesn't display a CI heading (unlike Tekton), please add a comment explaining why this check is skipped to aid future maintainers.Example:
- public async checkCIHeading(_page: Page): Promise<void> {} + // GitHub Actions integration does not display a CI-specific heading + // in the Developer Hub UI, so this check is intentionally skipped. + public async checkCIHeading(_page: Page): Promise<void> {}
14-40: Strengthen GitHub Actions page verification.The implementation correctly navigates to the GitHub Actions page, but only verifies the URL without confirming that Actions content is actually visible or functional. This could lead to false positives if GitHub changes their page structure or if the Actions tab is empty/broken.
Consider enhancing the verification:
- Use the
GithubActionsPO.github.actionsTabselector (defined in the page object) to verify the Actions tab is visible- Check for presence of workflow runs, status indicators, or the Actions-specific UI elements
- Consider using
'networkidle'instead of'domcontentloaded'on line 30 and 34 if GitHub's Actions page loads content asynchronouslyExample enhancement after line 37:
// Verify Actions tab is active/visible import { GithubActionsPO } from '../../page-objects/githubActionsPo'; const actionsTab = githubPage.locator(GithubActionsPO.github.actionsTab); await expect(actionsTab).toBeVisible({ timeout: 10000 }); // Optionally verify workflow content exists const workflowsList = githubPage.locator('.workflow-list, [data-testid="workflow-list"]'); await expect(workflowsList).toBeAttached({ timeout: 10000 });
27-30: Consider timeout for new page event.The new page opening doesn't have an explicit timeout, which could cause the test to hang indefinitely if the link fails to open a new page. While Playwright has a default navigation timeout, it's better to be explicit for inter-context operations.
Apply this diff:
- const githubPagePromise = page.context().waitForEvent('page'); + const githubPagePromise = page.context().waitForEvent('page', { timeout: 30000 }); await viewSourceLink.click(); const githubPage = await githubPagePromise;src/ui/page-objects/githubActionsPo.ts (1)
1-5: Remove unusedGithubActionsPOpage object or document its intended purpose.The search confirms
GithubActionsPOis not imported or used anywhere in the codebase—it exists only in its definition file. For a testing framework, unused page objects reduce maintainability and can mislead developers about available test utilities. Either remove this unused export or document its planned integration if it's intended for future use.If the Actions tab selector will be used, integrate it into the plugin's verification flow (e.g.,
checkActionsmethod) where it belongs.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/ui/commonUi.ts(1 hunks)src/ui/page-objects/githubActionsPo.ts(1 hunks)src/ui/plugins/ci/ciFactory.ts(2 hunks)src/ui/plugins/ci/githubActionsPlugin.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
{src,tests}/ui/**/*.ts
📄 CodeRabbit inference engine (README.md)
{src,tests}/ui/**/*.ts: UI entities (classes/components) must include either "Ui" or "plugin" in their names to distinguish them from backend entities
When entering secrets in UI tests, blur the input field so secrets do not appear in screenshots
Files:
src/ui/page-objects/githubActionsPo.tssrc/ui/commonUi.tssrc/ui/plugins/ci/githubActionsPlugin.tssrc/ui/plugins/ci/ciFactory.ts
src/ui/page-objects/**/*Po.ts
📄 CodeRabbit inference engine (README.md)
Page object identifier files must reside in src/ui/page-objects and have a Po.ts suffix
Files:
src/ui/page-objects/githubActionsPo.ts
src/ui/**
⚙️ CodeRabbit configuration file
Focus on page object pattern implementation, element selection strategies, UI test stability, maintainability, proper wait conditions, error handling, and flaky test prevention. Ensure tests are resilient to UI changes.
Files:
src/ui/page-objects/githubActionsPo.tssrc/ui/commonUi.tssrc/ui/plugins/ci/githubActionsPlugin.tssrc/ui/plugins/ci/ciFactory.ts
src/ui/plugins/**/*.ts
📄 CodeRabbit inference engine (README.md)
src/ui/plugins/**/*.ts: Plugin-related functionality must be stored in src/ui/plugins, organized by plugin type (e.g., git, ci), and file names should match the plugin’s short name
Classes defined in plugin files must include either "Ui" or "plugin" in their class names
Files:
src/ui/plugins/ci/githubActionsPlugin.tssrc/ui/plugins/ci/ciFactory.ts
🧬 Code graph analysis (2)
src/ui/plugins/ci/githubActionsPlugin.ts (2)
src/ui/plugins/ci/baseCIPlugin.ts (1)
BaseCIPlugin(8-107)src/ui/page-objects/commonPo.ts (1)
GitPO(23-27)
src/ui/plugins/ci/ciFactory.ts (1)
src/ui/plugins/ci/githubActionsPlugin.ts (1)
GithubActionsPlugin(5-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / tssc-test-on-pull-request
🔇 Additional comments (3)
src/ui/plugins/ci/ciFactory.ts (1)
4-4: LGTM! Factory integration is clean.The GitHub Actions plugin is correctly integrated into the factory following the same pattern as the existing Tekton plugin. The import and case statement are properly structured.
Also applies to: 23-24
src/ui/plugins/ci/githubActionsPlugin.ts (2)
5-8: LGTM! Constructor properly delegates to base class.
42-45: LGTM! URL builder is clean and straightforward.The trailing slash handling and path appending logic is correct. Since
repoUrlcomes from a verifiedhrefattribute (validated as non-null on line 23-25), the assumption of a well-formed URL is reasonable.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/ui/commonUi.ts (1)
50-55: Simplify redundant filtering logic.The
filter({ hasText: name })on line 52 is redundant becausegetByRole('heading', { name })already performs an exact text match. When multiple headings match, the additional filter doesn't narrow the selection further—it just adds overhead.Consider simplifying:
const heading = page.getByRole('heading', { name }); if ((await heading.count()) > 1) { - await expect(heading.filter({ hasText: name }).first()).toBeVisible({ timeout: 20000 }); + await expect(heading.first()).toBeVisible({ timeout: 20000 }); } else { await expect(heading).toBeVisible({ timeout: 20000 }); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/ui/commonUi.ts(1 hunks)src/ui/page-objects/githubActionsPo.ts(1 hunks)src/ui/plugins/ci/ciFactory.ts(2 hunks)src/ui/plugins/ci/githubActionsPlugin.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
{src,tests}/ui/**/*.ts
📄 CodeRabbit inference engine (README.md)
{src,tests}/ui/**/*.ts: UI entities (classes/components) must include either "Ui" or "plugin" in their names to distinguish them from backend entities
When entering secrets in UI tests, blur the input field so secrets do not appear in screenshots
Files:
src/ui/plugins/ci/githubActionsPlugin.tssrc/ui/plugins/ci/ciFactory.tssrc/ui/page-objects/githubActionsPo.tssrc/ui/commonUi.ts
src/ui/plugins/**/*.ts
📄 CodeRabbit inference engine (README.md)
src/ui/plugins/**/*.ts: Plugin-related functionality must be stored in src/ui/plugins, organized by plugin type (e.g., git, ci), and file names should match the plugin’s short name
Classes defined in plugin files must include either "Ui" or "plugin" in their class names
Files:
src/ui/plugins/ci/githubActionsPlugin.tssrc/ui/plugins/ci/ciFactory.ts
src/ui/**
⚙️ CodeRabbit configuration file
Focus on page object pattern implementation, element selection strategies, UI test stability, maintainability, proper wait conditions, error handling, and flaky test prevention. Ensure tests are resilient to UI changes.
Files:
src/ui/plugins/ci/githubActionsPlugin.tssrc/ui/plugins/ci/ciFactory.tssrc/ui/page-objects/githubActionsPo.tssrc/ui/commonUi.ts
src/ui/page-objects/**/*Po.ts
📄 CodeRabbit inference engine (README.md)
Page object identifier files must reside in src/ui/page-objects and have a Po.ts suffix
Files:
src/ui/page-objects/githubActionsPo.ts
🧬 Code graph analysis (2)
src/ui/plugins/ci/githubActionsPlugin.ts (2)
src/ui/plugins/ci/baseCIPlugin.ts (1)
BaseCIPlugin(8-107)src/ui/page-objects/commonPo.ts (1)
GitPO(23-27)
src/ui/plugins/ci/ciFactory.ts (1)
src/ui/plugins/ci/githubActionsPlugin.ts (1)
GithubActionsPlugin(5-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / tssc-test-on-pull-request
🔇 Additional comments (3)
src/ui/plugins/ci/ciFactory.ts (1)
4-4: LGTM!Clean integration of the GitHub Actions plugin into the factory pattern, consistent with the existing Tekton case.
Also applies to: 23-24
src/ui/plugins/ci/githubActionsPlugin.ts (2)
10-10: Verify intentional empty implementations.These methods override
BaseCIPlugin's implementations with no-ops. The base class provides acheckCIHeadingimplementation that verifies heading visibility, which is now disabled for GitHub Actions CI.If these are placeholders, consider adding
// TODO:comments. If intentionally skipped, a brief comment explaining why GitHub Actions doesn't need these checks would improve maintainability.Also applies to: 12-12
42-45: LGTM - clean URL builder.The helper correctly handles trailing slashes. Assumes
repoUrldoesn't contain query parameters (reasonable given it's from a repository link'shrefattribute).
| export const GithubActionsPO = { | ||
| github: { | ||
| actionsTab: 'a[role="tab"][href$="/actions"]', | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify usage and consider naming consistency.
The page object GithubActionsPO is defined here but doesn't appear to be used in the corresponding GithubActionsPlugin implementation. Additionally, there's a naming inconsistency: the filename uses lowercase githubActionsPo.ts while the exported constant uses GithubActionsPO (uppercase G).
Run this script to verify whether this page object is actually used:
🏁 Script executed:
#!/bin/bash
# Search for usage of GithubActionsPO in the codebase
rg -n --type=ts 'GithubActionsPO' -C 2Length of output: 224
I need to verify the naming pattern used by other page objects in the codebase to determine if there's a genuine inconsistency issue.
🏁 Script executed:
#!/bin/bash
# Find all page object files and their naming patterns
fd -e ts . src/ui/page-objects 2>/dev/null | head -20Length of output: 262
🏁 Script executed:
#!/bin/bash
# Check exports from other page object files to verify naming consistency
rg -n 'export const' src/ui/page-objects/ --type=ts -A 1Length of output: 1087
🏁 Script executed:
#!/bin/bash
# Search for any imports of the githubActionsPo file
rg -n 'githubActionsPo' --type=ts -iLength of output: 538
🏁 Script executed:
#!/bin/bash
# Verify GithubActionsPO is not used anywhere in the codebase
rg -n 'GithubActionsPO|githubActionsPO|actionsTab' --type=tsLength of output: 169
Remove unused page object or integrate it into tests.
The GithubActionsPO constant is defined but never used anywhere in the codebase—it's dead code. Additionally, the export naming (GithubActionsPO) actually follows the established pattern used by other page objects (RegistryPO, TektonPO, DependenciesPO, CommonPO, AcsPO, GitPO), so there's no naming inconsistency to address.
For testing reliability and maintainability, either remove this unused page object or ensure the actionsTab selector is actively used in corresponding E2E tests. Keeping unused selectors in the codebase can create confusion about what's actually being tested.
🤖 Prompt for AI Agents
In src/ui/page-objects/githubActionsPo.ts lines 1-5, the GithubActionsPO export
defines an unused page object (actionsTab) that is dead code; either delete the
file and its export if no tests require the selector, or integrate it by
importing GithubActionsPO into the relevant E2E test(s) and replacing any
hard-coded selector strings with GithubActionsPO.github.actionsTab so the
selector is actively used; ensure after change you run the test suite to confirm
no references remain (if deleting) or that tests still pass (if integrating).
| public async checkPipelineRunsTable(page: Page): Promise<void> { | ||
| const overviewUrl = page.url().replace(/\/ci(?:\?.*)?$/, ''); | ||
| await page.goto(overviewUrl, { timeout: 20000 }); | ||
| await page.waitForLoadState('domcontentloaded'); | ||
|
|
||
| const viewSourceLink = page.locator(`${GitPO.githubLinkSelector}:has-text("${GitPO.viewSourceLinkText}")`); | ||
| await expect(viewSourceLink).toBeVisible({ timeout: 20000 }); | ||
|
|
||
| const repoUrl = await viewSourceLink.getAttribute('href'); | ||
| if (!repoUrl) { | ||
| throw new Error('Missing repository URL on View Source link'); | ||
| } | ||
|
|
||
| const githubPagePromise = page.context().waitForEvent('page'); | ||
| await viewSourceLink.click(); | ||
| const githubPage = await githubPagePromise; | ||
| await githubPage.waitForLoadState('domcontentloaded'); | ||
|
|
||
| const actionsUrl = this.buildActionsUrl(repoUrl); | ||
| if (!githubPage.url().startsWith(actionsUrl)) { | ||
| await githubPage.goto(actionsUrl, { timeout: 20000, waitUntil: 'domcontentloaded' }); | ||
| } | ||
|
|
||
| await expect(githubPage).toHaveURL(actionsUrl, { timeout: 20000 }); | ||
|
|
||
| await githubPage.close(); | ||
| } |
There was a problem hiding this comment.
Ensure proper cleanup on assertion failure.
If the URL assertion on line 37 fails, githubPage.close() on line 39 won't execute, leaving the page open and potentially causing resource leaks in the test run.
Wrap the page operations in a try-finally block:
const githubPagePromise = page.context().waitForEvent('page');
await viewSourceLink.click();
const githubPage = await githubPagePromise;
- await githubPage.waitForLoadState('domcontentloaded');
-
- const actionsUrl = this.buildActionsUrl(repoUrl);
- if (!githubPage.url().startsWith(actionsUrl)) {
- await githubPage.goto(actionsUrl, { timeout: 20000, waitUntil: 'domcontentloaded' });
- }
-
- await expect(githubPage).toHaveURL(actionsUrl, { timeout: 20000 });
-
- await githubPage.close();
+ try {
+ await githubPage.waitForLoadState('domcontentloaded');
+
+ const actionsUrl = this.buildActionsUrl(repoUrl);
+ if (!githubPage.url().startsWith(actionsUrl)) {
+ await githubPage.goto(actionsUrl, { timeout: 20000, waitUntil: 'domcontentloaded' });
+ }
+
+ await expect(githubPage).toHaveURL(actionsUrl, { timeout: 20000 });
+ } finally {
+ await githubPage.close();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async checkPipelineRunsTable(page: Page): Promise<void> { | |
| const overviewUrl = page.url().replace(/\/ci(?:\?.*)?$/, ''); | |
| await page.goto(overviewUrl, { timeout: 20000 }); | |
| await page.waitForLoadState('domcontentloaded'); | |
| const viewSourceLink = page.locator(`${GitPO.githubLinkSelector}:has-text("${GitPO.viewSourceLinkText}")`); | |
| await expect(viewSourceLink).toBeVisible({ timeout: 20000 }); | |
| const repoUrl = await viewSourceLink.getAttribute('href'); | |
| if (!repoUrl) { | |
| throw new Error('Missing repository URL on View Source link'); | |
| } | |
| const githubPagePromise = page.context().waitForEvent('page'); | |
| await viewSourceLink.click(); | |
| const githubPage = await githubPagePromise; | |
| await githubPage.waitForLoadState('domcontentloaded'); | |
| const actionsUrl = this.buildActionsUrl(repoUrl); | |
| if (!githubPage.url().startsWith(actionsUrl)) { | |
| await githubPage.goto(actionsUrl, { timeout: 20000, waitUntil: 'domcontentloaded' }); | |
| } | |
| await expect(githubPage).toHaveURL(actionsUrl, { timeout: 20000 }); | |
| await githubPage.close(); | |
| } | |
| public async checkPipelineRunsTable(page: Page): Promise<void> { | |
| const overviewUrl = page.url().replace(/\/ci(?:\?.*)?$/, ''); | |
| await page.goto(overviewUrl, { timeout: 20000 }); | |
| await page.waitForLoadState('domcontentloaded'); | |
| const viewSourceLink = page.locator(`${GitPO.githubLinkSelector}:has-text("${GitPO.viewSourceLinkText}")`); | |
| await expect(viewSourceLink).toBeVisible({ timeout: 20000 }); | |
| const repoUrl = await viewSourceLink.getAttribute('href'); | |
| if (!repoUrl) { | |
| throw new Error('Missing repository URL on View Source link'); | |
| } | |
| const githubPagePromise = page.context().waitForEvent('page'); | |
| await viewSourceLink.click(); | |
| const githubPage = await githubPagePromise; | |
| try { | |
| await githubPage.waitForLoadState('domcontentloaded'); | |
| const actionsUrl = this.buildActionsUrl(repoUrl); | |
| if (!githubPage.url().startsWith(actionsUrl)) { | |
| await githubPage.goto(actionsUrl, { timeout: 20000, waitUntil: 'domcontentloaded' }); | |
| } | |
| await expect(githubPage).toHaveURL(actionsUrl, { timeout: 20000 }); | |
| } finally { | |
| await githubPage.close(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/ui/plugins/ci/githubActionsPlugin.ts around lines 14 to 40, the newly
opened githubPage may not be closed if an assertion (toHaveURL) or other
operation throws, causing resource leaks; wrap the page operations in a
try/finally so the page is always closed: after awaiting the githubPagePromise
and assigning githubPage, perform the navigation/assertions inside a try block
and in the finally block await githubPage.close(), guarding the close with a
null/undefined check so close is executed even when an error is thrown.
| const actionsUrl = this.buildActionsUrl(repoUrl); | ||
| if (!githubPage.url().startsWith(actionsUrl)) { | ||
| await githubPage.goto(actionsUrl, { timeout: 20000, waitUntil: 'domcontentloaded' }); | ||
| } | ||
|
|
||
| await expect(githubPage).toHaveURL(actionsUrl, { timeout: 20000 }); |
There was a problem hiding this comment.
URL matching logic may be too strict.
Line 33 checks if the URL starts with actionsUrl (allowing query parameters), but line 37 expects an exact match. If the GitHub page includes query parameters (e.g., ?tab=workflows), the test will fail even though it's on the correct Actions page.
Consider using a more flexible matcher:
- await expect(githubPage).toHaveURL(actionsUrl, { timeout: 20000 });
+ await expect(githubPage).toHaveURL(new RegExp(`^${actionsUrl.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}(\\?.*)?$`), { timeout: 20000 });Or simplify by checking the conditional navigation succeeded:
const actionsUrl = this.buildActionsUrl(repoUrl);
if (!githubPage.url().startsWith(actionsUrl)) {
await githubPage.goto(actionsUrl, { timeout: 20000, waitUntil: 'domcontentloaded' });
}
- await expect(githubPage).toHaveURL(actionsUrl, { timeout: 20000 });
+ await expect(githubPage.url()).toMatch(new RegExp(`^${actionsUrl.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}`));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const actionsUrl = this.buildActionsUrl(repoUrl); | |
| if (!githubPage.url().startsWith(actionsUrl)) { | |
| await githubPage.goto(actionsUrl, { timeout: 20000, waitUntil: 'domcontentloaded' }); | |
| } | |
| await expect(githubPage).toHaveURL(actionsUrl, { timeout: 20000 }); | |
| const actionsUrl = this.buildActionsUrl(repoUrl); | |
| if (!githubPage.url().startsWith(actionsUrl)) { | |
| await githubPage.goto(actionsUrl, { timeout: 20000, waitUntil: 'domcontentloaded' }); | |
| } | |
| await expect(githubPage).toHaveURL(new RegExp(`^${actionsUrl.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}(\\?.*)?$`), { timeout: 20000 }); |
🤖 Prompt for AI Agents
In src/ui/plugins/ci/githubActionsPlugin.ts around lines 32 to 37, the code
navigates to actionsUrl then asserts an exact URL match which fails if GitHub
appends query params; change the assertion to allow query parameters by
asserting the page URL starts with actionsUrl (or use a regex/startsWith-style
matcher) instead of exact equality, or replace the expect with a check that
githubPage.url().startsWith(actionsUrl) so the test passes when extra query
string or fragment is present.
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/ui/plugins/ci/githubActionsPlugin.ts (2)
27-68: Ensure proper cleanup on assertion failure.If any assertion or operation fails (lines 37, 42, 51, 62, or 66),
githubPage.close()on line 68 won't execute, leaving the page open and causing resource leaks in the test run.Wrap the page operations in a try-finally block:
const githubPagePromise = page.context().waitForEvent('page'); await viewSourceLink.click(); const githubPage = await githubPagePromise; - await githubPage.waitForLoadState('domcontentloaded'); - - const actionsUrl = this.buildActionsUrl(repoUrl); - if (!githubPage.url().startsWith(actionsUrl)) { - await githubPage.goto(actionsUrl, { timeout: 20000, waitUntil: 'domcontentloaded' }); - } - - await expect(githubPage).toHaveURL(actionsUrl, { timeout: 20000 }); - - // Verify workflow runs exist - updated selectors for current GitHub UI - // GitHub now uses different structure - look for workflow run items - const workflowRuns = githubPage.locator('[data-testid="run-list-item"], .Box-row, div[class*="TimelineItem"]').first(); - await expect(workflowRuns).toBeVisible({ timeout: 20000 }); - - // Click on first workflow run - try multiple possible selectors - const firstRunLink = githubPage.locator( - 'a[href*="/actions/runs/"], ' + - '.Box-row a[href*="/runs/"], ' + - '[data-testid="run-list-item"] a, ' + - 'div[class*="TimelineItem"] a[href*="/runs/"]' - ).first(); - await expect(firstRunLink).toBeVisible({ timeout: 20000 }); - - await firstRunLink.click(); - await githubPage.waitForURL(/\/actions\/runs\//, { - timeout: 20000, - waitUntil: 'domcontentloaded' - }); - - // Verify we're on workflow run detail page by checking for key elements - // Look for Summary heading or section - this is always present on run detail pages - const summaryHeading = githubPage.getByRole('heading', { name: 'Summary' }); - await expect(summaryHeading).toBeVisible({ timeout: 20000 }); - - // Go back to actions list - await githubPage.goBack({ waitUntil: 'domcontentloaded' }); - await expect(githubPage).toHaveURL(actionsUrl, { timeout: 20000 }); - - await githubPage.close(); + try { + await githubPage.waitForLoadState('domcontentloaded'); + + const actionsUrl = this.buildActionsUrl(repoUrl); + if (!githubPage.url().startsWith(actionsUrl)) { + await githubPage.goto(actionsUrl, { timeout: 20000, waitUntil: 'domcontentloaded' }); + } + + await expect(githubPage).toHaveURL(actionsUrl, { timeout: 20000 }); + + // Verify workflow runs exist - updated selectors for current GitHub UI + // GitHub now uses different structure - look for workflow run items + const workflowRuns = githubPage.locator('[data-testid="run-list-item"], .Box-row, div[class*="TimelineItem"]').first(); + await expect(workflowRuns).toBeVisible({ timeout: 20000 }); + + // Click on first workflow run - try multiple possible selectors + const firstRunLink = githubPage.locator( + 'a[href*="/actions/runs/"], ' + + '.Box-row a[href*="/runs/"], ' + + '[data-testid="run-list-item"] a, ' + + 'div[class*="TimelineItem"] a[href*="/runs/"]' + ).first(); + await expect(firstRunLink).toBeVisible({ timeout: 20000 }); + + await firstRunLink.click(); + await githubPage.waitForURL(/\/actions\/runs\//, { + timeout: 20000, + waitUntil: 'domcontentloaded' + }); + + // Verify we're on workflow run detail page by checking for key elements + // Look for Summary heading or section - this is always present on run detail pages + const summaryHeading = githubPage.getByRole('heading', { name: 'Summary' }); + await expect(summaryHeading).toBeVisible({ timeout: 20000 }); + + // Go back to actions list + await githubPage.goBack({ waitUntil: 'domcontentloaded' }); + await expect(githubPage).toHaveURL(actionsUrl, { timeout: 20000 }); + } finally { + await githubPage.close(); + }
32-37: URL matching logic may be too strict.Line 33 checks if the URL starts with
actionsUrl(allowing query parameters), but line 37 expects an exact match. If GitHub appends query parameters (e.g.,?tab=workflows), the test will fail even though it's on the correct Actions page.Consider using a more flexible matcher:
- await expect(githubPage).toHaveURL(actionsUrl, { timeout: 20000 }); + await expect(githubPage.url()).toMatch(new RegExp(`^${actionsUrl.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}`));
🧹 Nitpick comments (2)
src/ui/plugins/ci/githubActionsPlugin.ts (2)
15-15: URL manipulation could be fragile.The regex
replace(/\/ci(?:\?.*)?$/, '')assumes the CI page URL always ends with/ci(plus optional query params). If the URL structure changes or includes additional path segments, this replacement may fail silently or produce incorrect results.Consider a more robust approach:
- const overviewUrl = page.url().replace(/\/ci(?:\?.*)?$/, ''); + // Navigate up one level from /ci to the component overview + const currentUrl = new URL(page.url()); + const pathSegments = currentUrl.pathname.split('/').filter(Boolean); + if (pathSegments[pathSegments.length - 1] === 'ci') { + pathSegments.pop(); + } + currentUrl.pathname = pathSegments.join('/') || '/'; + const overviewUrl = currentUrl.toString();
10-10: Add inline comments to document empty method overrides for maintainability.The empty implementations of
checkCIHeadingandcheckActionsappear intentional (evidenced by the underscore-prefixed unused parameters), and GitHub Actions likely lacks these UI elements compared to other CI systems. However, without explanatory comments, future maintainers may misunderstand this design choice.Suggested improvement:
- Add a brief comment above each empty method explaining why it's intentionally empty, e.g.:
// GitHub Actions UI does not display a CI heading; verification is done in checkPipelineRunsTable public async checkCIHeading(_page: Page): Promise<void> {}The
checkPipelineRunsTableimplementation demonstrates solid E2E testing patterns with multiple selector strategies for resilience and proper wait conditions—good work there.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Dockerfileis excluded by none and included by none
📒 Files selected for processing (5)
integration-tests/config/testplan.json(0 hunks)src/api/gitlab/config/gitlab.config.ts(2 hunks)src/api/gitlab/gitlab.client.ts(3 hunks)src/api/gitlab/services/gitlab-merge-request.service.ts(1 hunks)src/ui/plugins/ci/githubActionsPlugin.ts(1 hunks)
💤 Files with no reviewable changes (1)
- integration-tests/config/testplan.json
🧰 Additional context used
📓 Path-based instructions (4)
src/api/**
⚙️ CodeRabbit configuration file
Focus on error handling, retry mechanisms, proper async/await usage, API client patterns, authentication security, and comprehensive logging for debugging. Ensure all API calls have appropriate timeouts and circuit breaker patterns.
Files:
src/api/gitlab/services/gitlab-merge-request.service.tssrc/api/gitlab/gitlab.client.tssrc/api/gitlab/config/gitlab.config.ts
{src,tests}/ui/**/*.ts
📄 CodeRabbit inference engine (README.md)
{src,tests}/ui/**/*.ts: UI entities (classes/components) must include either "Ui" or "plugin" in their names to distinguish them from backend entities
When entering secrets in UI tests, blur the input field so secrets do not appear in screenshots
Files:
src/ui/plugins/ci/githubActionsPlugin.ts
src/ui/plugins/**/*.ts
📄 CodeRabbit inference engine (README.md)
src/ui/plugins/**/*.ts: Plugin-related functionality must be stored in src/ui/plugins, organized by plugin type (e.g., git, ci), and file names should match the plugin’s short name
Classes defined in plugin files must include either "Ui" or "plugin" in their class names
Files:
src/ui/plugins/ci/githubActionsPlugin.ts
src/ui/**
⚙️ CodeRabbit configuration file
Focus on page object pattern implementation, element selection strategies, UI test stability, maintainability, proper wait conditions, error handling, and flaky test prevention. Ensure tests are resilient to UI changes.
Files:
src/ui/plugins/ci/githubActionsPlugin.ts
🧬 Code graph analysis (1)
src/ui/plugins/ci/githubActionsPlugin.ts (2)
src/ui/plugins/ci/baseCIPlugin.ts (1)
BaseCIPlugin(8-107)src/ui/page-objects/commonPo.ts (1)
GitPO(23-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / tssc-test-on-pull-request
🔇 Additional comments (6)
src/api/gitlab/config/gitlab.config.ts (2)
14-14: Documentation aligns with the implementation.The updated default value is consistent with the implementation at line 96.
96-96: Increased timeout aligns with GitLab API operation characteristics.The 60-second default is appropriate for potentially slow GitLab operations (CI/CD pipelines, large repository queries). The change is backward-compatible since timeout remains optional.
src/api/gitlab/gitlab.client.ts (2)
24-24: Documentation accurately reflects the new default timeout.The JSDoc comments are consistent with the implementation changes.
Also applies to: 101-101
113-113: LGTM: Timeout propagation is correct.The fallback to 60000 aligns with the config builder default and ensures a reasonable timeout is always set.
src/api/gitlab/services/gitlab-merge-request.service.ts (1)
145-167: Error structure verified—code is correct and ready.The error structure
error.cause?.response?.statusmatches @gitbeaker/rest v43.0.0 conventions, where request failures throw GitbeakerRequestError with cause containing the Response object including the HTTP status. The 405 handling logic executes reliably:
- Verification call properly queries actual MR state as recovery mechanism
- Early return on successful verification prevents unnecessary re-throws
- Original error is properly transformed and re-thrown via
createGitLabErrorFromResponse()if verification fails or doesn't apply- Comprehensive logging at warning/success/error levels aids debugging
The code meets framework reliability and maintainability standards for error handling and recovery patterns.
src/ui/plugins/ci/githubActionsPlugin.ts (1)
71-76: LGTM!The
buildActionsUrlhelper correctly extracts the base repository URL and constructs the Actions URL. The regex pattern is appropriate, and the trailing slash handling ensures consistent URL formation.
| queryTimeout: this.timeout, | ||
| rejectUnauthorized: config.sslVerify, |
There was a problem hiding this comment.
Add fallback for rejectUnauthorized to ensure secure defaults.
While queryTimeout propagation is excellent, rejectUnauthorized should have an explicit fallback to true since config.sslVerify is optional in the GitLabConfig interface. If a config is created without using the builder, SSL verification might be unintentionally disabled.
Apply this diff to ensure SSL verification defaults to true:
this.client = new Gitlab({
host: this.baseUrl,
token: config.token,
queryTimeout: this.timeout,
- rejectUnauthorized: config.sslVerify,
+ rejectUnauthorized: config.sslVerify ?? true,
});🤖 Prompt for AI Agents
In src/api/gitlab/gitlab.client.ts around lines 117-118, the rejectUnauthorized
option is currently set directly from config.sslVerify which is optional; change
it to use a safe fallback so SSL verification defaults to true (e.g. set
rejectUnauthorized to config.sslVerify ?? true or an equivalent boolean-coercing
expression) and ensure the value passed is a boolean.
| // Go back to actions list | ||
| await githubPage.goBack({ waitUntil: 'domcontentloaded' }); | ||
| await expect(githubPage).toHaveURL(actionsUrl, { timeout: 20000 }); |
There was a problem hiding this comment.
Same URL matching strictness after navigation.
Line 66 has the same strict URL matching issue as line 37. After goBack(), GitHub may restore query parameters that were present before navigating to the run detail page.
Apply the same flexible matching approach:
await githubPage.goBack({ waitUntil: 'domcontentloaded' });
- await expect(githubPage).toHaveURL(actionsUrl, { timeout: 20000 });
+ await expect(githubPage.url()).toMatch(new RegExp(`^${actionsUrl.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}`));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Go back to actions list | |
| await githubPage.goBack({ waitUntil: 'domcontentloaded' }); | |
| await expect(githubPage).toHaveURL(actionsUrl, { timeout: 20000 }); | |
| // Go back to actions list | |
| await githubPage.goBack({ waitUntil: 'domcontentloaded' }); | |
| await expect(githubPage.url()).toMatch(new RegExp(`^${actionsUrl.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}`)); |
🤖 Prompt for AI Agents
In src/ui/plugins/ci/githubActionsPlugin.ts around lines 64 to 66, the test
currently asserts the exact actionsUrl after githubPage.goBack(), which can fail
if GitHub restores prior query parameters; change the assertion to use a
flexible URL match (e.g., assert the current URL starts with or contains the
actionsUrl or use a regex that matches actionsUrl as the prefix) so the check
tolerates restored query params while still ensuring we returned to the actions
list. Ensure the expectation keeps the same timeout and only relaxes the strict
equality to prefix/contains/regex matching.
4d0a865 to
137c961
Compare
… of the CI in the flow
BohdanMar
force-pushed
the
feature/github-actions-ci-verification
branch
from
137c961 to
e762834
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/ui/plugins/ci/githubActionsPlugin.ts (1)
27-39: Always close the new tab and relax URL matching; also bind the new tab to the click.
- Ensure the new page is tied to the click via Promise.all + predicate.
- Close the page in a finally to avoid leaks if assertions fail. [duplicate of earlier feedback]
- Allow query params in URL assertion to avoid false negatives. [duplicate of earlier feedback]
- const githubPagePromise = page.context().waitForEvent('page'); - await viewSourceLink.click(); - const githubPage = await githubPagePromise; - await githubPage.waitForLoadState('domcontentloaded'); - - const actionsUrl = this.buildActionsUrl(repoUrl); - if (!githubPage.url().startsWith(actionsUrl)) { - await githubPage.goto(actionsUrl, { timeout: 20000, waitUntil: 'domcontentloaded' }); - } - - await expect(githubPage).toHaveURL(actionsUrl, { timeout: 20000 }); - - await githubPage.close(); + const [githubPage] = await Promise.all([ + page.context().waitForEvent('page', p => p.opener() === page), + viewSourceLink.click(), + ]); + try { + await githubPage.waitForLoadState('domcontentloaded'); + + const actionsUrl = this.buildActionsUrl(repoUrl); + if (!githubPage.url().startsWith(actionsUrl)) { + await githubPage.goto(actionsUrl, { timeout: 20000, waitUntil: 'domcontentloaded' }); + } + + const escaped = actionsUrl.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + await expect(githubPage).toHaveURL(new RegExp(`^${escaped}(\\?.*)?$`), { timeout: 20000 }); + } finally { + await githubPage.close(); + }
🧹 Nitpick comments (1)
src/ui/plugins/ci/githubActionsPlugin.ts (1)
15-18: Consolidate navigation wait to reduce flakiness.Use waitUntil in goto instead of a separate waitForLoadState.
- await page.goto(overviewUrl, { timeout: 20000 }); - await page.waitForLoadState('domcontentloaded'); + await page.goto(overviewUrl, { timeout: 20000, waitUntil: 'domcontentloaded' });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/ui/commonUi.ts(1 hunks)src/ui/page-objects/githubActionsPo.ts(1 hunks)src/ui/plugins/ci/ciFactory.ts(2 hunks)src/ui/plugins/ci/githubActionsPlugin.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ui/plugins/ci/ciFactory.ts
- src/ui/commonUi.ts
- src/ui/page-objects/githubActionsPo.ts
🧰 Additional context used
📓 Path-based instructions (3)
{src,tests}/ui/**/*.ts
📄 CodeRabbit inference engine (README.md)
{src,tests}/ui/**/*.ts: UI entities (classes/components) must include either "Ui" or "plugin" in their names to distinguish them from backend entities
When entering secrets in UI tests, blur the input field so secrets do not appear in screenshots
Files:
src/ui/plugins/ci/githubActionsPlugin.ts
src/ui/plugins/**/*.ts
📄 CodeRabbit inference engine (README.md)
src/ui/plugins/**/*.ts: Plugin-related functionality must be stored in src/ui/plugins, organized by plugin type (e.g., git, ci), and file names should match the plugin’s short name
Classes defined in plugin files must include either "Ui" or "plugin" in their class names
Files:
src/ui/plugins/ci/githubActionsPlugin.ts
src/ui/**
⚙️ CodeRabbit configuration file
Focus on page object pattern implementation, element selection strategies, UI test stability, maintainability, proper wait conditions, error handling, and flaky test prevention. Ensure tests are resilient to UI changes.
Files:
src/ui/plugins/ci/githubActionsPlugin.ts
🧬 Code graph analysis (1)
src/ui/plugins/ci/githubActionsPlugin.ts (2)
src/ui/plugins/ci/baseCIPlugin.ts (1)
BaseCIPlugin(8-107)src/ui/page-objects/commonPo.ts (1)
GitPO(23-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / tssc-test-on-pull-request
| super(name, registryOrg); | ||
| } | ||
|
|
||
| public async checkCIHeading(_page: Page): Promise<void> {} |
There was a problem hiding this comment.
Don’t override checkCIHeading to a no-op; call the base implementation.
Overriding removes an existing verification from BaseCIPlugin, weakening the test. Call super or remove the override.
- public async checkCIHeading(_page: Page): Promise<void> {}
+ public async checkCIHeading(page: Page): Promise<void> {
+ return super.checkCIHeading(page);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async checkCIHeading(_page: Page): Promise<void> {} | |
| public async checkCIHeading(page: Page): Promise<void> { | |
| return super.checkCIHeading(page); | |
| } |
🤖 Prompt for AI Agents
In src/ui/plugins/ci/githubActionsPlugin.ts around line 10, the method
checkCIHeading is overridden as an empty no-op which removes the verification
implemented in BaseCIPlugin; restore the base behavior by either removing this
override entirely or calling the base implementation (e.g., make the method call
and await super.checkCIHeading(_page) or return super.checkCIHeading(_page)) so
the original verification runs.
| private buildActionsUrl(repoUrl: string): string { | ||
| const trimmedUrl = repoUrl.endsWith('/') ? repoUrl.slice(0, -1) : repoUrl; | ||
| return `${trimmedUrl}/actions`; | ||
| } |
There was a problem hiding this comment.
Harden buildActionsUrl against query/fragment in repoUrl.
If repoUrl includes ?tab=… or #…, current logic yields an invalid path like …?tab=…/actions.
- private buildActionsUrl(repoUrl: string): string {
- const trimmedUrl = repoUrl.endsWith('/') ? repoUrl.slice(0, -1) : repoUrl;
- return `${trimmedUrl}/actions`;
- }
+ private buildActionsUrl(repoUrl: string): string {
+ try {
+ const u = new URL(repoUrl);
+ const base = `${u.origin}${u.pathname.replace(/\/$/, '')}`;
+ return `${base}/actions`;
+ } catch {
+ // Fallback for unexpected non-absolute URLs
+ const noHash = repoUrl.split('#')[0];
+ const noQuery = noHash.split('?')[0];
+ const trimmed = noQuery.replace(/\/$/, '');
+ return `${trimmed}/actions`;
+ }
+ }🤖 Prompt for AI Agents
In src/ui/plugins/ci/githubActionsPlugin.ts around lines 42 to 45, the
buildActionsUrl currently simply trims a trailing slash and appends "/actions",
which breaks when repoUrl contains a query or fragment (e.g. ?tab= or #). Parse
the repoUrl (use the URL constructor or a safe fallback for non-absolute
values), remove search and hash components, ensure there is no trailing slash,
then append "/actions" and return the resulting string; handle invalid URLs by
falling back to stripping anything after the first '?' or '#' before appending.
|
@BohdanMar: The following test has Failed, say /retest to rerun failed tests.
Inspecting Test ArtifactsTo inspect your test artifacts, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/konflux-test-storage/rhtap-team/rhtap-cli:e2e-4.19-w272fTest results analysis<not enabled> OCI Artifact Browser URL<not enabled> |
3 participants
Add: verification for GitHub Actions integration as part of the CI in the flow
Summary by CodeRabbit
New Features
Bug Fixes