feat: add playwright tests that do not require cucumber feature files#658
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @carlosthe19916, your pull request is too large to review
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #658 +/- ##
==========================================
+ Coverage 57.28% 63.21% +5.93%
==========================================
Files 146 150 +4
Lines 2430 2474 +44
Branches 552 561 +9
==========================================
+ Hits 1392 1564 +172
+ Misses 833 687 -146
- Partials 205 223 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
carlosthe19916
changed the title
carlosthe19916
added
the
backport release/0.3.z
Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
# Conflicts: # .devcontainer/template.json
carlosthe19916
force-pushed
the
task/move-trustify-testsw
branch
from
afd3bbd to
07f30d6
Compare
Reviewer's GuideThis PR migrates and consolidates the existing Trustify-tests into a full Playwright-based E2E suite by introducing page object model classes, test pages and tabs, helper utilities, and comprehensive spec files for UI interactions, along with updating the devcontainer configuration to point at the correct Dockerfile. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider DRY-ing up the nearly identical Tab classes by extracting a generic base or factory to reduce boilerplate and maintenance overhead.
- Many page objects expose internal Locator fields (e.g. _toolbar, _table) publicly – make those private or provide explicit methods to better encapsulate implementation details.
- Navigation.goToSidebar always starts by calling
/upload; this indirection can slow or break tests—consider parameterizing the entry path or navigating directly to the target page.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider DRY-ing up the nearly identical Tab classes by extracting a generic base or factory to reduce boilerplate and maintenance overhead.
- Many page objects expose internal Locator fields (e.g. _toolbar, _table) publicly – make those private or provide explicit methods to better encapsulate implementation details.
- Navigation.goToSidebar always starts by calling `/upload`; this indirection can slow or break tests—consider parameterizing the entry path or navigating directly to the target page.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
There was a problem hiding this comment.
@carlosthe19916 Overall, I'd say the code is up to a high standard and can be approved. However, I can see that it has a rather large overlap with the currently existing UI tests, duplicating many of the methods and scenarios. Seeing as we already have less time to write these tests than we would like, I'd really like to avoid having to extensively rewrite the little that's already in there. It would be nice to get an opinion from @queria on how to approach this.
One way to avoid the overlap and have a situation where we have a test run where certain tests run basically twice or more times (differently named tests that test the same thing), would be to add tags to these tests. If I understand it correctly, the purpose of this PR is to test UI elements, whereas the QE tests are supposed to test user scenarios, which currently leads to very similar tests. It's possible that the QE tests will eventually diverge from these UI element tests, so it would make sense to run these two sets separately. Again, would like the opinion of @queria .
Aside from the above, I've left a few comments on minor improvements and notes on what to watch out for in the future.
| await tab.click(); | ||
| } | ||
|
|
||
| async clickOnPageAction(actionName: string) { |
There was a problem hiding this comment.
We should make sure to unify the usage of methods like this across the repo, since they also implemented somewhere in the UI tests directory exactly the same way and used in the tests. The better we do this, the less of a pain it will be. I'm just adding this comment, so we don't forget to create issues in the repo for this sort of thing.
| @@ -0,0 +1,23 @@ | |||
| import { expect } from "@playwright/test"; | |||
|
|
|||
| export const sortArray = (arr: string[], asc: boolean) => { | |||
There was a problem hiding this comment.
Same as above. I'm pretty sure that Rajan implemented something similar in one of his tests, so we should make sure we don't duplicate code unnecessarily.
| // Navigate to next page | ||
| await nextPageButton.click(); | ||
|
|
||
| // Verify that previous buttons are enabled after moving to next page |
There was a problem hiding this comment.
If we are this thorough, should we also check if the next button is disabled on the last page?
| await this._page.getByRole("menuitem", { name: actionName }).click(); | ||
| } | ||
|
|
||
| async verifyTableIsSortedBy(columnName: string, asc: boolean = true) { |
There was a problem hiding this comment.
Again, this and the next method are for sure candidates for unification with what's already in some of the UI tests.
e2e/tests/ui/pages/advisory-details/vulnerabilities/VulnerabilitiesTab.ts
Outdated
Show resolved
Hide resolved
e2e/tests/ui/pages/advisory-details/vulnerabilities/filter.spec.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
|
@sourcery-ai review |
|
Hi @queria! 👋 Only authors and team members can run @sourcery-ai commands on public repos. If you are a team member, install the @sourcery-ai bot to get access ✨ |
|
@sourcery-ai review |
@sourcery-ai |
Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
|
@carlosthe19916 Yeah, I agree. Could you just please tag all the tests with a tag, e.g. |
@vobratil For executing only the BDD tests we would do (all tests in this PR will be excluded): On the other hand, for executing only the tests in this PR without including any of the bdd tests we could do: As per this PR, both will run, the dbb and the tests inside this PR as defined in the package.json here https://github.com/carlosthe19916/trustify-ui/blob/task/move-trustify-testsw/e2e/package.json#L12C31-L12C71 And the separation between dbb and the tests in this PR are defined here https://github.com/carlosthe19916/trustify-ui/blob/task/move-trustify-testsw/e2e/playwright.config.ts#L73-L80 I can add tags, but I think we could use tags in a different way. i believe we already have some tags like |
carlosthe19916
added
needs-backport
|
Btw, same logic applies to the "api" tests, we are not setting labels to separate api tests, we are using Playwright projects. E.g. |
|
@carlosthe19916 Oh, I didn't know you could run the BDD tests like that. In that case there's nothing else to discuss. Approving :). |
vobratil
left a comment
vobratil
left a comment
There was a problem hiding this comment.
@carlosthe19916 After the discussion, I don't think this PR needs any more changes. Approved.
4 participants
Summary by Sourcery
Set up a standalone Playwright-based end-to-end testing suite without relying on Cucumber feature files by adding page object abstractions and comprehensive UI test specifications, and update the devcontainer configuration to enable code generation support.
New Features:
Enhancements:
Tests: