- Notifications
You must be signed in to change notification settings - Fork928
test: add an e2e audit logs test#12868
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Parkreiner left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As long as@Emyrk thinks the other file changes look good, there's nothing worth blocking here. Had a few questions/recommendations about fine-tuning the selectors, though
Uh oh!
There was an error while loading.Please reload this page.
| const createdWorkspace = page.locator(".MuiTableRow-root", { | ||
| hasText: `${userName} created workspace ${workspaceName}`, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't fully understand this code, but I'm a little worried. If I'm understanding it right, the code is basically grabbing an element via the CSS class it gets from MUI, and then asserting it has the specific content?
I don't know how stable MUI class names are, but is there any way we can update this to be a role-based selector? Assuming MUI doesn't mess up their table semantics,this element should have arow role as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'll double check, but I didn't notice a role, and I generally prefer those.
but also MUI's classes names are a documented part of their public API, so it should be safe to depend on.
| const loginMessage = `${userName} logged in`; | ||
| // Filter by resource type | ||
| await page.getByText("All resource types").click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not fully sure what element this is, but could the selector be updated to have alink/button role, since whatever it is, it looks like something that can be interacted with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
there are multiple buttons on the page though. those roles would be ambiguous.
| const createdWorkspace = page.locator(".MuiTableRow-root", { | ||
| hasText: `${userName} created workspace ${workspaceName}`, | ||
| }); | ||
| await createdWorkspace.getByLabel("open-dropdown").click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this a label that we're visibly showing in the UI? The hyphen makes it look like an implementation detail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's anaria-label. Which probably still shouldn't have a hyphen, but it is what it is. 😅
| // Filter by resource type | ||
| await page.getByText("All resource types").click(); | ||
| await page.getByRole("menu").getByText("Workspace Build").click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could this be turned into
awaitpage.getByRole("menu",{name:"Workspace Build"}).click();
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
no, this is descending into the menu, and getting by text inside of it to click a specific option. that would just get the whole menu.
| await expect(page.getByText(loginMessage)).toBeVisible(); | ||
| // Filter by action type | ||
| await page.getByText("All actions").click(); |
ParkreinerApr 11, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same comment as above, since this is some kind of interactive element
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we could addtestids to them, but tbh I kinda hate those 😅 if you'd prefer that tho, we use them all over the place and I cope
Closes#12507