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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
More tidying prior to Models in Browse Data #38126
Conversation
323e55d
to
ec104b1
Compare
cy.findByText("Our analytics").should("not.exist"); | ||
cy.findByRole("heading", { name: /Our data/ }).should("be.visible"); | ||
cy.findByRole("treeitem", { name: /Our data/ }).should("not.exist"); | ||
cy.findByRole("treeitem", { name: "Our analytics" }).should("not.exist"); | ||
appBar().should("not.exist"); |
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 think this test is actually ensuring that the side nav, not the top nav, is not rendered
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.
IIRC, @WiNloSt originally wrote these tests.
I think the test is doing what it says on the tin - if we hide the side navigation and hide the logo, we want to make sure the top bar is not rendered.
There is a test below that explicitly makes sure nav bar is hidden via param.
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.
Thank you for helping me understand. I've reverted that change
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.
@nemanjaglumac got it exactly right. That test was added during the implementation of hiding Metabase logo.
For context if you're interested:
cy.button("Sign in").click(); | ||
cy.findByRole("alert") | ||
.filter(':contains("did not match stored password")') | ||
.should("be.visible"); |
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.
cy.findByRole("alert", {name: "did not match stored password"})
does not work; this longer formula is needed.
Codenotify: Notifying subscribers in CODENOTIFY files for diff 291ecc0...5d75fe7.
|
|
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.
looks great, except we don't want to keep adding more chainers off of cy.
e2e/support/commands/ui/button.ts
Outdated
Cypress.Commands.add( | ||
"heading", | ||
{ | ||
prevSubject: "optional", | ||
}, | ||
(subject, headingName, timeout) => { | ||
const config = { | ||
name: headingName, | ||
timeout, | ||
}; | ||
|
||
return subject | ||
? cy.wrap(subject).findByRole("heading", config) | ||
: cy.findByRole("heading", config); | ||
}, | ||
); | ||
|
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.
@nemanjaglumac might have better judgment here than I, but we're trying not to make new cy.something()
functions in favor of simple helpers like getHeading(name)
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.
Yup, what Ryan said.
If anything, we want to reduce the amount of custom commands.
findByRole
is already a custom command - it comes from Cypress Testing Library.
Can we please use it directly as is?
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.
Thanks both. I've removed that extra helper function.
cy.findByText("Our analytics").should("not.exist"); | ||
cy.findByRole("heading", { name: /Our data/ }).should("be.visible"); | ||
cy.findByRole("treeitem", { name: /Our data/ }).should("not.exist"); | ||
cy.findByRole("treeitem", { name: "Our analytics" }).should("not.exist"); | ||
appBar().should("not.exist"); |
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.
IIRC, @WiNloSt originally wrote these tests.
I think the test is doing what it says on the tin - if we hide the side navigation and hide the logo, we want to make sure the top bar is not rendered.
There is a test below that explicitly makes sure nav bar is hidden via param.
f1c9289
to
d67e988
Compare
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 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.
Thanks for improving a11y and for addressing the reviews.
@rafpaf Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
* Make embedding-full-app.cy.spec.js and associated components more semantic * Make signin.cy.spec.js and associated components more semantic * Fix comment * minor tweak * Remove aria-label=breadcrumbs * Fix variable name * Fix e2e tests * Remove cy.heading * Restore test name
* Make embedding-full-app.cy.spec.js and associated components more semantic * Make signin.cy.spec.js and associated components more semantic * Fix comment * minor tweak * Remove aria-label=breadcrumbs * Fix variable name * Fix e2e tests * Remove cy.heading * Restore test name
The PR makes some e2e tests and associated components more semantic