-
Notifications
You must be signed in to change notification settings - Fork 365
[WIP] - Added automated tests with cypress for cloud-provider form #9620
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 cloud-provider form #9620
Conversation
28cf4f4
to
f9e5dba
Compare
73928e3
to
4e01348
Compare
TODO:
|
This is really nice! I'd be happy to merge as is, but I want us to consider pluggability with this. If possible it would be nice it plugins could bring their own cypress tests. cc @agrare Would like your thoughts here as well. |
Completely naive to cypress observation, but is there any way to split this up into smaller components? I don't know where to start reviewing almost 5k lines in one file. Also +1 on merging as is and improving as we go though |
Do you think we should do this to break the tests out?
|
How much shared code would they have? How difficult would it be to create a shared helper that each test could import? I think that can be done in a followup if necessary. @asirvadAbrahamVarghese let us know if you think it would be easier to do this now or later.
I think these can be done after this PR is in. |
}); | ||
|
||
// Validate elements | ||
validateVmwareVcloudFormFields(true); |
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.
So, I can't review this whole thing because it's huge but I think creating shared helper functions could help eliminate some duplication and dry up some of the extra code here. For example, this function is defined in each of the cloud provider vendor types. A shared function should be the next step.
Maybe let's get this in and then refactor it into shared functions. If we're still at several hundreds of lines, maybe we need to split out the tests per vendor.
4e01348
to
af85e37
Compare
Just working on cleaning this up to reuse a bunch of stuff. Better to wait for that to land and see how it looks, if I come back to it later, I’ll likely lose track... |
af85e37
to
6f3edd0
Compare
* Generates all test suites for a provider | ||
* @param {Object} providerConfig - The provider configuration object | ||
*/ | ||
export function generateProviderTests(providerConfig) { |
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 maintain the current test setup, this isn’t defined as a Cypress command because it’s executed directly in the top-level describe
block, rather than inside a test (it
) block. Cypress commands must be used within it
blocks
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.
Ah... that makes sense. We have a similar limitation with defining methods in our rspec tests. If you try to define it at the wrong test scope, it doesn't work.
6f3edd0
to
896538b
Compare
896538b
to
4d6cbf6
Compare
* @param {boolean} assertDeleteFlashMessage - Whether to assert the delete flash message | ||
*/ | ||
Cypress.Commands.add( | ||
'selectProviderAndDeleteWithOptionalFlashMessage', |
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'm looking forward to replacing all of this deletion code with:
afterEach(() => {
cy.appDbState('restore');
});
from: #9633
For now, this is good... I need a reference that works first. It will be good to not have to worry about cleanup as it distracts from the test.
const uniqueId = generateUniqueIdentifier(); | ||
nameFieldValue = `${providerConfig.nameValue} - verify-edit-form-validation-error - ${uniqueId}`; | ||
hostValue = `${slugifyWith(providerConfig.type, '-')}-${uniqueId}.com`; | ||
if (isAzureStack) { |
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 can't believe you figured out all of the different pages that needed these conditionals for all the providers. Nice job. These conditionals could hopefully go away if we can standardize the pages.
Yeah still good with merging this as is and improving as we go |
CP4AIOPS-18919:
Validates add, edit, refresh & delete actions on all the 11 cloud provider forms:
@miq-bot assign @jrafanie
@miq-bot add-label cypress
@miq-bot add-label test
@miq-bot add-label wip