-
Notifications
You must be signed in to change notification settings - Fork 15
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
Apply PageObject pattern Clusters Overview tests. #3266
Apply PageObject pattern Clusters Overview tests. #3266
Conversation
…move data-testid selectors for consistency
…d interaction functions in clusters overview po
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.
Hey @vicenteqa looking good.
Besides a couple of minor things, my main doubt is whether keeping the skipped tests.
We might just not have those, which anyway are not running and track an actionable to actually address them properly.
What do you think?
}; | ||
|
||
export const addTagButtonsAreDisabled = () => | ||
cy.get(addTagButtons).should('have.class', 'opacity-50'); |
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.
suggestion: I am aware you found it like this, however what about relying on the disabled
attribute rather than the css class?
Same goes for the following.
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 will check :)
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 checked and these elements don't have the disabled
attribute
<span class="leading-5 font-semibold opacity-50 pointer-events-none rounded-full px-2 py-1 text-sm inline-flex text-green-800 bg-green-100 flex items-center cursor-pointer hover:scale-110 transition ease-in-out delay-50" aria-hidden="true"><svg class="" data-testid="eos-svg-component" transform="rotate(0) translate(0, 0) scale(1, 1)" fill="#276749" width="14" height="14" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><path d="M21,12l-4.37,6.16C16.26,18.68,15.65,19,15,19h-3l0-6H9v-3H3V7c0-1.1,0.9-2,2-2h10c0.65,0,1.26,0.31,1.63,0.84L21,12z M10,15H7v-3H5v3H2v2h3v3h2v-3h3V15z"></path></svg> Add Tag</span>
so it will have to stay like this
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 see, they're not actual buttons. Ok let's keep as it is, maybe we can improve in the future by using aria-*
attributes when needed.
Thanks for double checking.
@vicenteqa as per our offline discussion I am fine eitherway: we can keep or remove skipped 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.
LGTM
Description
Apply page object pattern to clusters overview tests
Key points:
Bonus track:
No changes required in docs.
Fixes # (issue)
Did you add the right label?
Remember to add the right labels to this PR.
How was this tested?
Describe the tests that have been added/changed for this new behavior.
Did you update the documentation?
Remember to ask yourself if your PR requires changes to the following documentation:
Add a documentation PR or write that no changes are required for the documentation.