JS unit tests: update popover matcher #54168
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ramonjd
requested review from
ajitbohra,
peterwilsoncc and
ellatrix
as code owners
|
Size Change: 0 B Total Size: 1.52 MB ℹ️ View Unchanged
|
|
Flaky tests detected in e71b618. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6091438030
|
Mamaduka
reviewed
Sep 5, 2023
Nooo! I thought I was special :( |
Yeah, I could reproduce it by running the tests just once 🙂 This PR is a nice refactoring, but it won't have influence on that bug. The bug is caused by the fact that the popover appearance is animated. In a rAF loop, the opacity goes from 0 to 1, and other properties like We used to have plans to add the |
jsnajdr
reviewed
Sep 5, 2023
There was a problem hiding this comment.
When introducing these wrappers a year ago with @tyxla, we had good reasons why they need to be separate. I would have to find the exact PRs where these discussions happened to know the details. But apparently these reasons disappeared over time, as this PR works perfectly fine.
| @@ -5,7 +5,8 @@ | |||
| * @param {HTMLElement} element Popover element. | |||
| */ | |||
| function toBePositionedPopover( element ) { | |||
| const pass = element.classList.contains( 'is-positioned' ); | |||
| const popover = element.closest( '.components-popover' ) || element; | |||
There was a problem hiding this comment.
There should be no need to default to element. If the element or its ancestor doesn't have the .components-popover class, the check should immediately fail because no popover to check has been found. There's no point checking whether a non-popover element has also the .is-positioned class.
There was a problem hiding this comment.
Also, it would be nice if the matcher detected that the element is either null or not inside a Popover, and printed a nice message. At this moment it's just a TypeError:
TypeError: Cannot read properties of null (reading 'closest')
TypeError: Cannot read properties of null (reading 'classList')
I think we had that at one moment in the past, somehow it got lost.
There was a problem hiding this comment.
Thanks for the tips and context here. Much appreciated 🙇🏻
Updated accordingly.
Thanks for testing @Mamaduka and @jsnajdr - also for the explainer. I'm not sure if it's my hardware, but it also took me 20+ times to fail. I though 10 was the magic number 😄 I wonder if mocking
Good to know. I've updated the PR to return more meaningful messages as per @jsnajdr's advice. If folks are okay with the refactor I'll merge, otherwise happy to just kill the PR mercifully. |
jsnajdr
reviewed
Sep 5, 2023
jsnajdr
approved these changes
Sep 5, 2023
| const popover = element && element.closest( '.components-popover' ); | ||
| const isPopoverPositioned = | ||
| popover && popover.classList.contains( 'is-positioned' ); | ||
| const pass = !! popover && !! isPopoverPositioned; |
There was a problem hiding this comment.
Some style nits:
- can use
element?.closestandpopover?.classListoptional chainings passcan be simply!! isPopoverPositioned, the!! popovercondition is redundant.
…rapping `getWrappingPopoverElement()`, so this commit just moves `getWrappingPopoverElement` logic into `toBePositionedPopover` to save everyone the trouble.
…element - change messaging based on error type
ramonjd
force-pushed
the
update/testing-matcher-popover
branch
from
f1cf806 to
e71b618
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR moves
getWrappingPopoverElementlogic intotoBePositionedPopover.This PR was originally designed to fix the flaky test in
packages/nux/src/components/dot-tip/test/index.js, which usedtoBePositionedPopoverwithout a correspondinggetWrappingPopoverElement.Why?
There was no use of the
toBePositionedPopover()matcher without a wrappinggetWrappingPopoverElement().The alternative to this PR is to just wrap the flaky test assertion in a custom
getWrappingPopoverElement().Testing Instructions
CI tests should pass
To make sure the
packages/nux/src/components/dot-tip/test/index.jstests pass, in your console, run: