-
Notifications
You must be signed in to change notification settings - Fork 364
Added automated tests with cypress for Schedule form under Applicatio… #9504
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
Added automated tests with cypress for Schedule form under Applicatio… #9504
Conversation
@asirvadAbrahamVarghese Can you split the tests up into multiple tests. Try not to combine everything into 1 test. The different test cases can be:
|
Can you change the file name to schedule.cy.js |
cc @jrafanie |
/* ===== Adding a schedule ===== */ | ||
addSchedule(); | ||
cy.get('#main_div #flash_msg_div .alert-success').contains( | ||
'Schedule "Test name" was saved' |
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, we'll need to be careful of side effect test which modify the tables that are presented in the UI as a failure could fail to clean up after itself. This could cause other tests to fail, for example, if they are listing schedules and asserting the contents.
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.
‘Test name’ and ‘Dummy name’ are the two possible schedules created during these tests. To handle cleanup, defined an afterEach block that checks for these names and deletes them if they exist, especially in cases where assertions might fail - 82d569885aa283acb65a0e51db59ae14aebe088f
During GHA, since these tests will run against a blank database, we shouldn’t expect any pre-existing records with those names. However, if that’s not guaranteed, we might have to consider to generate unique schedule names for each test using a combination of a base name and a timestamp or date string, with the help of Date utility functions.
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.
Right, my comment was more about cleaning up afterwards. So, keep side effects, such as new records that are displayed in the UI, as minimal as possible and we can eventually move to doing cypress-rails way with transactions that rollback changes by labeling side effect tests and doing this rollback for these.
It looks like some of the tests are sporadic in nature and are fairly slow. Sporadic failures are often timing issues waiting for ajax or pages to be rendered as the computer drives the browser much faster than a user would. I don't know what's "too slow" but it would be good to note any steps in the tests that are slow and either comment on it so we can try to fix it later or see if we can make them faster. How slow is it locally @asirvadAbrahamVarghese ? |
FYI, I started on some work to try to get each cypress test file to login only once and share that session with each test in the file. Unfortunately, there is other state in the session that leaks between tests in this way, but it might be something we can do if we can figure out how to share just the login and not have to do that in each test. See: #9499 |
Yeah right @jrafanie, this can fail intermittently due to timing issues, I am looking further into that. With session management implemented (as in PR #9499 ), the execution time should improve. I just ran the tests using session management. |
By the way, we haven't really documented it but there are two improvements to set in your environment but they must be understood as they can cause issues
|
}); | ||
|
||
afterEach(() => { | ||
cy.get('li.list-group-item').each(($el) => { |
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.
One concern to keep in mind is tests here can fail in any part of the test. You may need to ensure you're on the right page by visiting it first, before looking for the list group item.
Ultimately, I hope we can minimize the tests that make changes to what's presented on the various screens by saving or creating objects. The hope is we can "tag" these tests with "rollback" or "transaction" or something so we can transparently tell the database to begin a transaction and automatically roll it back afterwards.
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.
You may need to ensure you're on the right page by visiting it first, before looking for the list group item.
I just navigated through the UI looking out for route URLs just to confirm /ops/explorer maps only to Settings -> Application Settings - Please correct me if that’s not the case.
So this change kind of confirms that we are on the right page before trying to perform clean up - c98c6fd
we can transparently tell the database to begin a transaction and automatically roll it back afterwards.
Yes, implementing something like that could help us drop the cleanup scripts entirely and improve test performance. I’ll give it a try...
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.
Yes, implementing something like that could help us drop the cleanup scripts entirely and improve test performance. I’ll give it a try...
We can pair program on that. There's cypress-rails that can do that. I think we can leverage that with cypress grep so we can tag tests that need the BEGIN transaction / ROLLBACK around the tests.
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.
In other words, it's not required for this PR. It's a longer term solution we're thinking of doing.
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 just navigated through the UI looking out for route URLs just to confirm /ops/explorer maps only to Settings -> Application Settings - Please correct me if that’s not the case.
So this change kind of confirms that we are on the right page before trying to perform clean up - c98c6fd
👍 Thanks, I didn't look to see if we moved to other forms within the test where the list group item might not show up.
cypress/support/e2e.js
Outdated
if (err.message.includes(`Cannot read properties of undefined (reading 'received')`) || // Error handler for Chrome | ||
if (err.message.includes(`Cannot read properties of undefined (reading 'received')`) || // Error handler for Chrome | ||
err.message.includes(`Cannot read properties of undefined (reading '0')`) || // Error handler for Chrome | ||
err.message.includes('response.filtered_item_list[0] is undefined') || // Error handler for Firefox |
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 know we're handling errors here. I'm curious if these are from web socket notifications. If so, I wonder if we can improve behavior and reliability by disabling web socket notifications in cypress EXCEPT in tests that are testing web socket notifications. I haven't looked at that yet cc @GilbertCherrie @elsamaryv
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.
Along these same lines, I was noticing that we always load the 4 dashboard widgets (chart/reports) after login on each page. Part of the PR I started doing to try to share the same login between tests in the same file, #9499, I wanted to change login to load a different page after the login such as /utilization or /optimization... I wonder if we can leverage the "my settings" start page to pick a page that is a faster start page for all users:
Another option would be to customize our default dashboard for cypress to have no widgets on the loading page and switch to a dashboard with graphs/charts for tests for dashboards.
cc @Fryguy (note, I think Jason had the idea for the start page)
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 know we're handling errors here. I'm curious if these are from web socket notifications.
This one Cannot read properties of undefined (reading '0') is JS run time error thrown from here,
I think the best approach is to fix such run time errors from application level rather than handling them during tests.
response?.filtered_item_list?.[0]?.[0]
this should have helped but seems like few of our babel plugins are a bit old (they can't parse this saying Unexpected token) so we will have to chain the validation with AND
response && response.filtered_item_list && response.filtered_item_list[0] && response.filtered_item_list[0][0]
Do you think this change should also go along with this PR?
The other error seen in Firefox, response.filtered_item_list[0] is undefined, isn’t appearing anymore, so we probably don’t need to worry about it, removing - 4e7b327
Tried with CYPRESS=true ( |
1f7f6ea
to
7f09ffa
Compare
Great. 43 vs. 35 if that's all that was changed is not bad. Since you did "open" mode (cypress open vs. cypress run), retries should be disabled. If you do run mode, (cypress run), you need to disable retries locally to do any benchmarking... this is why I made open mode not run with retries... if you need run mode to also skip retries, you can disable it here: https://github.com/ManageIQ/manageiq-ui-classic/blob/master/cypress.config.js#L22 Note, CI runs in run mode, so for now, we default local to also run with retries. Note, even in the CI test run, the tests that didn't retry were pretty long. It's only a problem if you see it's really slow. Slower tests that test a lot of things are fine. It's really only a problem if we're needlessly testing unrelated things or testing the same things in different tests.
And retries aren't going to be completely eliminated since the application is asynchronous in nature so we want to see if there are easy ways to eliminate some of the timing based retries without wasting too much time trying to fix things that are just too hard to prevent from happening all the time. In other words, if you see a test always retry multiple times, it might be worth seeing if you can figure out why. If it's sometimes retrying, that's fine. |
@GilbertCherrie This is something we should add to the cypress docs you wrote up. |
).as('getExpandedNotifications'); | ||
cy.menu('Settings', 'Application Settings'); | ||
cy.wait('@getNotifications'); | ||
cy.wait('@getExpandedNotifications'); |
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.
FYI, I was playing around with fixtures to try to reduce the calls to /api/notifications
. It didn't really speed things up but it was needless noise in the cypress open
output. It's not currently stubbing notifications but is easy to add. Note, I'm not capturing the intercept with an as
so I'm not waiting for them. We can probably do that if it's useful.
See: #9489
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.
yeah right, intercepting and waiting for the notifications api is not helping I am going to remove it in my next commit.
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.
Great. It's worth trying different ways to make these tests more reliable. We'll be able to make better decisions as we try different things and learn more about what causes the failures.
goToAppSettings(); | ||
cy.intercept('POST', '/ops/tree_select?id=xx-msc&text=Schedules').as('getSchedules'); | ||
cy.get('[title="Schedules"]').click(); | ||
cy.wait('@getSchedules'); |
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 there's are the right way to improve test reliability by waiting on the various requests to complete before moving on. We might need to refactor common intercepts
as helper methods. Keep this in mind as you find that you need to do this over and over again.
We have similar concerns where we don't use unique class, ids, etc. in the DOM. We should probably change code where possible to make it much easier to find elements in the DOM for testing.
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, this is a mental TODO. Let's keep in mind what we need to intercept and wait in order to make tests be more reliable. As we get a handful of common patterns, we can try to refactor them to helper methods.
selectConfigMenu('Edit this Schedule'); | ||
// Editing name and description | ||
cy.get('input#name').clear().type('Dummy name'); | ||
cy.get('input#description').clear().type('Dummy description'); |
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 found that our code does ajax calls to form changed or form field changed each time we type. I needed to fix an issue in the report pages by waiting for these ajax calls to complete before moving to the next field. See: #9487
); | ||
|
||
/* ===== Deleting schedule ===== */ | ||
deleteSchedule('Dummy 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.
Do we need this if it's handled in the after each?
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.
The afterEach was intended to check for any leftover data from test failures and delete it if present, but as you mentioned, we don’t really need these explicit calls since the cleanup process already handles that - 68a9ec5
Understood, I’ll try one last thing that I have in mind, then I’ll tap out 😄 |
cy.contains('li.list-group-item', scheduleName).click(); | ||
cy.on('window:confirm', (text) => { | ||
expect(text).to.eq( | ||
'Warning: This Schedule and ALL of its components will be permanently removed!' |
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 don't know if we need to test for the exact wording on the confirmation dialog. It's fine for now but we might want to just check for a case insensitive 'warning'. If we change wording in the code, we may have to update several places in tests. For the purpose of the test, you just care that there's a warning.
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.
Yeah right that will be a problem going forward, this is the browser dialog that appears asking for confirmation to delete.
Instead of checking the exact text, we can simply verify that it was displayed right?
and should we consider these type of alerts the same way and ignore the text?
We could move these static strings to a constants file so they can be reused by both the component and the corresponding tests.
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.
Exactly. I can see a confirmation_warning
helper or similar that could check that generically. Similar for "success_saved" or similar for successful notifications on save.
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.
Added helpers to assert alert messages - f92730c
df0acbb
to
349aad9
Compare
Yes I am just doing some manual cleanup with cherry-picks and squashes to avoid messy merges, I’ll give you a shout if I get stuck
Up next, I’m checking that out.. |
91aad75
to
3967d77
Compare
def00ef
to
375fab2
Compare
375fab2
to
df5dcbc
Compare
5aacc65
to
a2c7783
Compare
c18b2de
to
31b33e6
Compare
31b33e6 - Updated the toolbar command to return a Cypress chainable(here), making it compatible with Also replaced forEach with a for loop to allow early loop exit using return.
|
|
||
/** | ||
* Custom Cypress command to validate flash messages. | ||
* @param {string} alertType - Type of alert (success, warning, error, info). |
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.
Sorry, I think this one should be flashType
after we decided to rename it.
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.
Again my bad, missed the variables too, will make 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.
Done - 09cdb2b
Looks good to me, just a minor rename/type to fix. I'm ok if you want to merge this now @GilbertCherrie. It can be cleaned up afterwards if need be. |
Checked commits asirvadAbrahamVarghese/manageiq-ui-classic@46fb33c~...09cdb2b with ruby 3.1.7, rubocop 1.56.3, haml-lint 0.64.0, and yamllint |
@GilbertCherrie this looks good to me. We did a lot of rework in here and we'll continue to refactor things as we learn more. I'm going to merge. Nice job @asirvadAbrahamVarghese! |
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
…n-Settings
CP4AIOPS-16165
Pr that adds cypress tests for Schedule form(Settings > Application Settings > Settings > Schedules > Configuration > Add a new schedule)
Tests executed across browsers



@miq-bot assign @GilbertCherrie
@miq-bot add-label cypress
@miq-bot add-label test