-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Cypress tests for query parameters #3810
Conversation
afterEach(() => { | ||
cy.getByTestId('SaveButton').click(); | ||
cy.url().should('match', /\/queries\/\d+\/source/); | ||
}); |
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.
You sure this is needed? Can leave new query unsaved.
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 either save the query or this workaround 😬
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.
Oh right right!
Suggesting to add tests in Dashboard context. |
Mostly I want to test the -- Just to share some points I was thinking about:
|
I mean specifically test the "apply" functionality in the dashboard context, that it triggers widget refresh.
For what purpose? To db-seed these queries? |
Great initiative! |
It seems that Cypress has an issue when it's necessary to click more than once on Antd's Date Calendar, the first click works, the following ones don't ^^. This affected creating specs for DateTime and DateRanges. I think my last test will be to trigger DateTime by clicking enter or clicking outside the panel.
I thought about adding a smaller test just to make sure the behavior extends to the dashboard context, but this is sort of already happening with specs that rely on that behavior (e.g: "Height behavior on refresh"). Do you think we gain any value by having that separate?
🤔. I though more about development and unautomated tests. After running Cypress you'd get Redash with a basic set of organized data (in this case, queries that you could test each parameter). I'll think better about it, perhaps sth not for now as I don't really use Cypress data after it runs (and guessing nobody actually does it here). |
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.
Updated this to use requests instead of UI (reduced 30 seconds). Date parameters are still created through UI since Cypress showed similar clicking issues with it when the parameter was set in the request.
@@ -19,11 +19,11 @@ export function createQuery(data, shouldPublish = true) { | |||
}, data); | |||
|
|||
// eslint-disable-next-line cypress/no-assigning-return-values | |||
let request = cy.request('POST', '/api/queries', merged); | |||
let request = cy.request('POST', '/api/queries', merged).then(({ body }) => body); |
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.
To be consistent with the shouldPublish = true
changed it to return the body directly.
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.
Let the good times roll
What type of PR is this? (check all applicable)
Description
Following #3737 (and the bug #3803) I wanted to add a few tests to another important part of Redash: Query parameters 😁
Parameters:
Related Tickets & Documents
--
Mobile & Desktop Screenshots/Recordings (if there are UI changes)