-
Notifications
You must be signed in to change notification settings - Fork 365
[WIP] - Added automated tests with cypress for Namespace form #9517
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
base: master
Are you sure you want to change the base?
[WIP] - Added automated tests with cypress for Namespace form #9517
Conversation
Checked commit asirvadAbrahamVarghese@1338c30 with ruby 3.1.7, rubocop 1.56.3, haml-lint 0.64.0, and yamllint |
cypress/e2e/ui/Automation/Embedded-Automate/Explorer/namespace.cy.js
Outdated
Show resolved
Hide resolved
cypress/e2e/ui/Automation/Embedded-Automate/Explorer/namespace.cy.js
Outdated
Show resolved
Hide resolved
cypress/e2e/ui/Automation/Embedded-Automate/Explorer/namespace.cy.js
Outdated
Show resolved
Hide resolved
cypress/e2e/ui/Automation/Embedded-Automate/Explorer/namespace.cy.js
Outdated
Show resolved
Hide resolved
cy.get('#main-content .bx--btn-set button[type="submit"]') | ||
.contains(addButton) | ||
.click(); | ||
cy.wait('@addNamespaceApi'); |
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.
Seeing the method above, interceptAccordionApi
, we have to be concerned with overengineering things where we try to generally handle all intercept, get, click, wait calls. I think this is fine as a one-off as you did here. As we see lots of repetition, like you saw previously, we can do methods that generalize the user's UI actions like you did in interceptAccordionApi
.
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 need to figure out a way to handle API intercepts and waits without duplicating stuff or using aliases in a way that might make the test flaky.
cy.get('#main-content .bx--btn-set button[type="button"]') | ||
.contains(cancelButton) | ||
.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.
Note, I'm fine with doing multiple validations in the same test function. We have to be aware these are more integration/component tests so they need not be small unit tests. See also: https://docs.cypress.io/app/core-concepts/best-practices#Creating-Tiny-Tests-With-A-Single-Assertion
If the setup is much different, it might be better to keep them seperate like they are here.
cypress/e2e/ui/Automation/Embedded-Automate/Explorer/namespace.cy.js
Outdated
Show resolved
Hide resolved
e9a9f6c
to
b682c59
Compare
Tried clean up using api calls - b682c59 |
@GilbertCherrie @jrafanie @Fryguy @elsamaryv |
b42e5f2
to
23f0773
Compare
.contains(cancelButton) | ||
.should('be.visible'); | ||
// Validate add/save button visibility | ||
cy.get(buttonSelector(submitButtonType)) |
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.
Are all the validations here needed? Did they help you reduce sporadic test failures where some field labels weren't yet in the correct state?
Finally, validation/asserts like this are fine but please make sure that if they fail, it's not too hard to locate the problem. There's a lot of them in this function so it might be hard to tell what's the problem at a glance.
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.
This isn’t aimed at resolving or minimizing sporadic test failures. I added these tests (Validate Add Namespace form fields
, Validate Edit Namespace form fields
) specifically to validate the form components, based on our earlier cypress discussion about deeper Cypress component testing.
If you feel this level of testing, where the entire component structure is verified is necessary, I can refactor them into separate modules to keep things readable and make debugging easier. I was also planning to add similar tests for other forms as well - schedule, tenant, collect-logs
If this is not required we can drop this 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.
Some we check visible and enabled and others just visible but not checking disabled. I'm mentioning it here now because it's hard to tell which of these are more important and it could make reading and debugging the test difficult.
Obviously, we can't test every element on a form in every way possible so we should start with the most important tests/assertions and add others as you find them being important to check. I'll leav that to you to check which of these assertions are important.
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 the element validation for consistency across all elements
cypress/e2e/ui/Automation/Embedded-Automate/Explorer/namespace.cy.js
Outdated
Show resolved
Hide resolved
} | ||
|
||
function createNamespaceAndOpenEditForm() { | ||
/* @replace-with-api: Use cy.request for namespace setup, excluding the test meant to validate functionality via UI */ |
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.
Ok, I understand this. This is a todo/fixme to try to replace this with an API way to create the data needed for the test.
Note, we'll need to manually delete things if the afterEach() is not currently functional.
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.
This is a todo/fixme to try to replace this with an API way to create the data needed for the test.
Yes correct.
Note, we'll need to manually delete things if the afterEach() is not currently functional.
afterEach
is behaving as intended only the data creation step (domain creation) was triggering the abort error we discussed.
Deleting works correctly with the UI session, I just have to append csrf token into the request headers.
aae8b13
to
98812a0
Compare
// retrieve the id and token from the aliased state | ||
// to invoke api for deleting the created domain | ||
cy.get('@idAndToken').then((data) => { | ||
cy.request({ |
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's keep iterating on this type of cleanup. I really don't know of a good way to do typical setup/teardown unit testing conventions since we need both the UI process and sometimes a backend worker process to create things in the UI (report preview in reports page requires a worker process to generate the preview data) so it's really difficult to hook into the database's transaction to rollback changes in a test.
The difficulty with this type of delete is:
- If errors occur anywhere in the setup or test code, the UI page could be in a different state so navigating to the right place to remove data is tricky. Is the
x_button
you are POST'ing to guaranteed to be there if the test failed at a random place before it got to the correct screen?
So, please verify if you have a fatal error in random places in the before and in the test, that the after still runs and is able to handle errors such as not being on the page.
We may need to find a way to do this through the API at some point so we can get away from needing to be in the correct page "state" to run these.
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 the x_button you are POST'ing to guaranteed to be there if the test failed at a random place before it got to the correct screen?
From what I've observed so far (particularly with domain data), the delete operation doesn't appear to depend on the UI state. I tried hitting the delete-domain endpoint right after logging in, as well as after switching to another menu, for instance:
"Test-Data" is a domain created manually through UI, id is 449
I’m making a cy.request to the delete-domain endpoint with ID 449
hardcoded in the URL, and I’m using the CSRF token obtained from the meta tag., right after navigating to Settings -> Application Settings
instead of Automation -> Embedded Automate -> Explorer


please verify if you have a fatal error in random places in the before and in the test, that the after still runs and is able to handle errors such as not being on the page.
Here I'm intentionally causing the test to fail after the domain is created - using an incorrect assertion.
In the afterEach hook, request is fired to the delete-domain endpoint.
Since domain named "Test_Domain" is created in beforeEach, failure to delete the domain would have caused the other test cases to fail with a duplicate name error.
This raises a follow-up question - should API access be restricted based on the current UI state, as observed during domain creation (edit abort error), while delete requests are not. Could this be a bug?
98812a0
to
a6e81d1
Compare
} | ||
|
||
function extractDomainIdAndTokenFromResponse(interception) { | ||
const rawTreeObject = interception?.response?.body?.reloadTrees?.ae_tree; |
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.
Can this chained value fail? Should this function throw an error if the rawTreeObject is not found?
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.
If rawTreeObject
isn't found, the cleanup will fail because the CSRF token and record ID won’t be stored in the aliased state. This can happen if the create API call returns an error in the response, even though the status code is 200 (so we can’t trust status alone). Worse, if the record is created in the DB, future tests may fail due to leftover data from the incomplete cleanup. Probably going to undo this change.
csrfToken, | ||
}; | ||
// Creating an aliased state to store id and token | ||
cy.wrap(idAndToken).as('idAndToken'); |
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 looks like this is for extracting the csrf token for the deletes in the afterEach below I'm ok with this for now but I do wonder if we need a generic way to do this through the actual API in the cypress tests. Thoughts on this @GilbertCherrie @elsamaryv @Fryguy
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.
Thoughts on this @GilbertCherrie @elsamaryv @Fryguy? @asirvadAbrahamVarghese is this PR ready to go? It was failing in schedule form. I'll close and open the PR so it's tested against the latest merge point.
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’d like to drop this change for now, which uses cy.request
with a CSRF token to delete data, but I’m unsure if we have a viable alternative. Not sure if we can hit our current endpoints without the token for create/delete operations. For creation, we’d also need to handle the 'Edit aborted: multiple tabs or windows' error if we reuse the same session.
For now, I’m thinking we drop this and just stick with the UI-based cleanup, What do you all think?
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.
LGTM. I have some concern about the csrf extraction for doing the deletes via the UI routes for miq_ae_domain_delete. I haven't had a chance to see how we can do this through the API more generically from the cypress tests.
00d5cd4
to
e79eb1a
Compare
e79eb1a
to
0d14fa8
Compare
Looks like the namespace tests are failing @asirvadAbrahamVarghese, please take a look ![]() |
05d5d59
to
983d269
Compare
efb9f00
to
b1b55db
Compare
b1b55db
to
de09d9a
Compare
CP4AIOPS-16490
Pr that adds cypress tests for Namespace form(Automation -> Embedded Automate -> Explorer -> {Any-created-domain} -> Namespace form)
@miq-bot assign @GilbertCherrie
@miq-bot add-label cypress
@miq-bot add-label test