-
Notifications
You must be signed in to change notification settings - Fork 808
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
E2E Tests: Add Scheduled job for a Pre-Release Gutenberg block tests #19132
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 🤖 Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
|
.github/workflows/e2e-tests.yml
Outdated
working-directory: projects/plugins/jetpack/tests/e2e | ||
run: yarn test-e2e --testNamePattern=blocks | ||
|
||
- name: Scheduled run blocks tests with pre-release Gutenberg |
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 approach is kinda messy. We would end up with 3 runs for both push
and schedule
events. All of the "skipped" jobs would build Jetpack and upload artifacts anyway.
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.
You could do like I did for tests.yml: One lightweight job that produces the matrix, then the matrix job depends on that to use the generated matrix. Too bad GH doesn't have any other way to DRY when you need all the same setup in several similar but not-quite-identical jobs.
Even better would be if the E2E setup and running could be closer to how the rest of the tests do it so we could run them from the same matrix. But that's probably longer term than this PR, as it would probably require dropping the use of wp-env.
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 wonder if we really need to run no/latest variation on every PR. With this new pre-release maybe we can only run the GB != bundled in a schedule, and have only a full suite run with the bundled GB in PRs checks. We could then have 2 different workflows.
- Optional we could make the PR workflow depend on the build one, and use the build artefacts as input, so we don't build twice. This will also remove all the PHP setup as we don't need it to run the E2E tests.
- The second workflow will be similar to the one we have now, with a matrix and a Jetpack build, but no event condition.
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.
How about collapsing these two steps into one, and merging their if
expressions into one?
- name: Run blocks tests with ${{ matrix.gutenberg }} Gutenberg
if: |
( github.event_name != 'schedule' && matrix.gutenberg == 'latest' ) ||
( github.event_name == 'schedule' && matrix.gutenberg == 'pre-release' )
working-directory: projects/plugins/jetpack/tests/e2e
run: yarn test-e2e --testNamePattern=blocks
This approach is kinda messy. We would end up with 3 runs for both
push
andschedule
events. All of the "skipped" jobs would build Jetpack and upload artifacts anyway.
You could build Jetpack in a separate job (without a matrix) and upload the build as an artifact, declare that the test job needs:
the build job, and download the artifact there. (We have some precedent for something like that in the Gutenberg repo: See how the create-release
job uses the artifact from the build
job.)
As for the uploading of the test result artifacts, you could try changing the step's if:
from ${{ always() }}
to success() || failure()
(to maybe skip it if the previous step is skipped?) Not sure if that's going to work, though.
As a further/later optimization, it's probably even possible to also absorb the "run all tests" step into this one (e.g. through the use of an env variable).
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 wonder if we really need to run no/latest variation on every PR. With this new pre-release maybe we can only run the GB != bundled in a schedule, and have only a full suite run with the bundled GB in PRs checks.
Yeah, I don't think we really need to run latest
GB tests on every PR. We probably can get rid of the matrix in the job altogether.
Moving the build step into a separate job is a good idea. I'll look into it in a follow up PR
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.
with the latest changes from @adimoldovan around E2E workflow, I made the matrix defined in runtime. During the scheduled run, an additional job would be triggered that would run block tests with the latest GB plugin.
.github/workflows/e2e-tests.yml
Outdated
working-directory: projects/plugins/jetpack/tests/e2e | ||
run: yarn test-e2e --testNamePattern=blocks | ||
|
||
- name: Scheduled run blocks tests with pre-release Gutenberg |
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.
You could do like I did for tests.yml: One lightweight job that produces the matrix, then the matrix job depends on that to use the generated matrix. Too bad GH doesn't have any other way to DRY when you need all the same setup in several similar but not-quite-identical jobs.
Even better would be if the E2E setup and running could be closer to how the rest of the tests do it so we could run them from the same matrix. But that's probably longer term than this PR, as it would probably require dropping the use of wp-env.
8ace25b
to
c7b4d4c
Compare
Here's another idea, let me know if it makes sense to you: What if we automatically trigger the E2E tests as soon as a new version of GB is released (in addition to triggering them every 12hr)? When it comes to GB Calypso E2E tests, we can't yet do this, because we still need to manually update the plugin in the SVN for edge sites. But here, the whole process of installing and activating the plugin is automatic. This would be useful to have the results as soon as possible after a given release, and would allow JP teams to act on them more readily. We (@Automattic/team-calypso) would also be following the test results on Slack, and try to coordinate the fixes as soon as humanly possible. Unfortunately, there doesn't seem to be a way to "listen" to releases from other repos using GH actions, but maybe we could use the cron strategy described here. cc @ockham |
@fullofcaffeine this is an interesting idea. Although, I don't think it is possible to implement solely within Jetpack's CI configuration. The problem here is that to identify a new release we need to compare it against something. For that, we would need to store the current release somewhere. With GH Actions we can't do that. I have a few possible solutions though:
|
In case you didn't see it yet, either of the "trigger a GH Action" methods would make use of https://docs.github.com/en/rest/reference/repos#create-a-repository-dispatch-event, which needs to be invoked using a token with the "repo" scope. As for storing data, you could store it in a gist. I see marketplace actions exist for reading and writing them. Or you could create a special branch that contains a metadata file instead of code. |
This was the culprit: WordPress/gutenberg#29847. This branch has |
Wrote too fast. Still can't get it to work, even after removing all images, updating from That being said, that seems to be wp-env-related. Assuming this works well in CI, then I'd say let's get this shipped and I'll help testing by watching the slack channel as new GB versions are shipped. |
8e81a7e
to
e1dceee
Compare
- uses: actions/checkout@v2 | ||
- id: create-matrix | ||
run: | | ||
MATRIX='[{"group":"pre-connection" },{"group":"connection" },{"group":"post-connection" }]' |
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.
Just to make sure the behaviour I understand from this code is the intended one:
- On schedule all tests will run
- On pull request the gutenberg tests (with plugin set up) will not run
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.
Yes, but I'd frame it like this:
- For pull requests, we run all the tests with bundled Gutenberg.
- For the scheduled job, we run all the tests with bundled Gutenberg PLUS blocks tests with the latest dev Gutenberg
…re-release-gb-job
This looks good to me and I say let's ship it. |
Thanks for working on this, @brbrr! ❤️ |
Changes proposed in this Pull Request:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
There is no easy way to test it, unfortunately. To properly test it:
GUTENBERG=latest ./bin/env.sh start
to make sure the plugin setup works as expected