Add regression test for editor JS crash caused by rtlcss parsing exception #29326
Conversation
|
Size Change: +1.39 kB (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
fullofcaffeine
force-pushed
the
add/regression-test-rtlcss-parser-comment-crash
branch
from
41859d4 to
f17d95f
Compare
|
Wondering if we could a different puppeteer helper here, such as |
This was the first approach I tried. I've set the |
I tried again and setting the attribute on Peek.2021-02-25.13-13.mp4
|
fullofcaffeine
force-pushed
the
add/regression-test-rtlcss-parser-comment-crash
branch
from
f17d95f to
0e518ca
Compare
|
Spent more time tinkering, seems the only way is to re-append the scripts, let me know if you have any other ideas. I've updated and cleaned it up. I was also having a hard time coming up with a jest expectation that would detect the error. Without any expects, an error was still being captured by Jest, but no additional details were being shown:
Which could work but would make it a bit harder to diagnose the test by looking at it. I tried to use an expectation like: But it didn't work, it wouldn't detect the CSS syntax error, I guess because of a race condition. I've found a workaround, refer to the commit. This seems to work consistently. EDIT: I might actually try to capture the error message to also expose it in the `expect error message. EDIT2: There seems to be no straightforward way of adding a custom error message in Jest without creating a custom matcher. I'll leave it as is this time. |
fullofcaffeine
requested review from
ajitbohra,
nerrad and
ntwb
as code owners
fullofcaffeine
changed the title
| const head = document.head; | ||
|
|
||
| const componentsScriptSrc = scripts.find( ( s ) => | ||
| s.src.match( 'components/index.js' ) |
There was a problem hiding this comment.
I don't quite like the fact that this ends up coupling this test with the build output (to a portion of the path to the compiled entry-point for @wordpress/components), but maybe it's fine as it won't change often ¯_(ツ)_/¯
The alternative would be to just get all scripts without filtering, unshift the snippetScriptEl, and then re-add all of them again later. I tried that approach and it works, but then we would be re-evaluating all scripts, which is quite inefficient because we only really need to reload @wordpress/components.
As much as I don't quite like the current approach, I think it provides the best trade-offs. Both approaches would be kind of hacky, but I don't see any other way of reproducing the crash caused by rtlcss from an e2e test at the moment. See my other attempts here.
There was a problem hiding this comment.
Another thing is that there might be a possibility of a false-positive if the error is caught by the global error handler after the test finishes, which would mean the window._loadedWithoutErrors would still be true. While testing locally, the test consistently failed when I tested against v10.0.0 though. I didn't find a good way to wait for the error without it creating a bottleneck in the case the error doesn't happen.
…ge context Setting a variable to indicate an error happened is not needed as the uncaught error on the page will case the test to fail. Also, the additional message added by having the expect was not really adding much value.
|
Thanks @fullofcaffeine, it's great to see an e2e that actually manages to catch that issue! That said, I agree that the way we have to remove and re-add At this point, I think it'd be worth trying out to actually change the RTL test to install and activate an actual RTL language (for all the checks it runs). Let's maybe explore that in a different PR. Would you like to give that a try, or should I do that? 😄 |
|
Oh, there's also this plugin that I just learned about: https://wordpress.org/plugins/rtl-tester/ If we fail to make the e2e test install an RTL language, we might try to make it install that plugin instead 🤔 |
We can even take this a bit further: Turns out we already maintain a few helper plugins for e2e tests in Gutenberg, see https://github.com/WordPress/gutenberg/tree/6449773e1340d38c2eb4e0555677df54df531040/packages/e2e-tests/plugins Those are mostly used by the tests in https://github.com/WordPress/gutenberg/tree/6449773e1340d38c2eb4e0555677df54df531040/packages/e2e-tests/specs/editor/plugins. The relevant logic in the RTL tester plugin is fairly simple: https://github.com/yoavf/RTL-Tester/blob/0f53a0468a70a3af4dbb736703481e9921d7caea/rtl-tester.php#L87-L91 So let's maybe just copy that code into a newly added |
|
Follow up: #29598. |
fullofcaffeine
deleted the
add/regression-test-rtlcss-parser-comment-crash
branch
Description
Issue: #29250.
Add regression test to prevent issues related to RTL being active at the WP-admin level.*
*it seems you can activate RTL in two places: wp-admin and in the Gutenberg editor. The error we're trying to avoid a regression for only happens when RTL is active at the "WP admin level" (someone please correct me if this is the wrong mental model / I'm using the wrong term here).
After investigating where
rtlcssis parsing the CSS (see: https://github.com/ItsJonQ/g2/blob/main/packages/create-styles/src/create-compiler/plugins/rtl.js#L38), the challenge was then making sure thatdocument.dir = 'rtl'ran before that. Played with several approaches, including using Puppeteer'sevaluateOnNewDocumentto execute the snippet, but none of them worked.Had to do some gymnastics to make sure the snippet ran before the corresponding js. With that test, I am able to reproduce the error in GB <= 10.0.0. Still need to clean it up, and it's less than perfect (it effectively reloads the script after the page already loaded, while making sure the dir RTL property is set in the document) but it seems this approach is feasible.
How to test
v10.0.0(or revert commit2f93c432a3), thennpm ci && npm run buildand start a wp-env instance from there. Finally, in another Gutenberg copy, check out this branch (add/regression-test-rtlcss-parser-comment-crash) and run this test (you might also callonlyin theitto restrict the run to the single test added in this PR, if you'd like). Theloads editor without errors when RTL is set at the WP admin leveltest should fail 🔴:v10.0.0checkout, checkoutv10.0.2(ormaster) instead, runnpm ci && npm run buildand go back to the other Gutenberg instance, and run the test again. It should pass 🟢:Types of changes
New E2E test.
Checklist: