-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
#62221 Test current branch upgrade #8252
base: trunk
Are you sure you want to change the base?
#62221 Test current branch upgrade #8252
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Thanks for the ping @johnbillion. While I'm not the best person to review this proposal, I just wanted to ask a question about the "a new workflow which tests the upgrade process from the three latest major versions to the current branch" part: if this proposal is accepted, does it mean that we'll have to manually bump the related versions on each major release? This is not a blocker, but definitely something that will need some specific documentation on the major release handbook page :) |
@audrasjb Yes that's correct. We already have that in several other workflows, eg the performance tests, old branch tests, and upgrade tests. I'll make a note to get the major release handbook page udpated. |
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 looks good to me.
I did a test with a fatal error in wp-settings in another PR and the tests failed as expected, see this action run.
// Hello | ||
|
||
/** |
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.
// Hello | |
/** | |
/** |
Should be removed once the testing done. Right?
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.
Yep! Thanks.
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 don't love adding a separate workflow file for this when we already have an upgrade testing workflow in upgrade-testing.yml
.
I'm trying to think through how to consolidate this. I think ideally it would look something like:
- When pull requests change the related workflow files, everything runs (current branch and the usual expanded matrix).
- When pull requests do not change the related workflow files, run the new combinations being added here.
- When manually run by supplying the
new-version
input, only the preexisting combinations run (not using a zip of current branch).
I think this could be accomplished with some conditional checks, but we'd likely need to add a 3rd-party action like changed-files to know when to run each job.
My only other comment is that we already generate a ZIP of the code in the current branch during the build test workflow. However, that doesn't run for every commit (only when files change that affect the build process or something that could be tested in Playground. It would be nice to have one canonical ZIP of the branch for all workflows to use, but I recognize that's not straightforward.
I wish that path filtering could be applied for workflow_run
events. That could make it easier to only run a follow up workflow when desired.
- name: Upload ZIP as a GitHub Actions artifact | ||
uses: actions/upload-artifact@65c4c4a1ddee5b72f698fdd19549f0f0fb45cf08 # v4.6.0 | ||
with: | ||
name: wordpress-develop |
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.
The default behavior of upload-artifact is to not overwrite if an artifact with the same already exists. It seems that each pull request will have a unique artifact with the same name, but I'm not sure of subsequent runs in the same PR and whether the overwrite: true
input needs to be configured.
It also may be worth including at a minimum github.head_ref
in the name, and ${{ format( '{0}-{1}', github.head_ref, github.sha ) }}
at a maximum. If these files are ever downloaded locally, it could get confusing with many develop.zip
files.
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.
Artifacts are scoped to their workflow run ID, so there's no need to add additional context to the name to keep them separate from other runs or further commits to the same PR. There are other workflows in wordpress-develop that include more context in artifact names because the artifacts can be used in subsequent workflow runs (eg. the build process tests), but this build artifact is only used within the same workflow run.
Yeah, if you take a look at the first few commits in this branch you'll see that I was initially trying to use that build, but it requires using the
I understand your point but I'm not overly keen on adding further third party actions. Is a bunch of conditionals and an extra third party action better than an additional workflow? I don't think there's much between them. |
Thanks, @johnbillion. I think this is a good idea and things are looking good to me. Just to confirm one of my assumptions here, in addition to needing to bump the WP versions in the matrix passed for the reusable upgrade testing workflow, we'll also need to make sure the matrix stays up to date with the supported PHP and DB versions as that support changes over time, correct? I also notice that the current matrix doesn't include any object cache tests. Is that intentional or do you think there would be some value in including at least one object cache test in the matrix? |
@joemcgill That's correct. We have that case already with the other upgrade tests. It's unfortunate, and perhaps going forward we could populate the matrix from Good spot on the object caching. Looking at the Installation Tests workflow there's a matrix entry for Adding testing with and without memcached would require a change to the way these installation and upgrade workflow files work because they all use PHP directly on the host runner instead of using the containers. I'll open a follow-up ticket. |
This introduces a new workflow which tests the upgrade process from the three latest major versions to the current branch, which is built and zipped up and used by WP-CLI for the update.
The main benefit of this is that when a PR is opened, the upgrade process can be tested with the branch in the PR. This complements the full "Upgrade Tests" workflow which only runs once a release has been published. This will help increase the confidence of the introduction of checks for required PHP extensions in the upgrade routine and the corresponding
hash
extension requirement in #8138.This uses a smaller set of matrix values than the "Upgrade Tests" workflow. I think this is sufficient for now, it can always be expanded at a later date if necessary.
Workflow runs can be seen here: https://github.com/WordPress/wordpress-develop/actions/workflows/upgrade-develop-testing.yml
Trac ticket: https://core.trac.wordpress.org/ticket/56017
Trac ticket: https://core.trac.wordpress.org/ticket/62221
Trac ticket: https://core.trac.wordpress.org/ticket/62815
Note: This PR comments out the triggers for a bunch of other workflows just so they don't unnecessarily run in this PR. Those changes won't be committed.
@todo