test: add an e2e audit logs test #12868
Conversation
aslilac
requested review from
BrunoQuaresma,
Emyrk and
Parkreiner
and removed request for
BrunoQuaresma,
sreya and
Kira-Pilot
There was a problem hiding this comment.
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
| const createdWorkspace = page.locator(".MuiTableRow-root", { | ||
| hasText: `${userName} created workspace ${workspaceName}`, | ||
| }); |
There was a problem hiding this comment.
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 a row role as well
There was a problem hiding this comment.
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.
Not fully sure what element this is, but could the selector be updated to have a link/button role, since whatever it is, it looks like something that can be interacted with?
There was a problem hiding this comment.
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.
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.
It's an aria-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.
Could this be turned into
await page.getByRole("menu", { name: "Workspace Build" }).click();?
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
Same comment as above, since this is some kind of interactive element
There was a problem hiding this comment.
we could add testids 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