Cover block: Add integration tests to check isDark settings #53256
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
glendaviesnz
added
[Type] Automated Testing
|
Size Change: +3.33 kB (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for adding these tests and laying the ground work to fix the flickering Cover blocks 👍
The tests run successfully however I think we can avoid the container.getElementsByClassName approach.
I've left an inline comment with a suggestion that still has the test passing. Let me know if you think that will work.
andrewserong
reviewed
Aug 2, 2023
There was a problem hiding this comment.
These are testing well for me, too! The structure of what's happening in the tests looks good and logical to me 👍
One other thing I noticed is that the tests take a long-ish amount of time to run. Four tests doesn't sound too bad to me, but in case the running time of the tests is an issue, I wonder if it'd be acceptable to consolidate around just two tests? Feel free to ignore if the added milliseconds don't matter too much for these integration tests, though 🙂
Here's the timing for me locally:
Thanks for adding coverage here!
These tests will take longer to run than unit tests as they are firing up the whole editor, so waiting for that to initialise, popup windows to be rendered, etc. But, still significantly quicker than an e2e which would take 3000ms + for each test. However, your comment did make me take another look, and I think most of the tests were actually testing pretty much the same thing, so reduced it to one test, and added another that covers a case I wasn't checking which is removing the overlay color, so down to 2 tests which I think give enough coverage - but let me know what you think. |
glendaviesnz
commented
Aug 2, 2023
| } ); | ||
| await userEvent.click( popupColorPicker ); | ||
| expect( coverBlock ).not.toHaveClass( 'is-light' ); | ||
| } ); |
There was a problem hiding this comment.
Actually I think the following is a bug as if there is no overlay color it defaults to black, so doesn't seem to make sense to then set it to is-light , but I think we should leave this here and then update this test if we decide to fix this bug as part of the jitter fix.
There was a problem hiding this comment.
+1 one to fixing that bug separately. We can discuss it and get design input on a dedicated issue for it.
aaronrobertshaw
approved these changes
Aug 2, 2023
There was a problem hiding this comment.
Appreciate the iteration on this @glendaviesnz 👍
Tests all pass. Code LGTM.
Regarding the longish time the tests take to run, they compare roughly to the block toolbar tests for the block. I think the protection the tests may offer against regressions is worth it.
andrewserong
approved these changes
Aug 2, 2023
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?
Adds some tests to the Cover block to verify the existing isDark workflows
Why?
To fix #53211 we are probably going to need to do a major refactor of the isDark useEffects, so it will be good to have a clear understanding of the current functionality, and have tests for it where possible, so we can easily spot any regressions.
How?
Adds intergation tests
Testing Instructions
npm run test:unit block-library/src/cover/test/edit.js