-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Components: update ColorPicker
unit tests
#49698
Merged
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
chad1008
added
[Package] Components
/packages/components
[Feature] Component System
WordPress component system
labels
Apr 10, 2023
jsnajdr
reviewed
Apr 12, 2023
tyxla
reviewed
Apr 12, 2023
chad1008
force-pushed
the
ColorPicker-rework-unit-tests
branch
from
April 13, 2023 15:20
4c3f148
to
9fd4841
Compare
jsnajdr
approved these changes
Apr 14, 2023
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 have one more suggestion for improving a color picker query. Otherwise this looks good 👍 Thanks for addressing all feedback.
bph
added
[Type] Build Tooling
Issues or PRs related to build tooling
[Type] Automated Testing
Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
and removed
[Type] Build Tooling
Issues or PRs related to build tooling
labels
Apr 26, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
[Package] Components
/packages/components
[Type] Automated Testing
Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
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?
Updates
ColorPicker
's unit tests to provide more meaningful feedback, and replacefireEvent
withuserEvent
.Why?
The existing tests attempted to manipulate the two-dimensional color picker, and would check to see if
onChange
was fired with a hex or a hex8.While attempting to migrate these tests from
fireEvent
touserEvent
I found that while they might fire with the right format, it was never actually the correct color. It was always#000000
, optionally with two more digits for the alpha value. Chatting with @ciampo and @jsnajdr revealed that this is due to JSDOM not rendering layout, so moving slider handle between coordinates isn't actually possible.If I recall correctly, this led to a fun bug where trying to slide a slider resulted in effectively toggling it between 0 and infinity... strangeness ensued and long story longer, the existing tests weren't actually functioning as they appeared to be.
The new/updated tests aim to focus on the inputs our unit tests can get accurate data from.
How?
The individual inputs for hex, rgb, and hsl inputs are all tested independently. The existing test for legacy props (which uses a different internal format for color values and different callback when the value changes) has been updated as well.
I had initially hoped to also test the inputs as a group (e.g. updating all three rgb values and confirming the correct color was called) but this didn't prove possible. When updating the second or third value, the previous value(s) always reset to zero. I suspect this is happening due to the physical slider being updated to match the value entered by the test, which fails because of the no-layout issue I've described above.
Followup
I'd like to add another test for the
Copy
button, but I wasn't quite sure how to properly mock it. Rather than delay this PR, I'll put this on a list to come back to later.Testing Instructions
Confirm unit tests pass