-
Notifications
You must be signed in to change notification settings - Fork 367
Add cypress command select accordion item #9522
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
Add cypress command select accordion item #9522
Conversation
|
Checkout to this branch and run This test - a0a7779 - will be dropped before merge |
|
@asirvadAbrahamVarghese Don't worry about it for this PR, but try to keep commit message titles 72 chars or less. This helps avoid the text being elided on GitHub. For example: (72 chars because on the CLI git log adds a tab character at 8 char width, and 72+8 = 80) |
elsamaryv
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.
LGTM
@GilbertCherrie Looping you in to take a look as well.
cypress/support/commands/explorer.js
Outdated
| message: `Matched "${liText}" at index ${i}`, | ||
| }); | ||
|
|
||
| // Wrap the current li element in a jQuery object to per |
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 is a typo in this 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.
Updated - be6345
cypress/support/commands/explorer.js
Outdated
|
|
||
| // Wrap the current li element in a jQuery object to per | ||
| const currentLiElement = Cypress.$(listItems[i]); | ||
| const hasAngleRight = |
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.
Maybe rename hasAngleRight to isExpanded or something similar ? That would give better clarity about what this flag represents instead of focussing on the icon class used.
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.
How about isExpandable, since the purpose of the variable is to determine whether the node can be expanded?
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 makes sense
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 - be6345
cypress/support/commands/explorer.js
Outdated
| // Exit the current function scope | ||
| return; | ||
| } | ||
| // Click the most child node and terminate |
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 believe you meant
| // Click the most child node and terminate | |
| // Click the last child node and terminate |
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 - be6345
5736237 to
70caab4
Compare
| ]); | ||
| }); | ||
|
|
||
| it('should not collpase already expanded node', () => { |
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.
typo: collapse
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.
| } | ||
| // Click the node corresponding to the last label in the given path and terminate | ||
| cy.wrap(currentLiElement).click(); | ||
| return; |
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 function is a bit hard to follow.
These returns feel like guard clauses. Is the indentation correct here?
Would it be easier to read to do:
if (isClickableNode) {
cy.wrap(currentLiElement).click();
return;
}
... // rest of non-guard clausesIf we return early in guard clauses, we don't need to worry about if/else as we already returned if the guard clause was true/false.
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.
These returns feel like guard clauses. Is the indentation correct here?
Pretty sure the indentation is correct, since it's auto-formatted by Prettier 🤔
If we return early in guard clauses, we don't need to worry about if/else as we already returned if the guard clause was true/false
Agreed, changed the sequence - be6345
bbaa961 to
0e8ec14
Compare
0e8ec14 to
a0a7779
Compare
| @@ -0,0 +1,50 @@ | |||
| /* eslint-disable no-undef */ | |||
|
|
|||
| describe('Validate clickItem', () => { | |||
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 not sure what to name this but it should probably match the command name 'selectAccordionItem". That can be done later. I think we should get this in and see how we use it to help us better understand if the naming or structure could be improved.
| * If the path is not found, it will throw an error. | ||
| */ | ||
| Cypress.Commands.add('selectAccordionItem', (accordionPath) => { | ||
| cy.get('li.list-group-item').then(($items) => { |
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 curious if our other pages will be compatible with the selectors used here and below. As you use this shared command, make note of things that are different as we can change code to be more consistent or make different formats of the same command for different styles of pages that do things differently.
jrafanie
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.
Merging, I want to see how this looks as it's used.
…de using a path array
Added new cypress command to select accordion item based on the given path to that node.
Implements full tree traversal in a single pass till the target node. Once the first matching node is found, subsequent searches continue from that point onward, avoiding redundant iteration from the beginning.
E.g:

cy.selectAccordionItem([/^ManageIQ Region/, 'Tenants', 'My Company']);As seen below, if the "Tenants" node is found at index 57 and it's not the target node, it will be expanded (if not already). The search for the next node, "My Company", will then begin from index 58 onward.
@miq-bot assign @elsamaryv
@miq-bot add-label cypress
@miq-bot add-label test