-
Notifications
You must be signed in to change notification settings - Fork 821
Boost: Re-enable Critical CSS and Concatenate E2E tests #44105
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
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Boost plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
- Introduced a new plugin `e2e-critical-css-force-errors` to simulate errors in Critical CSS Advanced Recommendations. - Updated e2e tests to activate the new plugin during the Critical CSS module tests. - Added the new plugin to the Docker configuration for proper testing environment setup.
… the 'Regenerate' button. This improves the test coverage for user interactions within the Critical CSS advanced recommendations.
…per activation/deactivation of the e2e-critical-css-force-errors plugin in tests. This enhances the reliability of the Critical CSS module tests.
…s and ensure visibility of critical CSS meta information. This streamlines the test flow and focuses on essential assertions.
@@ -103,7 +103,7 @@ export default class JetpackBoostPage extends WpPage { | |||
|
|||
async waitForCriticalCssMetaInfoVisibility() { | |||
const selector = '[data-testid="critical-css-meta"]'; | |||
return this.waitForElementToBeVisible( selector, 3 * 60 * 1000 ); | |||
return this.waitForElementToBeVisible( selector, 4 * 60 * 1000 ); |
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.
This function is executed when waiting for the local Critical CSS generation to finish. I've given it an extra minute in case the local instance cannot finish generating within 3 minutes.
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 hope that's enough...
Out of scope for this, just thoughts.
Seeing this made me wonder if we shouldn't just limit the generation to the home page. After all, we just want to see that it actually works. We don't want to generate for the whole website...
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.
That's a great idea, I might discuss this with you offline regarding the lift of this. Or create a Linear issue for Triage for this if waiting for the Critical CSS continues to be flaky.
I'm still seeing a lot of timeouts for me. I believe I saw some chatter about localtunnel vs other solutions. I'm curious if we need to look at that layer. Let me know if you need my logs. |
A few React imports managed to get incorrectly switched to types rather than full imports. This should fix them.
@kraftbj
If this works for you, then I think we should look at potentially recommending another service/ngrok over localtunnel to run the tests locally (potentially even remotely to speed up builds). |
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 still find it weird that tests are flaky... Considering this is local critical css generation, even on local tunnel things should happen fast since it's localhost when it's running the process...
I really hope the changes you made finally address the flakiness 🤞🏻
@@ -103,7 +103,7 @@ export default class JetpackBoostPage extends WpPage { | |||
|
|||
async waitForCriticalCssMetaInfoVisibility() { | |||
const selector = '[data-testid="critical-css-meta"]'; | |||
return this.waitForElementToBeVisible( selector, 3 * 60 * 1000 ); | |||
return this.waitForElementToBeVisible( selector, 4 * 60 * 1000 ); |
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 hope that's enough...
Out of scope for this, just thoughts.
Seeing this made me wonder if we shouldn't just limit the generation to the home page. After all, we just want to see that it actually works. We don't want to generate for the whole website...
Addresses HOG-188: Enable Critical CSS E2E tests
Proposed changes:
This PR re-enables 2 out of 7 Jetpack Boost e2e test suites that were temporarily disabled in commit 0eb34d93ca7522549dcec7ea35c09d4f39ef5406 due to flakiness:
Re-enabled test suites:
specs/critical-css
)specs/concatenate
)Test suites still disabled (to be addressed in separate PRs):
specs/base
)specs/modules
)specs/page-cache
)specs/image-cdn
)specs/image-guide
)Jetpack product discussion
N/A
Does this pull request change what data or activity we track or use?
No
How this PR resolves the failing tests:
1. Extended timeout for Critical CSS generation
2. Added error simulation plugin
e2e-critical-css-force-errors.php
plugin to simulate Critical CSS generation errors3. Improved test flow and assertions
4. Removed temporary test.skip() calls
Other information:
Testing instructions:
From the
projects/plugins/boost/tests/e2e
directory:Via ngrok (Recommended, but requires a subscription)
Replace
$tunnel_url
with your actual ngrok URLInstall E2E test dependencies:
Start the Docker environment and tunnel:
Start your ngrok tunnel
ngrok http --url=$tunnel_url http://localhost:8889/
Run the specific re-enabled test suites:
Via localtunnel (Not recommended as it may be slow)
Install E2E test dependencies:
Start the Docker environment and tunnel:
pnpm run env:up && pnpm run tunnel:up
Run the specific re-enabled test suites:
Expected behavior:
e2e-critical-css-force-errors.php
plugin should be properly activated/deactivated during teststest.skip()
calls should remain in the re-enabled test filesVerification steps:
Check that tests are running in CI:
Verify error simulation works:
Test stability: