Autocomplete Component: add e2e tests (take two) #42905
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
|
Size Change: +1.72 kB (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
Just ping us when it's time! |
chad1008
force-pushed
the
add/Autocomplete-e2e-tests-take-two
branch
from
5a8e261 to
de8f68e
Compare
|
Marking this as ready for review (cc @ciampo @mirka). I wasn't ultimately able to find a way around the crash caused by the infinite loop in one of the regressions I wanted to test against. The test in question is skipped, but still included just in case we're ever able to make use of it. Happy to remove it if need be! |
chad1008
force-pushed
the
add/Autocomplete-e2e-tests-take-two
branch
2 times, most recently
from
136ed73 to
c2d27f2
Compare
ciampo
added
[Type] Automated Testing
ciampo
reviewed
Sep 1, 2022
There was a problem hiding this comment.
Props for writing clear tests, well commented, and with semantic assertions!
I confirm that some of the tests fail when adding changes from #41382, which is great and exactly what this PR is for ✅
Apart from inline comments, here's a couple more observations — although we're quite close to being able to merge this one!
Local failures
For some reason, when I ran npm run test:e2e -- packages/e2e-tests/specs/editor/various/autocomplete-and-mentions.test.js on my local machine, tests all timed out.
If then added the --puppeteer-interactive flag, and they all passed 🤷
From that point onwards, running the standard command (i.e. without --puppeteer-interactive) also worked.
E2E vs unit/integration
It still bugs me that we're not able to write these tests as Testing-Library unit tests, but we've already been through that conversation.
This component is tightly coupled with the editor as, as @mirka suggests, we should look into potentially moving it to another package (e.g @wordpress/block-editor), or at least improving its documentation.
packages/e2e-tests/specs/editor/various/autocomplete-and-mentions.test.js
Outdated
Show resolved
Hide resolved
packages/e2e-tests/specs/editor/various/autocomplete-and-mentions.test.js
Outdated
Show resolved
Hide resolved
packages/e2e-tests/specs/editor/various/autocomplete-and-mentions.test.js
Outdated
Show resolved
Hide resolved
chad1008
force-pushed
the
add/Autocomplete-e2e-tests-take-two
branch
from
bc4ed55 to
cb41332
Compare
chad1008
commented
Sep 21, 2022
ciampo
approved these changes
Sep 23, 2022
There was a problem hiding this comment.
I'm currently unable to run these tests locally, as I'm getting an error when trying to tun WordPress on Docker
But tests seem to pass in CI, and code changes LGTM.
Probably better if @WunderBart and/or @kevin940726 , who are more knowledgeable about PlayWright than I am, also take a look at this PR before merging.
I also appreciated the granularity of the comments, it really helped reviewing the refactor to Playwright 🙏
kevin940726
reviewed
Sep 24, 2022
There was a problem hiding this comment.
Nice job! Just a small thing regarding using role selectors for more readable selectors that don't depend on implementation details. Other than that it looks good to me! 👍
test/e2e/specs/editor/various/autocomplete-and-mentions.spec.js
Outdated
Show resolved
Hide resolved
test/e2e/specs/editor/various/autocomplete-and-mentions.spec.js
Outdated
Show resolved
Hide resolved
test/e2e/specs/editor/various/autocomplete-and-mentions.spec.js
Outdated
Show resolved
Hide resolved
test/e2e/specs/editor/various/autocomplete-and-mentions.spec.js
Outdated
Show resolved
Hide resolved
…and test callbacks
…sts. Update test names as needed
…ion and publication
chad1008
force-pushed
the
add/Autocomplete-e2e-tests-take-two
branch
from
cb41332 to
3886ba4
Compare
|
Thanks all! I've made some updates based on the provided feedback. @WunderBart/@kevin940726 let me know if anything else looks out of place! |
kevin940726
reviewed
Sep 27, 2022
test/e2e/specs/editor/various/autocomplete-and-mentions.spec.js
Outdated
Show resolved
Hide resolved
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?
Add some basic e2e testing for the Autocomplete component, as well as some tests specific to past regressions
Why?
The component currently lacks detailed testing, outside of some user mention tests. Increased testing will allow for safer/more confident updates to the component in the future.
How?
This is an alternate approach based on the initial work and feedback in #42674
There are two specific regressions introduced here (already reverted) that I want to include tests to avoid. See issues below for more details on those regressions:
These regression appear to effect only certain completers, like user mentions. I think the issue is actually any autocompleter that relies on fetched data to populate its options. I haven't fully confirmed that theory, but the important part is that these regressions don't impact a completer whose options come from a simple locally declared object.
With that in mind I wanted these tests to confirm if a failure was impacting simple completers, more complex completers (like user mentions), or both. To accomplish this I'm running the tests through a
describe.eachto account for all cases. Because the data set (the completer options) is different for the two paths, most of the tests conditionally define their test data within the test itself. This way the testing logic is the same but the data is tailored to each completer. It should also make it relatively easy to test additional completers in the future if needed.I also had to add a bit of logic for a single test in the
.eachto be skipped for user mentions. I wrapped this logic in a helper function so it can also be easily reused in the future if necessary.Finally, in addition to creating a custom completer, the testing plugin also creates a clone of the existing user mention completer. This is used in one test to confirm that two complex (i.e. data-fetching) completers used in the same block won't cause the browser to crash (one of the regressions I'm trying to guard against). Unfortunately, while this cloning process works and accurately triggers the regression, the infinite loop at the heart of that regression also crashes our tests 🤦. I haven't found a way around that, so I've left the test skipped for now. We can remove it (I know the linter disapproves of skipped tests) if that's preferable, but for now I opted to keep it in case we do find a way to utilize it while still getting test results in the event that this particular regression is ever reintroduced.
Testing Instructions
npm run test:e2e -- packages/e2e-tests/specs/editor/various/autocomplete-and-mentions.test.jsAutocompleteto passexhaustive-deps#41382 to re-introduce the regressions. Rebuild Gutenberg.Optional: Finally, if you remove the
.skipfrom theAutocomplete: should insert elements from multiple completers in a single blocktest and re-run the suite, you should find that it simply hangs, not returning any data. I'm calling this an optional step because we're really just testing that the test can trigger the regression by breaking your browser, which isn't actually all that helpful 😉