playwright: Add boot test for content-templates (HMS-9499) #3947
Conversation
ochosi
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #3947 +/- ##
==========================================
- Coverage 75.08% 74.17% -0.92%
==========================================
Files 202 205 +3
Lines 7446 7555 +109
Branches 2559 2584 +25
==========================================
+ Hits 5591 5604 +13
- Misses 1626 1709 +83
- Partials 229 242 +13 see 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
ochosi
force-pushed
the
content-templates-boot-test
branch
from
9ec96da to
5c0216c
Compare
ochosi
changed the title
ochosi
requested review from
ksiekl,
mgold1234 and
regexowl
and removed request for
a team
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
registerWithActivationKey, usinggetByText('Enable predictive analytics')/getByText('Enable remote remediations')combined withisChecked()assumes those text locators resolve to a checkbox/radio; if these are PatternFly switches or labels, this will likely be brittle or fail at runtime—consider targeting the underlying input/switch role directly (e.g. by role or data-test-id) instead of the text node. - The new
registerWithActivationKeyhelper hardcodes selection of an option withname: 'activation-key-', which seems very loose and may match multiple or no options depending on environment; consider making the activation key name configurable or matching a more specific label to avoid flakiness. - Multiple existing boot tests are now individually
test.skip-ed with a temporary comment; if these are meant to be disabled only while debugging, consider grouping them in adescribe.skipor guarding via an environment flag so they can be re-enabled or toggled more easily and are less likely to stay skipped unintentionally.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `registerWithActivationKey`, using `getByText('Enable predictive analytics')` / `getByText('Enable remote remediations')` combined with `isChecked()` assumes those text locators resolve to a checkbox/radio; if these are PatternFly switches or labels, this will likely be brittle or fail at runtime—consider targeting the underlying input/switch role directly (e.g. by role or data-test-id) instead of the text node.
- The new `registerWithActivationKey` helper hardcodes selection of an option with `name: 'activation-key-'`, which seems very loose and may match multiple or no options depending on environment; consider making the activation key name configurable or matching a more specific label to avoid flakiness.
- Multiple existing boot tests are now individually `test.skip`-ed with a temporary comment; if these are meant to be disabled only while debugging, consider grouping them in a `describe.skip` or guarding via an environment flag so they can be re-enabled or toggled more easily and are less likely to stay skipped unintentionally.
## Individual Comments
### Comment 1
<location> `playwright/helpers/wizardHelpers.ts:84-85` </location>
<code_context>
+ await page.getByRole('option', { name: 'activation-key-' }).click();
+
+ // Ensure Enable predictive analytics is checked
+ const insightsSwitch = page.getByText('Enable predictive analytics');
+ if (!(await insightsSwitch.isChecked())) {
+ await insightsSwitch.click();
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `getByText(...).isChecked()` is fragile and may fail if the target is not the actual checkbox/switch element.
`isChecked()` only works on checkable controls (checkbox/radio/switch). `getByText('Enable predictive analytics')` may resolve to a label or wrapper instead of the input, causing runtime errors or misleading results. Please select the actual control via role, e.g. `getByRole('switch', { name: 'Enable predictive analytics' })` (or `checkbox`) and apply the same pattern to the `Enable remote remediations` toggle.
</issue_to_address>
### Comment 2
<location> `playwright/BootTests/helpers/helpers.ts:294-303` </location>
<code_context>
+export const deleteTemplate = async (page: Page, templateName: string) => {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Broad `try/catch` causes any error (including navigation issues) to be silently swallowed when a template is missing.
Because the `try` wraps navigation, search, and the visibility `expect`, any failure in that block (network/auth issues, locator changes, etc.) is treated as “template not found” and the function just returns. If you only want to handle the “template does not exist” case, please narrow the `catch` to the specific operation that detects absence (e.g., the `expect` or a timeout-bounded `locator.count()` check) and allow unexpected errors to surface instead of being swallowed.
Suggested implementation:
```typescript
export const deleteTemplate = async (page: Page, templateName: string) => {
await closePopupsIfExist(page);
await test.step(
'Delete the template with name: ' + templateName,
async () => {
await navigateToTemplates(page);
const filterInput = page.getByRole('searchbox', { name: 'Filter by name' });
await filterInput.fill(templateName);
const templateRow = page.getByRole('row', { name: templateName });
const rowCount = await templateRow.count();
// If the template is not found after filtering, fail gracefully
if (rowCount === 0) {
return;
}
// Wait for the template row to appear before interacting with it
await templateRow.first().waitFor({ state: 'visible', timeout: 10000 });
```
I only see the beginning of the `deleteTemplate` function. To fully align with this change:
1. Remove any remaining `catch` block that was paired with the `try` you previously had, since the `try` has been removed.
2. If the rest of the function re-locates the template row, you can simplify it by reusing `templateRow.first()` from above for subsequent actions (clicking menus, confirming deletion, etc.).
3. If you want a different timeout than `10000`ms, adjust the `waitFor` timeout to match your test suite’s conventions.
</issue_to_address>
### Comment 3
<location> `playwright/BootTests/helpers/helpers.ts:382-391` </location>
<code_context>
+ shouldRetry = true;
+ } else {
+ // Find the system with matching hostname
+ const system = body.data.find(
+ (sys: { attributes: { display_name: string } }) =>
+ sys.attributes.display_name === hostname,
+ );
+
+ if (!system) {
+ console.log(
+ `System '${hostname}' not found in inventory, attempt ${attempts}/${maxAttempts}`,
+ );
+ shouldRetry = true;
+ } else {
+ // Check if system has a template_uuid assigned
+ const hasTemplate = !!system.attributes?.template_uuid;
+
+ if (hasTemplate) {
</code_context>
<issue_to_address>
**suggestion:** The inferred `system` shape is partially typed and could lead to unnoticed shape changes in the API response.
`system` is typed as `{ attributes: { display_name: string } }`, but later you access `attributes.template_uuid` and `attributes.template_name`. With this mismatch, TypeScript won’t catch backend changes or missing fields. Consider defining a `System` interface that includes all used fields (`display_name`, `template_uuid`, `template_name`, etc.) and using that type for `body.data` / `system` for stronger type safety.
Suggested implementation:
```typescript
} else {
interface System {
attributes: {
display_name: string;
template_uuid?: string;
template_name?: string;
};
// Include any other fields returned by the API that we don't use directly
[key: string]: unknown;
}
const body: { data?: System[] } = await response.json();
```
```typescript
// Find the system with matching hostname
const system = body.data.find(
(sys: System) => sys.attributes.display_name === hostname,
);
```
If `System` is already defined elsewhere in your codebase (e.g., shared types for the inventory API), you should instead import that existing type at the top of this file and remove the local `interface System` declaration. Adjust the import path and remove the inline `interface System` accordingly to avoid duplicate type definitions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const insightsSwitch = page.getByText('Enable predictive analytics'); | ||
| if (!(await insightsSwitch.isChecked())) { |
There was a problem hiding this comment.
issue (bug_risk): Using getByText(...).isChecked() is fragile and may fail if the target is not the actual checkbox/switch element.
isChecked() only works on checkable controls (checkbox/radio/switch). getByText('Enable predictive analytics') may resolve to a label or wrapper instead of the input, causing runtime errors or misleading results. Please select the actual control via role, e.g. getByRole('switch', { name: 'Enable predictive analytics' }) (or checkbox) and apply the same pattern to the Enable remote remediations toggle.
| export const deleteTemplate = async (page: Page, templateName: string) => { | ||
| await closePopupsIfExist(page); | ||
| await test.step( | ||
| 'Delete the template with name: ' + templateName, | ||
| async () => { | ||
| try { | ||
| await navigateToTemplates(page); | ||
| await page | ||
| .getByRole('searchbox', { name: 'Filter by name' }) | ||
| .fill(templateName); |
There was a problem hiding this comment.
suggestion (bug_risk): Broad try/catch causes any error (including navigation issues) to be silently swallowed when a template is missing.
Because the try wraps navigation, search, and the visibility expect, any failure in that block (network/auth issues, locator changes, etc.) is treated as “template not found” and the function just returns. If you only want to handle the “template does not exist” case, please narrow the catch to the specific operation that detects absence (e.g., the expect or a timeout-bounded locator.count() check) and allow unexpected errors to surface instead of being swallowed.
Suggested implementation:
export const deleteTemplate = async (page: Page, templateName: string) => {
await closePopupsIfExist(page);
await test.step(
'Delete the template with name: ' + templateName,
async () => {
await navigateToTemplates(page);
const filterInput = page.getByRole('searchbox', { name: 'Filter by name' });
await filterInput.fill(templateName);
const templateRow = page.getByRole('row', { name: templateName });
const rowCount = await templateRow.count();
// If the template is not found after filtering, fail gracefully
if (rowCount === 0) {
return;
}
// Wait for the template row to appear before interacting with it
await templateRow.first().waitFor({ state: 'visible', timeout: 10000 });I only see the beginning of the deleteTemplate function. To fully align with this change:
- Remove any remaining
catchblock that was paired with thetryyou previously had, since thetryhas been removed. - If the rest of the function re-locates the template row, you can simplify it by reusing
templateRow.first()from above for subsequent actions (clicking menus, confirming deletion, etc.). - If you want a different timeout than
10000ms, adjust thewaitFortimeout to match your test suite’s conventions.
| const system = body.data.find( | ||
| (sys: { attributes: { display_name: string } }) => | ||
| sys.attributes.display_name === hostname, | ||
| ); | ||
|
|
||
| if (!system) { | ||
| console.log( | ||
| `System '${hostname}' not found in inventory, attempt ${attempts}/${maxAttempts}`, | ||
| ); | ||
| shouldRetry = true; |
There was a problem hiding this comment.
suggestion: The inferred system shape is partially typed and could lead to unnoticed shape changes in the API response.
system is typed as { attributes: { display_name: string } }, but later you access attributes.template_uuid and attributes.template_name. With this mismatch, TypeScript won’t catch backend changes or missing fields. Consider defining a System interface that includes all used fields (display_name, template_uuid, template_name, etc.) and using that type for body.data / system for stronger type safety.
Suggested implementation:
} else {
interface System {
attributes: {
display_name: string;
template_uuid?: string;
template_name?: string;
};
// Include any other fields returned by the API that we don't use directly
[key: string]: unknown;
}
const body: { data?: System[] } = await response.json(); // Find the system with matching hostname
const system = body.data.find(
(sys: System) => sys.attributes.display_name === hostname,
);If System is already defined elsewhere in your codebase (e.g., shared types for the inventory API), you should instead import that existing type at the top of this file and remove the local interface System declaration. Adjust the import path and remove the inline interface System accordingly to avoid duplicate type definitions.
📝 WalkthroughWalkthroughMultiple Playwright boot tests were converted to skipped tests (AAP, Compliance CIS, several Content non-repeatable and repeatable tests, Satellite) by wrapping them with 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Fix all issues with AI Agents 🤖
In @playwright/BootTests/AAP/AAP.boot.ts:
- Around line 63-65: Reformat the test.skip arrow function parameters so
Prettier passes: update the test declaration for test.skip('AAP registration
boot integration test', async ({ page, cleanup }) => { ... }) to follow the
project's Prettier rules (either keep the async and the destructured params on a
single well-spaced line or break the parameter list onto its own line if the
linter requires), ensuring the destructured params { page, cleanup } are
correctly spaced and no trailing commas or extra spaces remain.
In @playwright/BootTests/Compliance/ComplianceCIS.boot.ts:
- Around line 184-186: The test.skip declaration for "Compliance alerts - lint
warnings display" has a formatting issue in the async arrow function parameter
list; reformat the function signature so it conforms to Prettier (ensure proper
spacing after the comma and no extraneous whitespace) — specifically update the
async ({ page, cleanup }) => { signature used in the test.skip call to match
project's prettier rules (e.g., proper spacing and/or place parameters on their
own line) and then run the formatter to verify the change.
- Around line 34-36: Reformat the skipped test declaration so the async
parameter list follows Prettier rules: update the line containing
test.skip('Compliance step integration test - CIS', async ({ page, cleanup }) =>
{ to ensure proper spacing/quoting (e.g., double vs single quotes per project
style) and consistent spacing around the arrow function parameters; run Prettier
or adjust to the project formatter so the async ({ page, cleanup }) => {
parameter formatting passes CI; locate the test.skip invocation in
ComplianceCIS.boot.ts to apply the change.
In @playwright/helpers/wizardHelpers.ts:
- Around line 90-93: The current code calls isChecked() on the text locator
(rhcSwitch = page.getByText('Enable remote remediations')) which targets the
label, not the input; update the locator to point at the actual checkbox/switch
input (e.g., use page.getByLabel('Enable remote remediations') or
page.getByRole('checkbox', { name: 'Enable remote remediations' }) or find the
input inside the label via locator('label:has-text("Enable remote remediations")
>> input')) and then call isChecked() and click() on that input locator (symbols
to change: rhcSwitch, page.getByText, isChecked, click).
- Around line 84-87: The code calls isChecked() on the text node returned by
getByText('Enable predictive analytics') (variable insightsSwitch); change the
locator to target the actual form control (e.g., use page.getByLabel('Enable
predictive analytics'), page.getByRole('checkbox' or 'switch', { name: 'Enable
predictive analytics' }), or find the input inside the container returned by
getByText) and then call isChecked() and click() on that input element instead
of the label text so the checked state is read and toggled correctly.
- Around line 70-95: The selector in registerWithActivationKey that clicks the
activation key option currently uses a vague prefix ('activation-key-') and can
match the wrong item; update registerWithActivationKey to select the exact
activation key by either accepting a parameter (e.g., activationKey) and using
that full name in the getByRole('option', { name: activationKey }) call, or by
targeting a stable attribute like a data-testid on the option (e.g.,
getByTestId('activation-key-<id>')), and replace the current partial-match
selector accordingly to guarantee the intended option is clicked.
🧹 Nitpick comments (3)
playwright/BootTests/helpers/helpers.ts (2)
428-431: Simplify redundant conditional.The static analysis tool correctly identifies that
shouldRetryis always truthy at this point. Every code path that reaches line 429 has already setshouldRetry = true, and the only path that sets it tofalse(initial state) returns early at line 410.🔎 Proposed fix
- // Check if we should retry with delay - if (shouldRetry && attempts < maxAttempts) { + // Wait before next retry attempt + if (attempts < maxAttempts) { await sleep(delayMs); }
382-385: Incomplete type annotation for system attributes.The inline type for
sysonly declaresdisplay_name, but the code later accessestemplate_uuidandtemplate_name(lines 394, 397). Consider extending the type to include all accessed properties for better type safety.🔎 Proposed fix
- const system = body.data.find( - (sys: { attributes: { display_name: string } }) => - sys.attributes.display_name === hostname, - ); + interface SystemAttributes { + display_name: string; + template_uuid?: string; + template_name?: string; + } + const system = body.data.find( + (sys: { attributes: SystemAttributes }) => + sys.attributes.display_name === hostname, + );playwright/BootTests/Content/ContentTemplate.boot.ts (1)
186-194: Consider documenting or tracking the FIXME.The comment indicates a workaround is needed for the package search to work. If this is a bug in the application, consider opening an issue to track it, so this workaround can be removed when fixed.
Would you like me to help create an issue to track this behavior?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
playwright/BootTests/AAP/AAP.boot.tsplaywright/BootTests/Compliance/ComplianceCIS.boot.tsplaywright/BootTests/Content/ContentNonRepeatable.boot.tsplaywright/BootTests/Content/ContentRepeatable.boot.tsplaywright/BootTests/Content/ContentTemplate.boot.tsplaywright/BootTests/Satellite/Satellite.boot.tsplaywright/BootTests/helpers/helpers.tsplaywright/helpers/wizardHelpers.ts
🧰 Additional context used
🧬 Code graph analysis (3)
playwright/helpers/wizardHelpers.ts (1)
playwright/helpers/helpers.ts (1)
isHosted(27-29)
playwright/BootTests/helpers/helpers.ts (1)
playwright/helpers/helpers.ts (1)
closePopupsIfExist(31-55)
playwright/BootTests/Content/ContentTemplate.boot.ts (5)
playwright/helpers/helpers.ts (1)
isHosted(27-29)playwright/BootTests/helpers/helpers.ts (5)
deleteRepository(75-106)deleteTemplate(294-324)navigateToRepositories(247-271)navigateToTemplates(277-285)pollForSystemTemplateAttachment(341-436)playwright/BootTests/helpers/OpenStackWrapper.ts (1)
OpenStackWrapper(5-288)playwright/helpers/login.ts (2)
ensureAuthenticated(43-69)storeStorageStateAndToken(133-151)playwright/helpers/navHelpers.ts (3)
navigateToLandingPage(63-69)ibFrame(50-57)fillInImageOutput(18-44)
🪛 GitHub Actions: Boot tests
playwright/BootTests/Content/ContentTemplate.boot.ts
[error] 274-276: System did not register within 300 seconds.
🪛 GitHub Actions: Development checks
playwright/BootTests/AAP/AAP.boot.ts
[error] 65-65: prettier/prettier: Replace ·page,·cleanup· with ⏎··page,⏎··cleanup,⏎
🪛 GitHub Check: ESLint
playwright/BootTests/Compliance/ComplianceCIS.boot.ts
[failure] 36-36:
Replace ·page,·cleanup· with ⏎··page,⏎··cleanup,⏎
[failure] 186-186:
Replace ·page,·cleanup· with ⏎··page,⏎··cleanup,⏎
playwright/BootTests/helpers/helpers.ts
[warning] 429-429:
Unnecessary conditional, value is always truthy
playwright/BootTests/AAP/AAP.boot.ts
[failure] 65-65:
Replace ·page,·cleanup· with ⏎··page,⏎··cleanup,⏎
⏰ 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). (49)
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: Sourcery review
🔇 Additional comments (13)
playwright/BootTests/Satellite/Satellite.boot.ts (1)
29-31: LGTM - Test temporarily disabled.The test is appropriately skipped with a clear TODO comment indicating temporary disablement. This aligns with the PR objective to isolate the new ContentTemplate test during debugging.
playwright/BootTests/Content/ContentRepeatable.boot.ts (1)
26-28: LGTM - Test temporarily disabled.The test is appropriately skipped with a clear TODO comment indicating temporary disablement. This aligns with the PR objective to isolate the new ContentTemplate test during debugging.
playwright/BootTests/helpers/helpers.ts (3)
272-285: LGTM!The
navigateToTemplatesfunction follows the same pattern as the existingnavigateToRepositoriesfunction but is simpler since it doesn't need to handle a zero state. The implementation is clear and consistent with other navigation helpers in this file.
294-324: LGTM!The
deleteTemplatefunction follows the established pattern fromdeleteRepositoryanddeleteCompliancePolicy. The graceful failure handling in the catch block is appropriate for cleanup operations that should not fail the test if the resource doesn't exist.
326-330: LGTM!Simple and correct sleep utility function.
playwright/BootTests/Content/ContentTemplate.boot.ts (6)
1-33: LGTM!The imports and test setup are well-organized. The test properly imports all necessary helpers and uses the cleanup fixture for resource management.
44-61: LGTM!Good practice registering cleanup functions early, before creating resources. This ensures cleanup runs even if the test fails mid-execution. The pre-cleanup of the repository URL (line 54) is a defensive measure to handle leftover resources from previous failed runs.
65-158: LGTM!The repository and template creation steps are well-structured. The 180-second timeouts for waiting on snapshot and template validation are appropriate given these are async backend operations.
245-277: Registration timeout is a known issue; consider adding retry diagnostics.The pipeline failure confirms the PR description's note about registration against stage failing. The
returnat line 263 works but may be confusing—it exits thetest.stepcallback, which completes the step successfully.Consider adding more diagnostic output on failure (e.g.,
subscription-manager identityorcloud-init status) to help debug the registration issue.🔎 Suggested diagnostic enhancement
// If we get here, registration never completed + // Get diagnostic info before failing + const [, identityOutput] = await image.exec( + 'sudo subscription-manager identity 2>&1 || true', + ); + const [, cloudInitOutput] = await image.exec( + 'sudo cloud-init status --long 2>&1 || true', + ); + // eslint-disable-next-line no-console + console.log('subscription-manager identity:', identityOutput); + // eslint-disable-next-line no-console + console.log('cloud-init status:', cloudInitOutput); + throw new Error( `System did not register within ${(maxAttempts * delayMs) / 1000} seconds`, );
279-291: LGTM!The verification approach is sound—checking that package URLs contain
/templates/confirms the system is using the content template rather than the base CDN. Per the PR description, this step is expected to fail in the current state until the registration issue is resolved.
293-315: LGTM!Re-authenticating after the long build process is good defensive practice. The polling parameters (10s delay, 30 attempts = 5 min max) are reasonable for waiting on system inventory updates.
playwright/BootTests/Content/ContentNonRepeatable.boot.ts (2)
27-29: Temporary test skip is acceptable; ensure tracking for re-enablement.The TODO comment clearly explains this is temporary while debugging ContentTemplate. The ESLint directive is appropriate to suppress the no-skipped-test rule for intentional skips.
Consider creating an issue or task to track re-enabling these tests once ContentTemplate debugging is complete.
144-146: LGTM!The skip pattern is consistently applied to all three tests with appropriate TODO comments and ESLint directives.
Also applies to: 301-303
| export const registerWithActivationKey = async (page: Page | FrameLocator) => { | ||
| if (isHosted()) { | ||
| await page.getByRole('button', { name: 'Register' }).click(); | ||
|
|
||
| // Ensure "Automatically register to Red Hat" is selected (should be default) | ||
| await expect( | ||
| page.getByRole('radio', { name: 'Automatically register to Red Hat' }), | ||
| ).toBeChecked(); | ||
|
|
||
| // Select an activation key from the dropdown | ||
| await page.getByRole('button', { name: 'Menu toggle' }).click(); | ||
| await page.getByRole('option', { name: 'activation-key-' }).click(); | ||
|
|
||
| // Ensure Enable predictive analytics is checked | ||
| const insightsSwitch = page.getByText('Enable predictive analytics'); | ||
| if (!(await insightsSwitch.isChecked())) { | ||
| await insightsSwitch.click(); | ||
| } | ||
|
|
||
| // Ensure Enable remote remediations is checked | ||
| const rhcSwitch = page.getByText('Enable remote remediations'); | ||
| if (!(await rhcSwitch.isChecked())) { | ||
| await rhcSwitch.click(); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for activation key related patterns to understand naming and usage
rg -n "activation-key" --type ts --type js -B 2 -A 2Repository: osbuild/image-builder-frontend
Length of output: 12242
🏁 Script executed:
#!/bin/bash
# Look for test fixtures, mock data, or configuration files that define activation keys
fd -e json -e ts -e js | xargs grep -l "activation.key\|activation-key" 2>/dev/null | head -20Repository: osbuild/image-builder-frontend
Length of output: 678
🏁 Script executed:
#!/bin/bash
# Search for the registerWithActivationKey function and its callers to understand context
rg -n "registerWithActivationKey" --type ts -B 3 -A 3Repository: osbuild/image-builder-frontend
Length of output: 1798
🏁 Script executed:
#!/bin/bash
# Look for other getByRole with option patterns to understand selector conventions
rg -n "getByRole.*option" --type ts -C 2Repository: osbuild/image-builder-frontend
Length of output: 18704
Consider using a more specific selector for the activation key option.
The partial name match 'activation-key-' on line 81 will select the first option that starts with this prefix. If the dropdown contains multiple activation keys with names starting with 'activation-key-', this selector risks selecting an unintended option. Use a more specific selector (e.g., the full key name or a data-testid attribute) to ensure the correct key is reliably selected.
🤖 Prompt for AI Agents
In @playwright/helpers/wizardHelpers.ts around lines 70-95, The selector in
registerWithActivationKey that clicks the activation key option currently uses a
vague prefix ('activation-key-') and can match the wrong item; update
registerWithActivationKey to select the exact activation key by either accepting
a parameter (e.g., activationKey) and using that full name in the
getByRole('option', { name: activationKey }) call, or by targeting a stable
attribute like a data-testid on the option (e.g.,
getByTestId('activation-key-<id>')), and replace the current partial-match
selector accordingly to guarantee the intended option is clicked.
e1dcdba to
700d6b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @playwright/BootTests/AAP/AAP.boot.ts:
- Around line 63-65: Prettier is failing on the test callback parameter
formatting in the test.skip declaration for "AAP registration boot integration
test"; change the async parameter destructuring from "async ({ page, cleanup })
=>" to remove spaces inside the braces so it reads "async ({page, cleanup}) =>"
(update the test.skip line in AAP.boot.ts accordingly) to satisfy Prettier.
In @playwright/BootTests/Compliance/ComplianceCIS.boot.ts:
- Around line 184-186: Prettier is flagging the test callback parameter
formatting for test.skip; reformat the async callback signature for the
'Compliance alerts - lint warnings display' test so it matches the repo's
Prettier rules (e.g., run the project's Prettier/formatter or adjust the
signature of test.skip(..., async ({ page, cleanup }) => { ... }) to the
expected single-line or multi-line style per config), then save and commit the
formatted change for test.skip and its async callback.
- Around line 34-36: Prettier is failing on the test callback parameter
formatting in the test.skip declaration; reformat the async callback signature
for the 'Compliance step integration test - CIS' test so it satisfies Prettier
(e.g. run Prettier or adjust the callback to a compliant style), specifically
update the test.skip callback that currently uses "async ({ page, cleanup }) =>"
to a properly formatted version (you can split the parameters onto their own
line or add explicit type annotations for { page, cleanup }) and then re-run
Prettier/CI to confirm the linting error is resolved.
In @playwright/helpers/wizardHelpers.ts:
- Around line 90-93: The code calls isChecked() on rhcSwitch which is created
with page.getByText('Enable remote remediations') (a text/label node) causing
runtime errors; change the locator to target the actual input/switch (e.g., use
page.getByLabel('Enable remote remediations') or a checkbox role locator like
page.locator('role=checkbox', { hasText: 'Enable remote remediations' }) so
rhcSwitch refers to the interactive element, then call isChecked() and click()
on that locator.
- Line 81: The selector using page.getByRole('option', { name: 'activation-key-'
}) is too vague and will pick the first option that starts with that prefix;
update the helper in wizardHelpers.ts to accept a specific activation key
parameter (e.g., activationKey) and replace the hardcoded partial name with an
explicit match (e.g., use the exact option name or build
`activation-key-${activationKey}`) when calling page.getByRole or otherwise
narrow the locator; ensure the calling sites pass the intended activation key
value so the correct option is selected.
- Around line 84-87: The locator insightsSwitch currently uses getByText('Enable
predictive analytics'), which targets a text/label and does not support
isChecked(); change the locator to target the actual control (e.g., use
getByLabel('Enable predictive analytics'), page.getByRole('switch', { name:
'Enable predictive analytics' }), or locate the input inside the label like
locator('label', { hasText: 'Enable predictive analytics'
}).locator('input[type="checkbox"]')) so that calling insightsSwitch.isChecked()
and insightsSwitch.click() operates on the real input element.
🧹 Nitpick comments (3)
playwright/BootTests/helpers/helpers.ts (1)
326-330: Consider using Playwright's built-in wait utilities.The custom
sleepfunction works but Playwright provides more robust waiting mechanisms that can handle race conditions better.For polling scenarios like this,
page.waitForTimeout()orexpect.poll()with custom predicates might be more idiomatic. However, the current implementation is acceptable for this use case.playwright/BootTests/Content/ContentTemplate.boot.ts (2)
186-194: FIXME: Workaround for wizard step loading requirement.The comment indicates this step shouldn't be necessary but is required to prevent package search failures. This suggests a potential wizard state management issue in the application. Consider tracking this as a bug to fix in the underlying wizard behavior.
Would you like me to open an issue to track this wizard step loading requirement as a bug to be fixed in the application?
245-277: Consider extracting polling constants.The polling configuration (30 attempts, 10,000ms delay) is defined inline. For consistency with
pollForSystemTemplateAttachmenton lines 304-310 which passes similar parameters, consider extracting these as named constants at the top of the test for clarity and easier tuning.♻️ Suggested refactor
+const REGISTRATION_POLL_DELAY_MS = 10_000; +const REGISTRATION_POLL_MAX_ATTEMPTS = 30; + test('Content integration test - Content Template', async ({ page, cleanup, }) => { // ... existing code ... await test.step('Wait for system registration to complete', async () => { - const maxAttempts = 30; - const delayMs = 10_000; // 10 seconds between attempts + const maxAttempts = REGISTRATION_POLL_MAX_ATTEMPTS; + const delayMs = REGISTRATION_POLL_DELAY_MS;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
playwright/BootTests/AAP/AAP.boot.tsplaywright/BootTests/Compliance/ComplianceCIS.boot.tsplaywright/BootTests/Content/ContentNonRepeatable.boot.tsplaywright/BootTests/Content/ContentRepeatable.boot.tsplaywright/BootTests/Content/ContentTemplate.boot.tsplaywright/BootTests/Satellite/Satellite.boot.tsplaywright/BootTests/helpers/helpers.tsplaywright/helpers/wizardHelpers.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- playwright/BootTests/Content/ContentNonRepeatable.boot.ts
- playwright/BootTests/Content/ContentRepeatable.boot.ts
🧰 Additional context used
🧬 Code graph analysis (2)
playwright/helpers/wizardHelpers.ts (1)
playwright/helpers/helpers.ts (1)
isHosted(27-29)
playwright/BootTests/helpers/helpers.ts (1)
playwright/helpers/helpers.ts (1)
closePopupsIfExist(31-55)
🪛 GitHub Actions: Development checks
playwright/BootTests/AAP/AAP.boot.ts
[error] 65-65: Prettier formatting check failed. Replace ·page,·cleanup· with ⏎··page,⏎··cleanup,⏎.
🪛 GitHub Check: ESLint
playwright/BootTests/helpers/helpers.ts
[warning] 429-429:
Unnecessary conditional, value is always truthy
playwright/BootTests/AAP/AAP.boot.ts
[failure] 65-65:
Replace ·page,·cleanup· with ⏎··page,⏎··cleanup,⏎
playwright/BootTests/Compliance/ComplianceCIS.boot.ts
[failure] 36-36:
Replace ·page,·cleanup· with ⏎··page,⏎··cleanup,⏎
[failure] 186-186:
Replace ·page,·cleanup· with ⏎··page,⏎··cleanup,⏎
⏰ 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). (3)
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
- GitHub Check: Service Tests
🔇 Additional comments (7)
playwright/BootTests/Satellite/Satellite.boot.ts (1)
29-34: LGTM!The test skip is properly formatted and documented with a TODO comment.
playwright/BootTests/Content/ContentTemplate.boot.ts (6)
65-87: LGTM!Repository creation and snapshot waiting steps are well-structured with appropriate timeouts for asynchronous operations.
89-158: LGTM!Content template creation step is thorough with proper waits for async operations and clear assertions at each wizard step.
222-243: LGTM!Build, deployment, and package verification steps are clean and follow the expected flow.
279-291: LGTM!Good verification approach to confirm the system is using content template URLs by checking the package download location.
293-315: LGTM with noted limitations.Good practice to re-authenticate before API calls after long-running operations. The polling logic is appropriate.
Per the PR objectives, steps 10 and 11 (including this verification) are currently failing due to subscription/registration issues against stage environment. The implementation looks correct; the failure appears to be an environmental/configuration issue rather than a code defect.
54-58: The review comment is incorrect. ThedeleteRepositoryfunction explicitly supports both name and URL identifiers through a singlerepositoryNameOrUrl: stringparameter. The function uses a 'Name/URL filter' textbox and includes a comment stating "We check for list size due to the need of deleting by the name OR url and there is no better selector for that." Both calls in the code—passingrepositoryUrlon line 54 andrepositoryNameon line 58—are valid and intended.Likely an incorrect or invalid review comment.
|
|
||
| // Select an activation key from the dropdown | ||
| await page.getByRole('button', { name: 'Menu toggle' }).click(); | ||
| await page.getByRole('option', { name: 'activation-key-' }).click(); |
There was a problem hiding this comment.
Vague selector may select the wrong activation key.
The partial name match 'activation-key-' will select the first option starting with this prefix. If multiple activation keys exist in the dropdown, this risks selecting an unintended key.
💡 Suggested improvement
Consider parameterizing the function to accept the specific activation key name:
-export const registerWithActivationKey = async (page: Page | FrameLocator) => {
+export const registerWithActivationKey = async (
+ page: Page | FrameLocator,
+ activationKeyName: string = 'activation-key-',
+) => {
if (isHosted()) {
await page.getByRole('button', { name: 'Register' }).click();
// Ensure "Automatically register to Red Hat" is selected (should be default)
await expect(
page.getByRole('radio', { name: 'Automatically register to Red Hat' }),
).toBeChecked();
// Select an activation key from the dropdown
await page.getByRole('button', { name: 'Menu toggle' }).click();
- await page.getByRole('option', { name: 'activation-key-' }).click();
+ await page.getByRole('option', { name: activationKeyName }).click();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @playwright/helpers/wizardHelpers.ts at line 81, The selector using
page.getByRole('option', { name: 'activation-key-' }) is too vague and will pick
the first option that starts with that prefix; update the helper in
wizardHelpers.ts to accept a specific activation key parameter (e.g.,
activationKey) and replace the hardcoded partial name with an explicit match
(e.g., use the exact option name or build `activation-key-${activationKey}`)
when calling page.getByRole or otherwise narrow the locator; ensure the calling
sites pass the intended activation key value so the correct option is selected.
| const insightsSwitch = page.getByText('Enable predictive analytics'); | ||
| if (!(await insightsSwitch.isChecked())) { | ||
| await insightsSwitch.click(); | ||
| } |
There was a problem hiding this comment.
Critical: isChecked() called on text element.
Line 85 calls isChecked() on the result of getByText('Enable predictive analytics'), which locates a text/label element, not the actual switch/checkbox input. This will either throw a runtime error or always evaluate to false.
🔧 Proposed fix
// Ensure Enable predictive analytics is checked
- const insightsSwitch = page.getByText('Enable predictive analytics');
+ const insightsSwitch = page.getByRole('switch', { name: 'Enable predictive analytics' });
if (!(await insightsSwitch.isChecked())) {
await insightsSwitch.click();
}If the element is not a switch role, use getByLabel('Enable predictive analytics') or locate the input within the container.
📝 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 insightsSwitch = page.getByText('Enable predictive analytics'); | |
| if (!(await insightsSwitch.isChecked())) { | |
| await insightsSwitch.click(); | |
| } | |
| const insightsSwitch = page.getByRole('switch', { name: 'Enable predictive analytics' }); | |
| if (!(await insightsSwitch.isChecked())) { | |
| await insightsSwitch.click(); | |
| } |
🤖 Prompt for AI Agents
In @playwright/helpers/wizardHelpers.ts around lines 84 - 87, The locator
insightsSwitch currently uses getByText('Enable predictive analytics'), which
targets a text/label and does not support isChecked(); change the locator to
target the actual control (e.g., use getByLabel('Enable predictive analytics'),
page.getByRole('switch', { name: 'Enable predictive analytics' }), or locate the
input inside the label like locator('label', { hasText: 'Enable predictive
analytics' }).locator('input[type="checkbox"]')) so that calling
insightsSwitch.isChecked() and insightsSwitch.click() operates on the real input
element.
| const rhcSwitch = page.getByText('Enable remote remediations'); | ||
| if (!(await rhcSwitch.isChecked())) { | ||
| await rhcSwitch.click(); | ||
| } |
There was a problem hiding this comment.
Critical: isChecked() called on text element.
Line 91 calls isChecked() on the result of getByText('Enable remote remediations'), which locates a text/label element, not the actual switch/checkbox input. This will either throw a runtime error or always evaluate to false.
🔧 Proposed fix
// Ensure Enable remote remediations is checked
- const rhcSwitch = page.getByText('Enable remote remediations');
+ const rhcSwitch = page.getByRole('switch', { name: 'Enable remote remediations' });
if (!(await rhcSwitch.isChecked())) {
await rhcSwitch.click();
}If the element is not a switch role, use getByLabel('Enable remote remediations') or locate the input within the container.
📝 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 rhcSwitch = page.getByText('Enable remote remediations'); | |
| if (!(await rhcSwitch.isChecked())) { | |
| await rhcSwitch.click(); | |
| } | |
| // Ensure Enable remote remediations is checked | |
| const rhcSwitch = page.getByRole('switch', { name: 'Enable remote remediations' }); | |
| if (!(await rhcSwitch.isChecked())) { | |
| await rhcSwitch.click(); | |
| } |
🤖 Prompt for AI Agents
In @playwright/helpers/wizardHelpers.ts around lines 90 - 93, The code calls
isChecked() on rhcSwitch which is created with page.getByText('Enable remote
remediations') (a text/label node) causing runtime errors; change the locator to
target the actual input/switch (e.g., use page.getByLabel('Enable remote
remediations') or a checkbox role locator like page.locator('role=checkbox', {
hasText: 'Enable remote remediations' }) so rhcSwitch refers to the interactive
element, then call isChecked() and click() on that locator.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
playwright/BootTests/Content/ContentTemplate.boot.ts (2)
186-194: FIXME noted — consider tracking this issue.This workaround indicates a bug where the package search fails unless the Repositories step is explicitly loaded first. Consider opening an issue to track fixing this in the application so the workaround can be removed.
Would you like me to open an issue to track this bug?
203-206: Fragile selector assumes package is always in first row.
getByRole('checkbox', { name: 'Select row 0' })will fail if the search returns multiple packages or if the expected package isn't in the first position. Consider using a more specific selector that targets the row containingpackageName.♻️ Suggested improvement
- await expect( - frame.getByRole('gridcell', { name: packageName }), - ).toBeVisible({ timeout: 60000 }); - await frame.getByRole('checkbox', { name: 'Select row 0' }).click(); + await expect( + frame.getByRole('gridcell', { name: packageName }), + ).toBeVisible({ timeout: 60000 }); + // Select the row containing our specific package + const packageRow = frame.locator('tr').filter({ hasText: packageName }); + await packageRow.getByRole('checkbox').click();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
playwright/BootTests/Content/ContentTemplate.boot.ts
⏰ 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). (22)
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: Sourcery review
- GitHub Check: boot-tests
- GitHub Check: playwright-tests
- GitHub Check: Service Tests
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
🔇 Additional comments (6)
playwright/BootTests/Content/ContentTemplate.boot.ts (6)
1-33: LGTM on imports.The imports are well-organized and cover all necessary helpers for the test workflow.
65-87: LGTM on repository creation.The step correctly creates a repository and waits for the snapshot to become valid with an appropriate timeout.
89-158: LGTM on template creation workflow.The template creation step thoroughly covers the wizard flow, including architecture/OS selection, repository inclusion, and waiting for the template to reach "Valid" status.
222-236: LGTM on build and boot steps.The image building and OpenStack instance provisioning steps are cleanly organized using helper abstractions.
245-277: Polling logic is sound, minor suggestion on API usage.The registration polling is implemented correctly. Consider using
page.waitForTimeout(delayMs)instead of rawsetTimeoutfor consistency with Playwright's API, though since this step runs SSH commands rather than page interactions, the current approach is acceptable.
279-301: LGTM on template attachment verification.Good practice to re-authenticate and refresh tokens after long-running build operations. The assertion includes a descriptive failure message for debugging.
This test 0. logs into consoledot 1. creates a custom repo 2. creates a content template 3. creates a blueprint with the content template 4. installs a package from the content template 5. builds the image 6. downloads and uploads it to openstack 7. launches it in openstack 8. verifies the image content 9. verifies the instance is connected to the content template
ochosi
force-pushed
the
content-templates-boot-test
branch
from
be9f516 to
af56f50
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @playwright/BootTests/AAP/AAP.boot.ts:
- Around line 63-65: The test callback parameter list is causing Prettier
formatting issues; remove the unused "cleanup" parameter from the async test
callback to resolve it. Locate the test.skip invocation for "AAP registration
boot integration test" (the line containing test.skip('AAP registration boot
integration test', async ({ page, cleanup }) => { ) and change the parameter
object to only include { page } (or name it { page, _cleanup } if the variable
must remain referenced), then run Prettier/format to ensure CI passes.
In @playwright/BootTests/Compliance/ComplianceCIS.boot.ts:
- Around line 184-186: The test callback's parameter destructuring is causing
Prettier formatting errors; change the signature of the skipped test from using
inline destructured params to a single parameter and destructure inside the
function body to satisfy Prettier: replace the callback async ({ page, cleanup
}) => { ... } used in test.skip('Compliance alerts - lint warnings display',
...) with async (fixtures) => { const { page, cleanup } = fixtures; ... } so
formatting rules are respected while keeping the same fixtures available.
- Around line 34-36: The test declaration's callback parameter spacing violates
Prettier; update the test.skip signature for "Compliance step integration test -
CIS" to use Prettier-compliant parameter spacing by changing the async callback
parameter from "async ({ page, cleanup }) =>" to "async ({page, cleanup}) =>"
(adjust spacing inside the parentheses/braces to match project Prettier rules)
so the file formats without errors.
In @playwright/BootTests/helpers/helpers.ts:
- Around line 294-311: The deleteTemplate function currently wraps
navigateToTemplates, the search input fill, and the visibility assertion in a
single try/catch which treats any error as "template not found"; instead, only
swallow the absence-of-template check: call navigateToTemplates(page) and
perform page.getByRole('searchbox', { name: 'Filter by name'
}).fill(templateName) outside of the try/catch so navigation/fill failures
surface, then narrow the try/catch to only the existence check (e.g., inspect
page.getByRole('row').filter({ hasText: templateName }) via a non-throwing check
like locator.count()/isVisible() or catch only the assertion error from expect)
so that genuine failures still fail the test while missing templates return
gracefully.
- Around line 341-436: The function pollForSystemTemplateAttachment has weak
typing for the API shape and an unnecessary shouldRetry conditional; fix by
defining a proper TypeScript interface for the API payload (e.g., interface
System { id: string; attributes: { display_name: string; template_uuid?: string;
template_name?: string; /* other fields */ } } and use body.data as System[]),
update the system find to use that type, and remove the redundant shouldRetry
check by simplifying the retry loop (return early on success, set a local flag
or simply await sleep(delayMs) when not last attempt), eliminating the final if
(shouldRetry && ...) branch and relying on attempts < maxAttempts to control
retries.
🧹 Nitpick comments (1)
playwright/BootTests/Content/ContentTemplate.boot.ts (1)
186-194: Technical debt: Repository step workaround.The FIXME comment indicates that loading the Repositories wizard step is required for package search to work. This suggests an initialization or timing issue in the wizard's state management.
Consider opening an issue to investigate why the Repositories step must be visited for package search to function correctly. The root cause may be related to lazy loading or state initialization in the wizard.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
playwright/BootTests/AAP/AAP.boot.tsplaywright/BootTests/Compliance/ComplianceCIS.boot.tsplaywright/BootTests/Content/ContentNonRepeatable.boot.tsplaywright/BootTests/Content/ContentRepeatable.boot.tsplaywright/BootTests/Content/ContentTemplate.boot.tsplaywright/BootTests/Satellite/Satellite.boot.tsplaywright/BootTests/helpers/helpers.tsplaywright/helpers/wizardHelpers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- playwright/helpers/wizardHelpers.ts
🧰 Additional context used
🧬 Code graph analysis (1)
playwright/BootTests/Content/ContentTemplate.boot.ts (6)
playwright/helpers/helpers.ts (1)
isHosted(27-29)playwright/BootTests/helpers/imageBuilding.ts (2)
buildImage(3-35)downloadImage(37-50)playwright/helpers/wizardHelpers.ts (4)
deleteBlueprint(113-145)registerWithActivationKey(70-95)fillInDetails(37-49)createBlueprint(16-27)playwright/BootTests/helpers/OpenStackWrapper.ts (1)
OpenStackWrapper(5-288)playwright/helpers/login.ts (2)
ensureAuthenticated(43-69)storeStorageStateAndToken(133-151)playwright/helpers/navHelpers.ts (3)
navigateToLandingPage(63-69)ibFrame(50-57)fillInImageOutput(18-44)
🪛 GitHub Actions: Development checks
playwright/BootTests/AAP/AAP.boot.ts
[error] 65-65: prettier/prettier: Replace ·page,·cleanup· with proper line breaks. Run 'prettier --write' to fix code style issues in this file.
🪛 GitHub Check: ESLint
playwright/BootTests/helpers/helpers.ts
[warning] 429-429:
Unnecessary conditional, value is always truthy
playwright/BootTests/AAP/AAP.boot.ts
[failure] 65-65:
Replace ·page,·cleanup· with ⏎··page,⏎··cleanup,⏎
playwright/BootTests/Compliance/ComplianceCIS.boot.ts
[failure] 36-36:
Replace ·page,·cleanup· with ⏎··page,⏎··cleanup,⏎
[failure] 186-186:
Replace ·page,·cleanup· with ⏎··page,⏎··cleanup,⏎
⏰ 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). (22)
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
- GitHub Check: Service Tests
- GitHub Check: playwright-tests
- GitHub Check: boot-tests
🔇 Additional comments (4)
playwright/BootTests/Content/ContentTemplate.boot.ts (2)
245-277: Registration polling is appropriate given known stage limitations.The manual polling for subscription-manager status aligns with the PR objectives, which note that registration against stage does not complete despite images being built with activation keys. This workaround allows the test to proceed locally and on production while documenting the stage-specific issue.
The 5-minute timeout (30 attempts × 10 seconds) provides reasonable buffer time for asynchronous registration to complete.
35-301: Comprehensive and well-structured integration test.The test effectively covers the complete content-template workflow:
- Repository creation with snapshot validation
- Template creation through multi-step wizard
- Blueprint creation with template selection
- Image build and OpenStack provisioning
- Package installation verification
- System registration and template attachment validation
The test includes proper cleanup, appropriate timeouts for long-running operations, and robust UI interaction patterns with retries and visibility checks.
playwright/BootTests/helpers/helpers.ts (1)
272-285: LGTM! Clean navigation helper.The
navigateToTemplatesfunction follows the same pattern asnavigateToRepositoriesand provides a clear way to navigate to the templates page with appropriate timeout handling.playwright/BootTests/Content/ContentNonRepeatable.boot.ts (1)
27-29: Reminder: Re-enable skipped tests after ContentTemplate debugging.All three non-repeatable content integration tests are temporarily disabled while debugging the ContentTemplate test. The test scaffolding remains intact, which is good for quick re-enablement.
When is the plan to re-enable these tests? Consider tracking the re-enablement with a dedicated issue to avoid these tests being forgotten.
Also applies to: 144-146, 301-303
| // TODO: Remove skip - temporarily disabled while debugging ContentTemplate test | ||
| // eslint-disable-next-line playwright/no-skipped-test | ||
| test.skip('AAP registration boot integration test', async ({ page, cleanup }) => { |
There was a problem hiding this comment.
Fix Prettier formatting to unblock CI/CD.
The test callback parameters violate Prettier rules. This issue was previously flagged and is blocking the pipeline.
🔧 Proposed fix
-test.skip('AAP registration boot integration test', async ({ page, cleanup }) => {
+test.skip('AAP registration boot integration test', async ({
+ page,
+ cleanup,
+}) => {🧰 Tools
🪛 GitHub Actions: Development checks
[error] 65-65: prettier/prettier: Replace ·page,·cleanup· with proper line breaks. Run 'prettier --write' to fix code style issues in this file.
🪛 GitHub Check: ESLint
[failure] 65-65:
Replace ·page,·cleanup· with ⏎··page,⏎··cleanup,⏎
🤖 Prompt for AI Agents
In @playwright/BootTests/AAP/AAP.boot.ts around lines 63 - 65, The test callback
parameter list is causing Prettier formatting issues; remove the unused
"cleanup" parameter from the async test callback to resolve it. Locate the
test.skip invocation for "AAP registration boot integration test" (the line
containing test.skip('AAP registration boot integration test', async ({ page,
cleanup }) => { ) and change the parameter object to only include { page } (or
name it { page, _cleanup } if the variable must remain referenced), then run
Prettier/format to ensure CI passes.
| // TODO: Remove skip - temporarily disabled while debugging ContentTemplate test | ||
| // eslint-disable-next-line playwright/no-skipped-test | ||
| test.skip('Compliance step integration test - CIS', async ({ page, cleanup }) => { |
There was a problem hiding this comment.
Fix Prettier formatting to unblock CI/CD.
The test callback parameters violate Prettier rules. This issue was previously flagged and is blocking the pipeline.
🔧 Proposed fix
-test.skip('Compliance step integration test - CIS', async ({ page, cleanup }) => {
+test.skip('Compliance step integration test - CIS', async ({
+ page,
+ cleanup,
+}) => {📝 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.
| // TODO: Remove skip - temporarily disabled while debugging ContentTemplate test | |
| // eslint-disable-next-line playwright/no-skipped-test | |
| test.skip('Compliance step integration test - CIS', async ({ page, cleanup }) => { | |
| // TODO: Remove skip - temporarily disabled while debugging ContentTemplate test | |
| // eslint-disable-next-line playwright/no-skipped-test | |
| test.skip('Compliance step integration test - CIS', async ({ | |
| page, | |
| cleanup, | |
| }) => { |
🧰 Tools
🪛 GitHub Check: ESLint
[failure] 36-36:
Replace ·page,·cleanup· with ⏎··page,⏎··cleanup,⏎
🤖 Prompt for AI Agents
In @playwright/BootTests/Compliance/ComplianceCIS.boot.ts around lines 34 - 36,
The test declaration's callback parameter spacing violates Prettier; update the
test.skip signature for "Compliance step integration test - CIS" to use
Prettier-compliant parameter spacing by changing the async callback parameter
from "async ({ page, cleanup }) =>" to "async ({page, cleanup}) =>" (adjust
spacing inside the parentheses/braces to match project Prettier rules) so the
file formats without errors.
| // TODO: Remove skip - temporarily disabled while debugging ContentTemplate test | ||
| // eslint-disable-next-line playwright/no-skipped-test | ||
| test.skip('Compliance alerts - lint warnings display', async ({ page, cleanup }) => { |
There was a problem hiding this comment.
Fix Prettier formatting to unblock CI/CD.
The test callback parameters violate Prettier rules. This issue was previously flagged and is blocking the pipeline.
🔧 Proposed fix
-test.skip('Compliance alerts - lint warnings display', async ({ page, cleanup }) => {
+test.skip('Compliance alerts - lint warnings display', async ({
+ page,
+ cleanup,
+}) => {📝 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.
| // TODO: Remove skip - temporarily disabled while debugging ContentTemplate test | |
| // eslint-disable-next-line playwright/no-skipped-test | |
| test.skip('Compliance alerts - lint warnings display', async ({ page, cleanup }) => { | |
| // TODO: Remove skip - temporarily disabled while debugging ContentTemplate test | |
| // eslint-disable-next-line playwright/no-skipped-test | |
| test.skip('Compliance alerts - lint warnings display', async ({ | |
| page, | |
| cleanup, | |
| }) => { |
🧰 Tools
🪛 GitHub Check: ESLint
[failure] 186-186:
Replace ·page,·cleanup· with ⏎··page,⏎··cleanup,⏎
🤖 Prompt for AI Agents
In @playwright/BootTests/Compliance/ComplianceCIS.boot.ts around lines 184 -
186, The test callback's parameter destructuring is causing Prettier formatting
errors; change the signature of the skipped test from using inline destructured
params to a single parameter and destructure inside the function body to satisfy
Prettier: replace the callback async ({ page, cleanup }) => { ... } used in
test.skip('Compliance alerts - lint warnings display', ...) with async
(fixtures) => { const { page, cleanup } = fixtures; ... } so formatting rules
are respected while keeping the same fixtures available.
| export const deleteTemplate = async (page: Page, templateName: string) => { | ||
| await closePopupsIfExist(page); | ||
| await test.step( | ||
| 'Delete the template with name: ' + templateName, | ||
| async () => { | ||
| try { | ||
| await navigateToTemplates(page); | ||
| await page | ||
| .getByRole('searchbox', { name: 'Filter by name' }) | ||
| .fill(templateName); | ||
| // Wait for the template row to appear | ||
| await expect( | ||
| page.getByRole('row').filter({ hasText: templateName }), | ||
| ).toBeVisible({ timeout: 10000 }); | ||
| } catch { | ||
| // No template of given name was found -> fail gracefully and do not raise error | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Broad try/catch still swallows navigation and search errors.
The try block wraps navigation, search input, and the visibility check. Any failure (network issues, locator changes, navigation problems) will be silently treated as "template not found" and the function will return early. This was flagged in a previous review.
Consider narrowing the error handling to specifically detect when the template doesn't exist:
♻️ Proposed refactor to narrow error handling
export const deleteTemplate = async (page: Page, templateName: string) => {
await closePopupsIfExist(page);
await test.step(
'Delete the template with name: ' + templateName,
async () => {
- try {
- await navigateToTemplates(page);
- await page
- .getByRole('searchbox', { name: 'Filter by name' })
- .fill(templateName);
- // Wait for the template row to appear
- await expect(
- page.getByRole('row').filter({ hasText: templateName }),
- ).toBeVisible({ timeout: 10000 });
- } catch {
- // No template of given name was found -> fail gracefully and do not raise error
+ await navigateToTemplates(page);
+ await page
+ .getByRole('searchbox', { name: 'Filter by name' })
+ .fill(templateName);
+
+ const templateRow = page.getByRole('row').filter({ hasText: templateName });
+ const rowCount = await templateRow.count();
+
+ if (rowCount === 0) {
return;
}
- await page
- .getByRole('row')
- .filter({ hasText: templateName })
- .getByLabel('Kebab toggle')
- .click();
+ await templateRow.getByLabel('Kebab toggle').click();
await page.getByRole('menuitem', { name: 'Delete' }).click();
await expect(page.getByText('Delete template?')).toBeVisible();
await page.getByRole('button', { name: 'Delete' }).click();
},
{ box: true },
);
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @playwright/BootTests/helpers/helpers.ts around lines 294 - 311, The
deleteTemplate function currently wraps navigateToTemplates, the search input
fill, and the visibility assertion in a single try/catch which treats any error
as "template not found"; instead, only swallow the absence-of-template check:
call navigateToTemplates(page) and perform page.getByRole('searchbox', { name:
'Filter by name' }).fill(templateName) outside of the try/catch so
navigation/fill failures surface, then narrow the try/catch to only the
existence check (e.g., inspect page.getByRole('row').filter({ hasText:
templateName }) via a non-throwing check like locator.count()/isVisible() or
catch only the assertion error from expect) so that genuine failures still fail
the test while missing templates return gracefully.
This way we can easily double-check in our boot tests if a system successfully registered and appears in the Insights inventory.
This way we can discern if the system registered, but Patch had delays in ingesting the system information.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @playwright/BootTests/helpers/helpers.ts:
- Around line 382-385: The found `system` object is under-typed and later
accessed for `template_uuid` and `template_name`, so update its type to include
those properties (e.g., an interface/type with attributes: { display_name:
string; template_uuid?: string; template_name?: string } or use the API response
type) and use that type in the body.data.find callback and the `system` variable
to ensure TypeScript checks those accesses; also remove the redundant
conditional around `shouldRetry` (the always-truthy check flagged by ESLint) by
simplifying the logic where `shouldRetry` is evaluated (eliminate the
unnecessary `if (shouldRetry)` branch and directly handle the retry/return
behavior consistent with the early return at line 410).
🧹 Nitpick comments (4)
playwright/BootTests/helpers/helpers.ts (1)
446-518: Consider adding type definition for consistency.Similar to
pollForSystemTemplateAttachment, the inline type{ display_name?: string; fqdn?: string }at line 488 could be extracted to an interface for better maintainability. Optional, but would improve consistency across both polling functions.♻️ Optional refactor
interface InventorySystem { id: string; display_name?: string; fqdn?: string; } // Then use: const system = body.results.find( (sys: InventorySystem) => sys.display_name === hostname || sys.fqdn === hostname, );playwright/BootTests/Content/ContentTemplate.boot.ts (3)
246-278:returninsidetest.stepcallback only exits the step, not the test.At line 264,
return;exits thetest.stepcallback but the test continues executing subsequent steps. This appears intentional (early exit on success), but could be clearer.Also, consider extracting this polling logic to a helper (similar to
pollForSystemInInventory) to reduce duplication and improve reusability.♻️ Consider extracting to a helper
// In helpers.ts export const pollForSystemRegistration = async ( image: OpenStackWrapper, delayMs: number = 10_000, maxAttempts: number = 30, ): Promise<void> => { for (let attempt = 1; attempt <= maxAttempts; attempt++) { const [exitCode, output] = await image.exec('sudo subscription-manager status'); console.log(`Registration check attempt ${attempt}/${maxAttempts}: exit=${exitCode}`); if (exitCode === 0) { console.log('System registration complete:', output); return; } if (attempt < maxAttempts) { await new Promise((resolve) => setTimeout(resolve, delayMs)); } } throw new Error(`System did not register within ${(maxAttempts * delayMs) / 1000} seconds`); };
81-88: Long timeout may mask repository creation issues.The 180-second timeout for waiting for "Valid" status is very long. Consider logging periodically or adding intermediate checks to help diagnose slow or failing repository snapshots during debugging.
187-195: FIXME comment indicates a workaround for a wizard bug.The comment at line 187 notes this step "shouldn't be necessary." Consider filing an issue to track this wizard behavior for future resolution.
Would you like me to help create an issue to track this wizard quirk where the Repositories step must be visited for package search to work?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
playwright/BootTests/Content/ContentTemplate.boot.tsplaywright/BootTests/helpers/helpers.ts
🧰 Additional context used
🧬 Code graph analysis (1)
playwright/BootTests/helpers/helpers.ts (1)
playwright/helpers/helpers.ts (1)
closePopupsIfExist(31-55)
🪛 GitHub Check: ESLint
playwright/BootTests/helpers/helpers.ts
[warning] 429-429:
Unnecessary conditional, value is always truthy
⏰ 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). (22)
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
- GitHub Check: Service Tests
- GitHub Check: playwright-tests
- GitHub Check: boot-tests
🔇 Additional comments (5)
playwright/BootTests/helpers/helpers.ts (2)
272-285: LGTM!Clean navigation helper with appropriate timeout for visibility check.
326-330: LGTM!Simple sleep utility.
playwright/BootTests/Content/ContentTemplate.boot.ts (3)
1-35: LGTM!Imports are well-organized with clear separation between Playwright test utilities, helper functions, and wrapper classes.
36-63: Good cleanup pattern with proper resource management.The cleanup registration order is correct—resources are cleaned up in reverse order of creation.
280-297: Good practice: re-authenticating after long-running operations.Refreshing authentication and token after the long build/boot process is a sensible approach to handle session expiration.
| const system = body.data.find( | ||
| (sys: { attributes: { display_name: string } }) => | ||
| sys.attributes.display_name === hostname, | ||
| ); |
There was a problem hiding this comment.
Improve type safety and remove redundant conditional.
-
Partial typing:
systemis typed as{ attributes: { display_name: string } }but you accesstemplate_uuid(line 394) andtemplate_name(line 397) which aren't in the type. TypeScript won't catch API shape changes. -
Redundant conditional at line 429: ESLint correctly flags that
shouldRetryis always truthy at this point—any successful path returns early at line 410.
🔧 Suggested fixes
+interface PatchSystem {
+ attributes: {
+ display_name: string;
+ template_uuid?: string;
+ template_name?: string;
+ };
+}
+
export const pollForSystemTemplateAttachment = async (
// ... params
): Promise<boolean> => {- const body = await response.json();
+ const body: { data?: PatchSystem[] } = await response.json();
// ...
const system = body.data.find(
- (sys: { attributes: { display_name: string } }) =>
+ (sys: PatchSystem) =>
sys.attributes.display_name === hostname,
); // Check if we should retry with delay
- if (shouldRetry && attempts < maxAttempts) {
+ if (attempts < maxAttempts) {
await sleep(delayMs);
}Also applies to: 394-394, 397-397, 429-429
🤖 Prompt for AI Agents
In @playwright/BootTests/helpers/helpers.ts around lines 382 - 385, The found
`system` object is under-typed and later accessed for `template_uuid` and
`template_name`, so update its type to include those properties (e.g., an
interface/type with attributes: { display_name: string; template_uuid?: string;
template_name?: string } or use the API response type) and use that type in the
body.data.find callback and the `system` variable to ensure TypeScript checks
those accesses; also remove the redundant conditional around `shouldRetry` (the
always-truthy check flagged by ESLint) by simplifying the logic where
`shouldRetry` is evaluated (eliminate the unnecessary `if (shouldRetry)` branch
and directly handle the retry/return behavior consistent with the early return
at line 410).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @playwright/BootTests/helpers/helpers.ts:
- Around line 341-436: The retry check in pollForSystemTemplateAttachment is
redundant: remove the shouldRetry flag (declaration and all assignments) and
replace the final conditional that currently checks "if (shouldRetry && attempts
< maxAttempts)" with a simpler "if (attempts < maxAttempts)" so the loop sleeps
when there are remaining attempts; keep all existing return true paths and the
catch behavior, and retain the sleep(delayMs) call and attempts increment logic
unchanged.
🧹 Nitpick comments (1)
playwright/BootTests/helpers/helpers.ts (1)
446-518: LGTM with operational note.The polling implementation is cleaner than
pollForSystemTemplateAttachment(no redundantshouldRetryflag). The logic correctly checks bothdisplay_nameandfqdnfields for hostname matching.
Be aware of the extended maximum wait time.
With default parameters (20s delay × 30 attempts), this function can wait up to 10 minutes. While appropriate for system registration and inventory sync scenarios, this could significantly extend test execution time if systems don't appear as expected.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
playwright/BootTests/helpers/helpers.ts
🧰 Additional context used
🪛 GitHub Check: ESLint
playwright/BootTests/helpers/helpers.ts
[warning] 429-429:
Unnecessary conditional, value is always truthy
⏰ 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). (21)
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: Sourcery review
- GitHub Check: Service Tests
- GitHub Check: playwright-tests
- GitHub Check: boot-tests
🔇 Additional comments (3)
playwright/BootTests/helpers/helpers.ts (3)
273-285: LGTM!The navigation helper follows the established pattern and includes appropriate wait conditions.
287-324: LGTM!The delete helper follows the established pattern with appropriate graceful failure handling when the template is not found.
326-330: LGTM!Simple internal utility for the polling helpers.
| export const pollForSystemTemplateAttachment = async ( | ||
| page: Page, | ||
| hostname: string, | ||
| expectedTemplateName?: string, | ||
| delayMs: number = 10_000, | ||
| maxAttempts: number = 30, | ||
| ): Promise<boolean> => { | ||
| /* eslint-disable no-console */ | ||
| let attempts = 0; | ||
|
|
||
| while (attempts < maxAttempts) { | ||
| attempts++; | ||
| let shouldRetry = false; | ||
|
|
||
| try { | ||
| // Query the systems API with search filter for the hostname | ||
| // Include Authorization header if TOKEN is available (set during login) | ||
| const headers: Record<string, string> = {}; | ||
| if (process.env.TOKEN) { | ||
| headers['Authorization'] = process.env.TOKEN; | ||
| } | ||
| const response = await page.request.get( | ||
| `/api/patch/v3/systems?search=${encodeURIComponent(hostname)}&limit=100`, | ||
| { headers }, | ||
| ); | ||
|
|
||
| if (response.status() !== 200) { | ||
| console.log( | ||
| `API request failed with status ${response.status()}, attempt ${attempts}/${maxAttempts}`, | ||
| ); | ||
| shouldRetry = true; | ||
| } else { | ||
| const body = await response.json(); | ||
|
|
||
| if (!body.data || !Array.isArray(body.data)) { | ||
| console.log( | ||
| `Invalid response format, attempt ${attempts}/${maxAttempts}`, | ||
| ); | ||
| shouldRetry = true; | ||
| } else { | ||
| // Find the system with matching hostname | ||
| const system = body.data.find( | ||
| (sys: { attributes: { display_name: string } }) => | ||
| sys.attributes.display_name === hostname, | ||
| ); | ||
|
|
||
| if (!system) { | ||
| console.log( | ||
| `System '${hostname}' not found in inventory, attempt ${attempts}/${maxAttempts}`, | ||
| ); | ||
| shouldRetry = true; | ||
| } else { | ||
| // Check if system has a template_uuid assigned | ||
| const hasTemplate = !!system.attributes?.template_uuid; | ||
|
|
||
| if (hasTemplate) { | ||
| const templateName = system.attributes.template_name; | ||
| if ( | ||
| expectedTemplateName && | ||
| templateName !== expectedTemplateName | ||
| ) { | ||
| console.log( | ||
| `System '${hostname}' is attached to template '${templateName}' but expected '${expectedTemplateName}', attempt ${attempts}/${maxAttempts}`, | ||
| ); | ||
| shouldRetry = true; | ||
| } else { | ||
| console.log( | ||
| `System '${hostname}' is attached to template: ${templateName}`, | ||
| ); | ||
| return true; | ||
| } | ||
| } else { | ||
| console.log( | ||
| `System '${hostname}' is not attached to any template, attempt ${attempts}/${maxAttempts}`, | ||
| ); | ||
| shouldRetry = true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.log( | ||
| `Error checking system template attachment: ${error}, attempt ${attempts}/${maxAttempts}`, | ||
| ); | ||
| shouldRetry = true; | ||
| } | ||
|
|
||
| // Check if we should retry with delay | ||
| if (shouldRetry && attempts < maxAttempts) { | ||
| await sleep(delayMs); | ||
| } | ||
| } | ||
| /* eslint-enable no-console */ | ||
|
|
||
| return false; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check where TOKEN is set in the test suite
rg -n "process\.env\.TOKEN" playwright/BootTests/ --type=ts -B 2 -A 2Repository: osbuild/image-builder-frontend
Length of output: 1142
🏁 Script executed:
# Also search for TOKEN assignment in login or setup
rg -n "TOKEN\s*=" playwright/BootTests/ --type=tsRepository: osbuild/image-builder-frontend
Length of output: 56
🏁 Script executed:
# Look at the helpers file structure to understand test setup
fd "helpers\.ts|login\." playwright/BootTests/ --type fRepository: osbuild/image-builder-frontend
Length of output: 113
🏁 Script executed:
# Look for login function and TOKEN setup
rg -n "TOKEN" playwright/BootTests/ --type=ts -B 3 -A 3 | head -100Repository: osbuild/image-builder-frontend
Length of output: 2552
🏁 Script executed:
# Check for .env files or environment setup
fd "\.(env|config)" playwright/BootTests/ --type fRepository: osbuild/image-builder-frontend
Length of output: 56
🏁 Script executed:
# Look for test fixtures or setup files
fd "(setup|fixture|before)" playwright/BootTests/ --type=tsRepository: osbuild/image-builder-frontend
Length of output: 243
🏁 Script executed:
# Check the structure of the BootTests directory
ls -la playwright/BootTests/ | head -20Repository: osbuild/image-builder-frontend
Length of output: 520
🏁 Script executed:
# Find the storeStorageStateAndToken function
rg -n "storeStorageStateAndToken" playwright/BootTests/ --type=ts -B 2 -A 10Repository: osbuild/image-builder-frontend
Length of output: 2293
🏁 Script executed:
# Check for ensureAuthenticated function
rg -n "ensureAuthenticated|login" playwright/BootTests/helpers/ --type=ts -B 2 -A 5Repository: osbuild/image-builder-frontend
Length of output: 1466
🏁 Script executed:
# Now let's verify the shouldRetry logic by examining the actual code flow
cat -n playwright/BootTests/helpers/helpers.ts | sed -n '340,436p'Repository: osbuild/image-builder-frontend
Length of output: 3883
Simplify the retry condition by removing the redundant shouldRetry check.
The conditional at line 429 is redundant. The shouldRetry variable can only be true when execution reaches that point (set in every failure path: lines 371, 379, 391, 405, 416, and 425), and reaching line 429 means the function did not return true at line 410. Simplify to just check attempts < maxAttempts.
🔧 Proposed fix
- // Check if we should retry with delay
- if (shouldRetry && attempts < maxAttempts) {
+ // Wait before next attempt
+ if (attempts < maxAttempts) {
await sleep(delayMs);
}You can also remove the shouldRetry variable entirely (line 353 and all assignments) since it's no longer needed.
TOKEN is properly set during authentication.
The TOKEN environment variable is established via storeStorageStateAndToken(page) called after ensureAuthenticated() in the test flow. This ensures the Authorization header is populated for API requests.
Be aware of the long maximum wait time.
With default parameters (10s delay × 30 attempts), this function can wait up to 5 minutes. This is reasonable for system registration scenarios but could slow down test execution if the system never appears.
📝 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.
| export const pollForSystemTemplateAttachment = async ( | |
| page: Page, | |
| hostname: string, | |
| expectedTemplateName?: string, | |
| delayMs: number = 10_000, | |
| maxAttempts: number = 30, | |
| ): Promise<boolean> => { | |
| /* eslint-disable no-console */ | |
| let attempts = 0; | |
| while (attempts < maxAttempts) { | |
| attempts++; | |
| let shouldRetry = false; | |
| try { | |
| // Query the systems API with search filter for the hostname | |
| // Include Authorization header if TOKEN is available (set during login) | |
| const headers: Record<string, string> = {}; | |
| if (process.env.TOKEN) { | |
| headers['Authorization'] = process.env.TOKEN; | |
| } | |
| const response = await page.request.get( | |
| `/api/patch/v3/systems?search=${encodeURIComponent(hostname)}&limit=100`, | |
| { headers }, | |
| ); | |
| if (response.status() !== 200) { | |
| console.log( | |
| `API request failed with status ${response.status()}, attempt ${attempts}/${maxAttempts}`, | |
| ); | |
| shouldRetry = true; | |
| } else { | |
| const body = await response.json(); | |
| if (!body.data || !Array.isArray(body.data)) { | |
| console.log( | |
| `Invalid response format, attempt ${attempts}/${maxAttempts}`, | |
| ); | |
| shouldRetry = true; | |
| } else { | |
| // Find the system with matching hostname | |
| const system = body.data.find( | |
| (sys: { attributes: { display_name: string } }) => | |
| sys.attributes.display_name === hostname, | |
| ); | |
| if (!system) { | |
| console.log( | |
| `System '${hostname}' not found in inventory, attempt ${attempts}/${maxAttempts}`, | |
| ); | |
| shouldRetry = true; | |
| } else { | |
| // Check if system has a template_uuid assigned | |
| const hasTemplate = !!system.attributes?.template_uuid; | |
| if (hasTemplate) { | |
| const templateName = system.attributes.template_name; | |
| if ( | |
| expectedTemplateName && | |
| templateName !== expectedTemplateName | |
| ) { | |
| console.log( | |
| `System '${hostname}' is attached to template '${templateName}' but expected '${expectedTemplateName}', attempt ${attempts}/${maxAttempts}`, | |
| ); | |
| shouldRetry = true; | |
| } else { | |
| console.log( | |
| `System '${hostname}' is attached to template: ${templateName}`, | |
| ); | |
| return true; | |
| } | |
| } else { | |
| console.log( | |
| `System '${hostname}' is not attached to any template, attempt ${attempts}/${maxAttempts}`, | |
| ); | |
| shouldRetry = true; | |
| } | |
| } | |
| } | |
| } | |
| } catch (error) { | |
| console.log( | |
| `Error checking system template attachment: ${error}, attempt ${attempts}/${maxAttempts}`, | |
| ); | |
| shouldRetry = true; | |
| } | |
| // Check if we should retry with delay | |
| if (shouldRetry && attempts < maxAttempts) { | |
| await sleep(delayMs); | |
| } | |
| } | |
| /* eslint-enable no-console */ | |
| return false; | |
| }; | |
| export const pollForSystemTemplateAttachment = async ( | |
| page: Page, | |
| hostname: string, | |
| expectedTemplateName?: string, | |
| delayMs: number = 10_000, | |
| maxAttempts: number = 30, | |
| ): Promise<boolean> => { | |
| /* eslint-disable no-console */ | |
| let attempts = 0; | |
| while (attempts < maxAttempts) { | |
| attempts++; | |
| let shouldRetry = false; | |
| try { | |
| // Query the systems API with search filter for the hostname | |
| // Include Authorization header if TOKEN is available (set during login) | |
| const headers: Record<string, string> = {}; | |
| if (process.env.TOKEN) { | |
| headers['Authorization'] = process.env.TOKEN; | |
| } | |
| const response = await page.request.get( | |
| `/api/patch/v3/systems?search=${encodeURIComponent(hostname)}&limit=100`, | |
| { headers }, | |
| ); | |
| if (response.status() !== 200) { | |
| console.log( | |
| `API request failed with status ${response.status()}, attempt ${attempts}/${maxAttempts}`, | |
| ); | |
| shouldRetry = true; | |
| } else { | |
| const body = await response.json(); | |
| if (!body.data || !Array.isArray(body.data)) { | |
| console.log( | |
| `Invalid response format, attempt ${attempts}/${maxAttempts}`, | |
| ); | |
| shouldRetry = true; | |
| } else { | |
| // Find the system with matching hostname | |
| const system = body.data.find( | |
| (sys: { attributes: { display_name: string } }) => | |
| sys.attributes.display_name === hostname, | |
| ); | |
| if (!system) { | |
| console.log( | |
| `System '${hostname}' not found in inventory, attempt ${attempts}/${maxAttempts}`, | |
| ); | |
| shouldRetry = true; | |
| } else { | |
| // Check if system has a template_uuid assigned | |
| const hasTemplate = !!system.attributes?.template_uuid; | |
| if (hasTemplate) { | |
| const templateName = system.attributes.template_name; | |
| if ( | |
| expectedTemplateName && | |
| templateName !== expectedTemplateName | |
| ) { | |
| console.log( | |
| `System '${hostname}' is attached to template '${templateName}' but expected '${expectedTemplateName}', attempt ${attempts}/${maxAttempts}`, | |
| ); | |
| shouldRetry = true; | |
| } else { | |
| console.log( | |
| `System '${hostname}' is attached to template: ${templateName}`, | |
| ); | |
| return true; | |
| } | |
| } else { | |
| console.log( | |
| `System '${hostname}' is not attached to any template, attempt ${attempts}/${maxAttempts}`, | |
| ); | |
| shouldRetry = true; | |
| } | |
| } | |
| } | |
| } | |
| } catch (error) { | |
| console.log( | |
| `Error checking system template attachment: ${error}, attempt ${attempts}/${maxAttempts}`, | |
| ); | |
| shouldRetry = true; | |
| } | |
| // Wait before next attempt | |
| if (attempts < maxAttempts) { | |
| await sleep(delayMs); | |
| } | |
| } | |
| /* eslint-enable no-console */ | |
| return false; | |
| }; |
🧰 Tools
🪛 GitHub Check: ESLint
[warning] 429-429:
Unnecessary conditional, value is always truthy
🤖 Prompt for AI Agents
In @playwright/BootTests/helpers/helpers.ts around lines 341 - 436, The retry
check in pollForSystemTemplateAttachment is redundant: remove the shouldRetry
flag (declaration and all assignments) and replace the final conditional that
currently checks "if (shouldRetry && attempts < maxAttempts)" with a simpler "if
(attempts < maxAttempts)" so the loop sleeps when there are remaining attempts;
keep all existing return true paths and the catch behavior, and retain the
sleep(delayMs) call and attempts increment logic unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
playwright/BootTests/helpers/helpers.ts (2)
341-431: Consider type safety improvements for API responses.The polling logic is solid and handles errors gracefully. However, the API response parsing could benefit from explicit type definitions to prevent potential runtime errors if the API contract changes.
Optional: Add type definitions for API responses
Consider defining types for the API response structure:
type SystemAttributes = { display_name: string; template_uuid?: string; template_name?: string; }; type SystemData = { attributes: SystemAttributes; }; type SystemsApiResponse = { data?: SystemData[]; };This would make the code more maintainable and catch breaking changes earlier.
Note on static analysis warning (line 424): The condition
shouldRetry && attempts < maxAttemptsis flagged as having an always-truthy second part. While technically redundant given the while-loop condition, it serves as defensive programming and doesn't harm functionality.
441-508: Verify the delay difference between polling functions.This function uses a 20-second delay (vs 10 seconds in
pollForSystemTemplateAttachment), resulting in up to 10 minutes of total polling time. Please confirm this longer delay is intentional for inventory registration.Similar to the template attachment poller, consider adding explicit type definitions for the Inventory API response structure to improve type safety and maintainability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
playwright/BootTests/Content/ContentTemplate.boot.tsplaywright/BootTests/helpers/helpers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- playwright/BootTests/Content/ContentTemplate.boot.ts
🧰 Additional context used
🧬 Code graph analysis (1)
playwright/BootTests/helpers/helpers.ts (1)
playwright/helpers/helpers.ts (1)
closePopupsIfExist(31-55)
🪛 GitHub Check: ESLint
playwright/BootTests/helpers/helpers.ts
[warning] 424-424:
Unnecessary conditional, value is always truthy
⏰ 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). (22)
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
- GitHub Check: playwright-tests
- GitHub Check: Service Tests
- GitHub Check: boot-tests
🔇 Additional comments (3)
playwright/BootTests/helpers/helpers.ts (3)
273-285: LGTM!The navigation helper is clean and follows the established pattern from
navigateToRepositories. The simpler implementation without zero-state handling appears intentional.
287-324: LGTM!The template deletion helper follows the established patterns from
deleteCompliancePolicyanddeleteRepository. The graceful error handling and proper use of test steps make this a solid addition.
326-330: LGTM!Standard sleep utility for the polling functions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
playwright/BootTests/helpers/helpers.ts (1)
346-447: Polling logic works correctly; minor cleanup opportunity.The retry logic and API polling are well-implemented with appropriate logging and error handling. The function correctly handles various failure scenarios and validates template attachment.
However, as flagged by static analysis, the condition at line 440 is redundant—if execution reaches that point,
shouldRetryis alwaystrue(otherwise the function would have returned early at line 421).♻️ Optional simplification
- // Check if we should retry with delay - if (shouldRetry && attempts < maxAttempts) { + // Wait before next attempt + if (attempts < maxAttempts) { await sleep(delayMs); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
playwright/BootTests/helpers/helpers.ts
🧰 Additional context used
🪛 GitHub Check: ESLint
playwright/BootTests/helpers/helpers.ts
[warning] 440-440:
Unnecessary conditional, value is always truthy
⏰ 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). (15)
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
- GitHub Check: Service Tests
- GitHub Check: playwright-tests
- GitHub Check: boot-tests
🔇 Additional comments (4)
playwright/BootTests/helpers/helpers.ts (4)
273-285: LGTM!The
navigateToTemplatesfunction follows the established pattern of other navigation helpers in this file and appropriately waits for the page content to be visible.
287-324: LGTM!The
deleteTemplatefunction follows the same defensive pattern asdeleteRepositoryanddeleteCompliancePolicy, with appropriate graceful failure handling for cleanup scenarios.
326-344: LGTM!The internal
sleepandgetAuthHeadershelpers are well-implemented. The JWT extraction from cookies is appropriate for authenticated API calls in test scenarios.
449-526: Verify the different default polling interval.The implementation is solid with appropriate retry logic and error handling. However, note that this function uses a default
delayMsof 20 seconds, whilepollForSystemTemplateAttachmentuses 10 seconds.Please confirm whether this difference is intentional based on the Inventory API's response characteristics, or if these should be aligned for consistency.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @playwright/BootTests/Content/ContentTemplate.boot.ts:
- Around line 308-346: This commented-out block containing an alternative flow
using pollForSystemInInventory and pollForSystemTemplateAttachment (which
references hostname and templateName) should be either deleted or documented:
remove the entire commented section if it's dead code, otherwise replace it with
a short explanatory comment above the block stating why it is preserved (e.g.,
"kept for temporary debugging during template boot validation; remove after PR X
or when feature Y stabilizes") and include an expected removal timeline; ensure
no stray console.log or test.step remnants remain and keep the active
pollForSystemsAttachedToTemplate implementation as the canonical logic.
In @playwright/BootTests/helpers/helpers.ts:
- Around line 453-456: The conditional "if (shouldRetry && attempts <
maxAttempts) { await sleep(delayMs); }" is redundant because shouldRetry is
always true when reached; remove the unnecessary shouldRetry check (or eliminate
the shouldRetry variable entirely) and change the check to only "if (attempts <
maxAttempts) { await sleep(delayMs); }" so the logic relies solely on attempts
and maxAttempts; update or remove any now-unused shouldRetry
declaration/assignments in the surrounding function (e.g., where shouldRetry is
set) to avoid dead code.
🧹 Nitpick comments (5)
playwright/BootTests/Content/ContentTemplate.boot.ts (3)
23-24: Remove or uncomment the pollForSystemTemplateAttachment import.The
pollForSystemTemplateAttachmentfunction is imported but commented out. However, this function is not used in the active test code (only in the commented section below), whilepollForSystemInInventoryis used in the commented block. Consider removing both commented imports since they're not used in the active code path.🧹 Cleanup unused imports
- // pollForSystemInInventory, - // pollForSystemTemplateAttachment,
186-193: FIXME: Document the root cause of the wizard step workaround.The FIXME comment indicates that loading the Repositories step shouldn't be necessary for package search to work. This suggests a potential bug in the wizard's state management or data loading. Consider opening a separate issue to track this UI quirk so it can be fixed properly.
Do you want me to create a follow-up issue to track this wizard behavior for proper investigation?
244-276: Consider extracting polling configuration to constants.The polling logic uses magic numbers (30 attempts, 10,000ms delay) directly in the code. Extracting these to named constants would improve readability and make it easier to tune timeouts across similar polling operations.
♻️ Extract polling configuration
+const REGISTRATION_POLL_DELAY_MS = 10_000; +const REGISTRATION_MAX_ATTEMPTS = 30; + await test.step('Wait for system registration to complete', async () => { // Registration happens asynchronously after boot via cloud-init/rhc // Poll subscription-manager status until registration completes - const maxAttempts = 30; - const delayMs = 10_000; // 10 seconds between attempts + const maxAttempts = REGISTRATION_MAX_ATTEMPTS; + const delayMs = REGISTRATION_POLL_DELAY_MS;playwright/BootTests/helpers/helpers.ts (2)
330-330: Minor: Consider using existing async utilities.The
sleephelper is a simple Promise wrapper. While functional, Playwright providespage.waitForTimeout()as a built-in alternative. However, since this is used for API polling (not page interactions), the current approach is acceptable.
355-461: Consider refactoring polling functions to reduce duplication.The three polling functions (
pollForSystemTemplateAttachment,pollForSystemInInventory,pollForSystemsAttachedToTemplate) share very similar structure: retry loop, delay, error handling, and logging. Consider extracting a generic polling helper to reduce code duplication and improve maintainability.♻️ Example generic polling helper
/** * Generic polling helper with configurable check function */ async function pollUntil<T>( checkFn: () => Promise<{ success: boolean; result?: T; shouldRetry: boolean }>, options: { delayMs: number; maxAttempts: number; operationName: string; } ): Promise<T | null> { let attempts = 0; while (attempts < options.maxAttempts) { attempts++; try { const { success, result, shouldRetry } = await checkFn(); if (success && result !== undefined) { return result; } if (!shouldRetry) { break; } } catch (error) { console.log( `${options.operationName} failed: ${error}, attempt ${attempts}/${options.maxAttempts}` ); } if (attempts < options.maxAttempts) { await sleep(options.delayMs); } } return null; }Then each specific polling function would call this helper with its specific check logic.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
playwright/BootTests/Content/ContentTemplate.boot.tsplaywright/BootTests/helpers/helpers.ts
🧰 Additional context used
🪛 GitHub Check: ESLint
playwright/BootTests/helpers/helpers.ts
[warning] 454-454:
Unnecessary conditional, value is always truthy
⏰ 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). (5)
- GitHub Check: playwright-tests
- GitHub Check: Service Tests
- GitHub Check: boot-tests
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
🔇 Additional comments (5)
playwright/BootTests/helpers/helpers.ts (4)
273-285: LGTM!The
navigateToTemplatesfunction correctly navigates to the templates page and waits for the expected content to be visible.
287-324: LGTM! Graceful failure handling for missing templates.The
deleteTemplatefunction properly handles the case where a template doesn't exist by catching the exception and failing gracefully. This is consistent with similar deletion helpers in the file.
565-677: LGTM! Comprehensive polling with proper error handling.The
pollForSystemsAttachedToTemplatefunction correctly handles the multi-step API workflow (template lookup → systems query) with appropriate error handling and detailed logging for debugging.
337-344: JWT cookie extraction is consistent with the authentication flow.The
cs_jwtcookie extraction pattern matches the approach used inplaywright/helpers/login.tsand is correctly formatted as a Bearer token. The function is used consistently across API calls (systems, Inventory, and other endpoints) with the same pattern. The JWT scope and permissions are managed by the backend APIs being called.playwright/BootTests/Content/ContentTemplate.boot.ts (1)
48-49: The hardcoded repository URL (https://jlsherrill.fedorapeople.org/fake-repos/needed-errata/) is accessible and appears to be an intentional test fixture used consistently across multiple boot test files (ContentTemplate.boot.ts, ContentRepeatable.boot.ts, ContentNonRepeatable.boot.ts). The URL is hosted on stable Fedora Project infrastructure and returns HTTP 200. The tests properly clean up the repository state after execution, indicating the developers are aware of this external dependency.
| // Check if we should retry with delay | ||
| if (shouldRetry && attempts < maxAttempts) { | ||
| await sleep(delayMs); | ||
| } |
There was a problem hiding this comment.
Fix the unnecessary conditional flagged by ESLint.
The ESLint warning is correct: the condition shouldRetry && attempts < maxAttempts on line 454 will always have shouldRetry as truthy because it's only set to true in all the code paths that reach this point. The check is redundant since shouldRetry is never set to false before this line.
🔧 Simplify the conditional
// Check if we should retry with delay
- if (shouldRetry && attempts < maxAttempts) {
+ if (attempts < maxAttempts) {
await sleep(delayMs);
}Or alternatively, remove the shouldRetry variable entirely if it serves no purpose:
- let attempts = 0;
-
while (attempts < maxAttempts) {
attempts++;
- let shouldRetry = false;
try {
// Query the systems API with search filter for the hostname
// Extract auth token from cookies for API authentication
const headers = await getAuthHeaders(page);
// ... rest of logic
} catch (error) {
console.log(
`Error checking system template attachment: ${error}, attempt ${attempts}/${maxAttempts}`,
);
- shouldRetry = true;
}
// Check if we should retry with delay
- if (shouldRetry && attempts < maxAttempts) {
+ if (attempts < maxAttempts) {
await sleep(delayMs);
}
}📝 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.
| // Check if we should retry with delay | |
| if (shouldRetry && attempts < maxAttempts) { | |
| await sleep(delayMs); | |
| } | |
| // Check if we should retry with delay | |
| if (attempts < maxAttempts) { | |
| await sleep(delayMs); | |
| } |
🧰 Tools
🪛 GitHub Check: ESLint
[warning] 454-454:
Unnecessary conditional, value is always truthy
🤖 Prompt for AI Agents
In @playwright/BootTests/helpers/helpers.ts around lines 453 - 456, The
conditional "if (shouldRetry && attempts < maxAttempts) { await sleep(delayMs);
}" is redundant because shouldRetry is always true when reached; remove the
unnecessary shouldRetry check (or eliminate the shouldRetry variable entirely)
and change the check to only "if (attempts < maxAttempts) { await
sleep(delayMs); }" so the logic relies solely on attempts and maxAttempts;
update or remove any now-unused shouldRetry declaration/assignments in the
surrounding function (e.g., where shouldRetry is set) to avoid dead code.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @playwright/BootTests/Content/ContentTemplate.boot.ts:
- Around line 206-210: The test passes the wrong identifier to
pollForSystemTemplateAttachment: replace the second argument blueprintName with
hostname when calling pollForSystemTemplateAttachment(page, ..., templateName,
...), since the helper signature expects hostname and queries
?search=${encodeURIComponent(hostname)}; also update the expect() failure
message to reference the hostname variable (e.g., `System '${hostname}' should
be attached to template '${templateName}'`) so the assertion message matches the
queried value.
🧹 Nitpick comments (2)
playwright/BootTests/helpers/helpers.ts (1)
454-454: Remove unnecessary conditional check.The
shouldRetryvariable is alwaystruewhen execution reaches this line (since the function returns early at line 435 whenshouldRetryisfalse). The meaningful check here is onlyattempts < maxAttempts.♻️ Simplify the conditional
- if (shouldRetry && attempts < maxAttempts) { + if (attempts < maxAttempts) { await sleep(delayMs); }playwright/BootTests/Content/ContentTemplate.boot.ts (1)
184-192: Document the wizard quirk workaround.The FIXME comment notes that opening the Repositories step is necessary for package search to work. While this is acknowledged in the PR objectives, consider whether this workaround could be addressed in a follow-up or if it represents an underlying wizard bug that should be reported.
Would you like me to help create an issue to track the wizard behavior where the package search requires the repositories step to be loaded first?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
playwright/BootTests/Content/ContentTemplate.boot.tsplaywright/BootTests/helpers/helpers.ts
🧰 Additional context used
🧬 Code graph analysis (1)
playwright/BootTests/helpers/helpers.ts (1)
playwright/helpers/helpers.ts (1)
closePopupsIfExist(31-55)
🪛 GitHub Check: ESLint
playwright/BootTests/helpers/helpers.ts
[warning] 454-454:
Unnecessary conditional, value is always truthy
⏰ 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). (25)
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
- GitHub Check: Sourcery review
- GitHub Check: Service Tests
- GitHub Check: playwright-tests
- GitHub Check: boot-tests
🔇 Additional comments (10)
playwright/BootTests/helpers/helpers.ts (5)
273-285: LGTM! Clean navigation helper.The function follows the established pattern from
navigateToRepositoriesand provides clear navigation to the templates page with appropriate visibility checks.
287-324: LGTM! Consistent deletion pattern.The
deleteTemplatefunction follows the same graceful failure pattern asdeleteRepositoryanddeleteCompliancePolicy, making the codebase more consistent.
326-344: LGTM! Appropriate internal helpers.The
sleepandgetAuthHeadershelpers are appropriately scoped as internal (non-exported) utilities for the polling functions.
471-555: LGTM! Well-structured inventory polling.The
pollForSystemInInventoryfunction provides comprehensive polling with proper error handling, logging, and flexible matching logic (checking bothdisplay_nameandfqdn).
355-461: The polling implementation is appropriately designed for the actual use case.The function correctly matches systems using
blueprintName(which becomes the instance name in OpenStack), and thestartsWithfallback provides robustness against potential display_name variations. The default polling parameters of 10 seconds delay with 30 attempts (5 minutes total) are reasonable for typical system registration times in hosted environments, comparable to or more aggressive than similar polling functions in the codebase (e.g.,pollForSystemInInventoryuses 20 seconds with 10 minutes total). No verification needed.playwright/BootTests/Content/ContentTemplate.boot.ts (5)
33-60: LGTM! Proper test setup with cleanup.The test setup properly generates unique identifiers and registers all cleanup functions to ensure resources are removed even if the test fails.
64-86: LGTM! Appropriate timeouts for repository operations.The repository creation and snapshot validation steps use reasonable timeouts (180 seconds) for operations that depend on external snapshot creation.
88-156: LGTM! Comprehensive template creation workflow.The multi-step template creation wizard is well-structured with appropriate waits and visibility checks at each step. The 180-second timeout for template validation aligns with the repository snapshot timeout.
243-275: Known registration failure on stage environment.According to the PR objectives, this registration check (and the subsequent verification steps) fails on stage because "subscription/registration of the image against stage does not complete despite the image being built with an activation key." While the polling logic appears correct, the test may need to be adjusted or the underlying registration issue resolved before this can reliably pass.
Consider whether this test should:
- Be marked as
.skip()on stage until the registration issue is resolved- Have adjusted timeouts/parameters for the stage environment
- Have the registration steps made optional until the infrastructure issue is fixed
278-287: LGTM! Proper session refresh before API calls.Re-authenticating before polling the Inventory API is a good practice, as the session may have expired during the long-running build and provisioning steps.
| await test.step('Set hostname for system identification', async () => { | ||
| await frame.getByRole('button', { name: 'Hostname' }).click(); | ||
| await frame.getByRole('textbox', { name: 'hostname input' }).fill(hostname); | ||
| await frame.getByRole('button', { name: 'Review and finish' }).click(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "hostname|blueprintName" playwright/BootTests/Content/ContentTemplate.boot.ts -C 2Repository: osbuild/image-builder-frontend
Length of output: 2349
🏁 Script executed:
rg -n "pollForSystemTemplateAttachment" playwright/BootTests/Content/ -A 10Repository: osbuild/image-builder-frontend
Length of output: 1941
🏁 Script executed:
rg -n "function pollForSystemTemplateAttachment|const pollForSystemTemplateAttachment|export.*pollForSystemTemplateAttachment" playwright/BootTests/helpers/ -A 15Repository: osbuild/image-builder-frontend
Length of output: 1193
🏁 Script executed:
rg -n "pollForSystemTemplateAttachment" playwright/BootTests/helpers/helpers.ts -A 25 | head -50Repository: osbuild/image-builder-frontend
Length of output: 1028
Fix incorrect parameter passed to pollForSystemTemplateAttachment.
Line 295 passes blueprintName but the function expects hostname. The function signature (helpers.ts:357) declares the second parameter as hostname: string and queries the API with ?search=${encodeURIComponent(hostname)}. Since blueprintName and hostname are distinct variables with different values (lines 42 and 46), passing blueprintName will cause the template attachment check to query the wrong system identifier.
Change line 295 to pass hostname instead of blueprintName, and update the error message at line 302 accordingly.
Affected code
const isAttached = await pollForSystemTemplateAttachment(
page,
blueprintName, // Should be: hostname
templateName,
10_000,
30,
);
expect(
isAttached,
`System '${blueprintName}' should be attached to template '${templateName}'`, // Should reference: hostname
).toBe(true);
🤖 Prompt for AI Agents
In @playwright/BootTests/Content/ContentTemplate.boot.ts around lines 206 - 210,
The test passes the wrong identifier to pollForSystemTemplateAttachment: replace
the second argument blueprintName with hostname when calling
pollForSystemTemplateAttachment(page, ..., templateName, ...), since the helper
signature expects hostname and queries ?search=${encodeURIComponent(hostname)};
also update the expect() failure message to reference the hostname variable
(e.g., `System '${hostname}' should be attached to template '${templateName}'`)
so the assertion message matches the queried value.
1 participant
TL;DR:
What's currently failing is the subscription/registration of the image against stage, despite it being built with an activation key. I could reproduce this manually with stage, not with production.
Otherwise, I verified all the steps in this test manually against prod, and it all worked as expected.
This test
FIXME: