Add automation to importers UI #715
Conversation
Reviewer's GuideThis PR adds comprehensive BDD automation for the Importers UI in TPA by introducing a new Playwright-BDD step definitions file and a corresponding feature file. The step definitions implement navigation, table header checks, filter/search interactions, row expansion with state validations, pagination controls, and call-to-action menu operations (enable/disable/run). The feature file defines scenarios that drive these steps end-to-end. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Several loops have trailing semicolons (e.g. for(i=0; i<=5; i++);) which nullify their bodies—please remove those semicolons to ensure the loops execute correctly.
- There’s a lot of repeated navigation and locator code—consider extracting common steps and selectors into reusable functions or constants to reduce duplication and improve maintainability.
- Avoid using hard waitForTimeout calls; replace them with Playwright’s built-in waiting methods (e.g., waitForSelector or expect(locator).toBeVisible()) for more reliable synchronization.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several loops have trailing semicolons (e.g. for(i=0; i<=5; i++);) which nullify their bodies—please remove those semicolons to ensure the loops execute correctly.
- There’s a lot of repeated navigation and locator code—consider extracting common steps and selectors into reusable functions or constants to reduce duplication and improve maintainability.
- Avoid using hard waitForTimeout calls; replace them with Playwright’s built-in waiting methods (e.g., waitForSelector or expect(locator).toBeVisible()) for more reliable synchronization.
## Individual Comments
### Comment 1
<location> `e2e/tests/ui/features/@importer-explorer/importer-explorer.step.ts:143` </location>
<code_context>
+Then("The user expands all the importers rows",
</code_context>
<issue_to_address>
Expand button validation uses hardcoded indices and may not cover all edge cases.
Instead of relying on fixed indices, iterate over the actual table rows to expand each importer. This will make the test robust to changes in the number of importers.
Suggested implementation:
```typescript
Then("The user expands all the importers rows",
async ({ page }) => {
// Select all importer rows in the table
const importerRows = await page.$$('tr[data-ouia-component-type="importer-row"]');
for (const row of importerRows) {
// Find the expand button within the row
const expandButton = await row.$('button[aria-label="Expand row"]');
if (expandButton) {
await expandButton.click();
}
}
```
- If your table rows do not use `data-ouia-component-type="importer-row"`, adjust the selector to match your actual markup.
- If the expand button uses a different `aria-label` or selector, update `'button[aria-label="Expand row"]'` accordingly.
- Remove any unused variables or logic related to hardcoded indices and button names elsewhere in the file if present.
</issue_to_address>
### Comment 2
<location> `e2e/tests/ui/features/@importer-explorer/importer-explorer.step.ts:240` </location>
<code_context>
+ }
+);
+
+Then("It should not be possible to add page numbers in the pagination text box",
+ async ({ page }) => {
+ expect(page.locator(`xpath=//div[@id='importer-table-pagination-top']//input[@aria-label='Current page']`)).not.toBeEditable();
+ expect(page.locator(`xpath=//div[@id='importer-table-pagination-bottom']//input[@aria-label='Current page']`)).not.toBeEditable();
+ }
+);
+
</code_context>
<issue_to_address>
Missing await for locator assertions.
Add 'await' before the locator assertions to ensure proper test execution and accurate results.
</issue_to_address>
### Comment 3
<location> `e2e/tests/ui/features/@importer-explorer/importer-explorer.step.ts:265` </location>
<code_context>
+Then("The user clicks on all the Call to Action buttons and Enable, Disable and Run options will be displayed",
</code_context>
<issue_to_address>
Loop logic and missing awaits may cause unreliable test execution.
The loop increments 'i' only within conditional blocks, risking infinite loops or missed iterations. Additionally, some locator actions lack 'await'. Refactor the loop for correct iteration and ensure all async actions are properly awaited.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| "Application navigates to Importers page", | ||
| async ({ page }) => { | ||
| await page.locator(`xpath=//h1[@class='pf-v6-c-content--h1' and text()="Importers"]`).isVisible(); | ||
| }); |
There was a problem hiding this comment.
suggestion (testing): Expand button validation uses hardcoded indices and may not cover all edge cases.
Instead of relying on fixed indices, iterate over the actual table rows to expand each importer. This will make the test robust to changes in the number of importers.
Suggested implementation:
Then("The user expands all the importers rows",
async ({ page }) => {
// Select all importer rows in the table
const importerRows = await page.$$('tr[data-ouia-component-type="importer-row"]');
for (const row of importerRows) {
// Find the expand button within the row
const expandButton = await row.$('button[aria-label="Expand row"]');
if (expandButton) {
await expandButton.click();
}
}- If your table rows do not use
data-ouia-component-type="importer-row", adjust the selector to match your actual markup. - If the expand button uses a different
aria-labelor selector, update'button[aria-label="Expand row"]'accordingly. - Remove any unused variables or logic related to hardcoded indices and button names elsewhere in the file if present.
| "The user navigates to TPA URL page successfully", | ||
| async ({ page }) => { | ||
| await page.locator(`xpath=//div[@class='pf-v6-c-card__title-text' and text()="Dashboard"]`).isVisible(); | ||
| } |
There was a problem hiding this comment.
issue (testing): Missing await for locator assertions.
Add 'await' before the locator assertions to ensure proper test execution and accurate results.
| async ({ page }) => { | ||
| await page.locator(`xpath=//div[@class='pf-v6-c-card__title-text' and text()="Dashboard"]`).isVisible(); | ||
| } | ||
| ); |
There was a problem hiding this comment.
issue (bug_risk): Loop logic and missing awaits may cause unreliable test execution.
The loop increments 'i' only within conditional blocks, risking infinite loops or missed iterations. Additionally, some locator actions lack 'await'. Refactor the loop for correct iteration and ensure all async actions are properly awaited.
| } | ||
| )); | ||
| const impTableHeader = ['Name', 'Type', 'Description', 'Source', 'Period', 'State']; | ||
| var i: number; |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
| Then( | ||
| "Call to Action button for each importer should be visible", | ||
| async ({ page }) => { | ||
| var i: number; |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
|
|
||
| Then("The user clicks on all the Call to Action buttons and Enable, Disable and Run options will be displayed", | ||
| async ({ page }) => { | ||
| for(var i=0; i<=4;) |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
|
|
||
| Then("The user has clicked on the Call to Action button under an enabled importer and disables the importer", | ||
| async ({ page }) => { | ||
| for(var i=0; i<=1;) { |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
|
|
||
| Then("The user has clicked on the Call to Action icon under a disabled importer and the Disable option is not visible", | ||
| async ({ page }) => { | ||
| for(var i=0; i<=2;) { |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
|
|
||
| Then("The user has clicked on the Kebab icon under a disabled importer and enables it", | ||
| async ({ page }) => { | ||
| for(var i=0; i<=2;) { |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
|
|
||
| Then("The user has clicked on the Call to Action icon under an enabled importer and then selects Run", | ||
| async ({ page }) => { | ||
| for(var i=0; i<=1;) { |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
| Given User is authenticated | ||
|
|
||
| Scenario: The user navigates to Importers Explorer page | ||
| Given The user navigates to TPA URL page successfully |
There was a problem hiding this comment.
I think it is better to avoid these extra 'navigate to homepage' generic steps. And especially repeating them in every scenario too.
- You already have
Background-Given User is authenticated(similar to existing advisory/sbom/vulnerability-explorer tests)
-- That can be expected to result in You being already on TPA website (so have left menu available)
Also if similar step would be needed, then:
- If all
Scenariosunder theFeatureneed such step it would be better to add it toBackgroundinstead - The TPA URL page - You try to navigate to Dashboard, we should in current situation avoid navigation to Dashboard unless it is relevant for the test (e.g. of dashboard itself), as currently it is very expensive page (compared to most), so if then it is better for these universal-not-related-to-test cases better to navigate elsewhere like to
/uploador/importerswhich are way faster and do not slow test execution that much, as also do not cause that big load on the backend
| import { DetailsPage } from "../../helpers/DetailsPage"; | ||
| import { ToolbarTable } from "../../helpers/ToolbarTable"; | ||
| import { SearchPage } from "../../helpers/SearchPage"; | ||
| import { test } from "@playwright/test"; |
There was a problem hiding this comment.
We have tweaked/extended 'test' object, so please replace with:
| import { test } from "@playwright/test"; | |
| import { test } from "../../fixtures"; |
There was a problem hiding this comment.
@ikanias Unfortunately this PR has quite a lot of problems with things like defining redundant test steps, not taking advantage of already defined methods, relying heavily on very specific xpaths, unnecessary imports and others. I can suggest taking a look at how other user stories and their steps are defined, using the Playwright Code Gen tool to help create maintainable code and perhaps even take advantage of the Trustify MCP server to generate good test steps from the BDD file.
| Given User is authenticated | ||
|
|
||
| Scenario: The user navigates to Importers Explorer page | ||
| Given The user navigates to TPA URL page successfully |
|
|
||
| Scenario: The user navigates to Importers Explorer page | ||
| Given The user navigates to TPA URL page successfully | ||
| When The user selects the importers menu option |
There was a problem hiding this comment.
Since we can expect a similar step in other scenarios, too, it would make sense to make this one re-usable, e.g. User selects the <menuOption> option and put it in a shared file in the ui/features directory.
| Scenario: The user navigates to Importers Explorer page | ||
| Given The user navigates to TPA URL page successfully | ||
| When The user selects the importers menu option | ||
| Then Application navigates to Importers page |
There was a problem hiding this comment.
We already have a similar step here. We should make it general and re-usable and move it to a shared file in ui/features, so that we avoid redundancy.
| Given The user navigates to TPA URL page successfully | ||
| When The user selects the importers menu option | ||
| Then Application navigates to Importers page | ||
| Then Call to Action button for each importer should be visible |
There was a problem hiding this comment.
I don't believe this is up-to-date, anymore.
| Scenario: The user can see that the filter icon appears in importers page | ||
| Given The user navigates to TPA URL page | ||
| When The user selects the importers overview page | ||
| Then The column headers should be visible |
There was a problem hiding this comment.
The names of the actual headers should definitely be a part of the feature file, otherwise this step is pretty vague.
| )); | ||
| const impTableHeader = ['Name', 'Type', 'Description', 'Source', 'Period', 'State']; | ||
| var i: number; | ||
| for(i=0; i<=5; i++); |
There was a problem hiding this comment.
The code would be more readable if we used Playwright methods such as getByText("Source").
There was a problem hiding this comment.
Actually, we already have a method for this, even if deprecated verifyTableHeaderContains(columnHeader).
| { | ||
| if (actualHeaders[i] != impTableHeader[i]) | ||
| { | ||
| console.log('The column name ' + impTableHeader[i] + 'is displayed incorrectly') |
There was a problem hiding this comment.
This way the log won't appear in the logs. We need to use logger.info(message). However, if the assertion fails, Playwright informs us about it, so we don't need to actually log this.
| Given( | ||
| "The user navigates to TPA URL page", | ||
| async ({ page }) => { | ||
| await page.locator(`xpath=//div[@class='pf-v5-c-card__title-text' and text()="Your dashboard"]`).isVisible(); |
There was a problem hiding this comment.
Why re-define these steps, if we can re-use the ones defined previously? Also, the previous method uses v6 and this one v5, demonstrating that these xpaths are brittle and should be avoided if possible.
| ); | ||
|
|
||
| When( | ||
| "The user selects the importers overview page", |
There was a problem hiding this comment.
Same as above. This is probably true for many steps in this file.
| Then("The importers page should be displayed with pagination and correct paging values", | ||
| async ({ page }) => { | ||
| await expect(page.locator(`xpath=//div[@id='importer-table-pagination-top']//span[normalize-space(text())='of' and text()=1]`)).toBeVisible(); | ||
| await expect(page.locator(`xpath=//button[@id='pagination-id-top-toggle']//b[normalize-space(text())=1 and text()='5']`)).toBeVisible(); |
There was a problem hiding this comment.
We definitely shouldn't hardcode values like text()=5`` here.
There was a problem hiding this comment.
The current implementation heavily uses XPath, hardcoded identifiers such as "OUIA-Generated-Button-plain-11" and static conditioned for loops. Optimizing this step definition would improve test stability and maintainability. As @vobratil mentioned, playwright MCP can be used for these.
5 participants
This PR contains the .feature file and the .ts file of importer-explorer for the importers UI automation in TPA.
Summary by Sourcery
Add end-to-end UI automation for the Importer Explorer in TPA, including Playwright BDD step definitions and feature scenarios covering navigation, filtering, search, row expansion, pagination, and importer action workflows.
Tests: