-
Notifications
You must be signed in to change notification settings - Fork 367
Replace cypress manual UI catalog setup with factories #9699
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
Replace cypress manual UI catalog setup with factories #9699
Conversation
|
@asirvadAbrahamVarghese can you review this and try it locally? I see a few problems running it with:
and an ajax tree_autoload happens after it already fails... so it didn't wait for it: Note, we do a tree_select intercept but I don't see one for the tree_autoload. |
| cy.selectAccordionItem([ | ||
| ALL_CATALOG_ITEMS_ACCORDION_ITEM, | ||
| UNASSIGNED_ACCORDION_ITEM, |
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.
We don't need this navigation to "Unassigned" node here, could you please remove this also?
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.
That's actually causing a failure described in #9705 (comment)... while it makes sense to remove that line here, I'm curious if there's a valid use case that could hit that bug.
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 tried a few scenarios and here’s what I found: this one fails because the first selectAccordionItem call clicks "Unassigned" node, which triggers both tree_autoload and tree_select calls. We’re not waiting for both right now, since the interceptApi command only supports a single alias.
While the tree_autoload from the first selectAccordionItem call is still in progress, the next selectAccordionItem call starts. At that point, the expand button (span.fa-angle-right) isn’t available yet (it’s still showing the spinner - span.fa-spinner), so the command assumes the node is already expanded and moves on to find the target node ("Test-Catalog-Item"). The search reaches the end of the tree and throws an error because the node isn’t there yet...

This 🔝 is a pretty rare case where we click the parent node and immediately try to expand it to select a child in the next step,
But calling selectAccordionItem back to back shouldn't cause trouble, trying something like selecting different nodes within the same accordion tree:
cy.selectAccordionItem([
'All Catalog Items',
'My Catalog',
'Testing',
]);
cy.selectAccordionItem([
'All Catalog Items',
'Unassigned',
'Test Catalog Item',
]);
First call to expand "My Catalog" and select "Testing":

Second call to expand "Unassigned" and select "Test Catalog Item"::

We might need to enhance interceptApi to handle multiple aliases from a single event and make sure it plays nicely with Cypress, I’ll start working on it slowly but soon
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.
For now lets try this: #9706
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, #9706 is merged and I verified it worked. The extra cy.selectAccordionItem call doesn't make sense here, but in case we have a use case where we select two different accordion items back to back, that change should allow it now.
|
Let’s give this one a try and see how it goes: #9705 |
34a6604 to
482d024
Compare
| ]); | ||
| addCatalogItem(CATALOG_ITEM_NAME_1); | ||
| addCatalogItem(CATALOG_ITEM_NAME_2); | ||
|
|
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 important to note, this test was already doing the test teardown below:
afterEach(() => {
cy.appDbState('restore');
});Note, the resource actions aren't required for the test but creating the service template in the UI does create them so they been kept to match the UI behavior.
482d024 to
f8ffebb
Compare
|
ok, all ready to go on this one... still 💚 in CI |
asirvadAbrahamVarghese
left a comment
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, lets get this in





Replace UI creation of service template with factories (catalog)
Note, the resource actions aren't required for the test but creating the
service template in the UI does create them so they been kept to match the UI
behavior.
Follow up to #9697