Skip to content
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

Allow running of "Smoke test release" on draft releases #36997

Merged
merged 19 commits into from Mar 8, 2023

Conversation

rodelgc
Copy link
Contributor

@rodelgc rodelgc commented Feb 28, 2023

All Submissions:

Changes proposed in this Pull Request:

Closes #36998.

This PR would allow the Smoke test release workflow to be run on a draft release. Also, it contains a few several fixes in the workflow file.

The main reason why the Smoke test release workflow previously couldn't run on draft releases was because it's using the default github.token when retrieving the list of releases. Since this token has limited permissions, it can only access releases/pre-releases that are already published.

To fix this, the Smoke test release workflow now uses the same E2E_GH_TOKEN used in other workflows. With this token, draft releases can already be accessed.

How to test the changes in this Pull Request:

  1. Create a draft release. Make sure it has:
    1. A woocommerce.zip asset, and
    2. A temporary tag for testing purposes, like test-pr-36997.
  2. Go to the Smoke test release page.
  3. In the "Use workflow from" dropdown, select this branch e2e/release-include-drafts.
  4. In the "WooCommerce Release Tag" box, enter the tag you created in step 1.
  5. Click "Run workflow" to manually start it.
  6. Wait for it to finish. All jobs should be executed like so:
    yAS4WTxv76
  7. Go to WooCommerce Test Reports > Smoke Tests on Releases page.
  8. You should see the tag listed in the page, and that the API and E2E test results are displayed, regardless of whether they passed or failed.
    Mp0drcndSH
    If you're taken to a 404 page, chances are the reports are still being uploaded. Wait for a few more minutes, then refresh the page.
  9. Delete the draft release after testing.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you created a changelog file for each project being changed, ie pnpm --filter=<project> changelog add?
  • Have you included testing instructions?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Feb 28, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 28, 2023

Test Results Summary

Commit SHA: 718ef83

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 57s
E2E Tests189006019516m 20s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

@codecov
Copy link

codecov bot commented Feb 28, 2023

Codecov Report

Merging #36997 (718ef83) into trunk (7c76118) will decrease coverage by 0.0%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             trunk   #36997     +/-   ##
==========================================
- Coverage     46.7%    46.7%   -0.0%     
  Complexity   17188    17188             
==========================================
  Files          429      429             
  Lines        64820    64821      +1     
==========================================
- Hits         30253    30251      -2     
- Misses       34567    34570      +3     
Impacted Files Coverage Δ
plugins/woocommerce/includes/class-wc-tax.php 78.6% <0.0%> (-0.5%) ⬇️
...ugins/woocommerce/includes/class-wc-post-types.php 2.9% <0.0%> (-<0.1%) ⬇️

@rodelgc rodelgc changed the title E2e/release include drafts Run "Smoke test release" on draft releases Mar 2, 2023
@rodelgc rodelgc changed the title Run "Smoke test release" on draft releases Allow running of "Smoke test release" on draft releases Mar 2, 2023
types: [published]
types: [released, prereleased]
Copy link
Contributor Author

@rodelgc rodelgc Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Fix]

I noticed in the 7.4.1 and 7.5.0-beta.2 releases that this workflow wasn't triggered after they were made public. I figured it's because these releases may have already been published, and were just updated to be a pre-release or a release later on. Therefore, the published release type wasn't triggered because they were already in a published state to begin with.

Setting the release type to released and prereleased I think would be more appropriate. But, is it, @woocommerce/atlas? Thanks 😃

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured it's because these releases may have already been published, and were just updated to be a pre-release or a release later on.

I'm not sure that this is the case, as I always make sure to select one of those two before publishing a release... but if released and prereleased trigger this workflow as expected, I think this should be okay.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw this in the release event documentation:

Note: The prereleased type will not trigger for pre-releases published from draft releases, but the published type will trigger. If you want a workflow to run when stable and pre-releases publish, subscribe to published instead of released and prereleased.

Might this conflict with this change? 🤔

group: ${{ github.workflow }}-${{ github.ref }}
group: ${{ github.workflow }}-${{ github.event.release.tag_name || inputs.tag }}
Copy link
Contributor Author

@rodelgc rodelgc Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Fix]

I noticed that the concurrency group was erroneous because the main intention of this workflow is to group the release smoke tests based on the release version (tag) they're testing, not based on github.ref. Leaving the concurrency group to use github.ref will cause a smoke test for release 7.5.0, for example, to unexpectedly cancel the smoke test of another release version.

I fixed it here so that the release smoke tests are grouped by release tag, (whether this tag came from the release or workflow_dispatch event) thereby preventing concurrent release smoke tests on different tags/versions from cancelling each other.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! 🙌

created: ${{ steps.created-at.outputs.created }}
steps:
- name: Validate tag
if: ${{ github.event_name == 'workflow_dispatch' }}
env:
GH_TOKEN: ${{ github.token }}
run: gh release view "${{ github.event.inputs.tag }}" --repo=woocommerce/woocommerce
GH_TOKEN: ${{ secrets.E2E_GH_TOKEN }}
Copy link
Contributor Author

@rodelgc rodelgc Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted in the PR description, I replaced the default github.token with E2E_GH_TOKEN. You would see similar diffs like this throughout this PR.

Comment on lines -35 to +42
- name: Get tag
uses: actions/github-script@v6
id: tag
with:
result-encoding: string
script: |
console.log( "${{ github.event_name }}" );
return "${{ github.event.release.tag_name }}" || "${{ github.event.inputs.tag }}"
- name: Get tag from triggered event
id: get-tag
env:
RELEASE_TAG: ${{ github.event.release.tag_name || inputs.tag }}
run: |
echo "Triggered event: ${{ github.event_name }}"
echo "Tag from event: $RELEASE_TAG"
echo "tag=$RELEASE_TAG" >> $GITHUB_OUTPUT
Copy link
Contributor Author

@rodelgc rodelgc Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Fix]

The return line was erroneous because the || should be inside ${{ }}. Fixed it here by doing just that. Also removed the use of the actions/github-script action in order to simplify the logic of this step and make it more readable (I hope 🤞) .

Comment on lines -109 to +110
uses: aws-actions/configure-aws-credentials@v1
uses: aws-actions/configure-aws-credentials@v1-node16
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Fix]

This fixes the node v12 deprecation messages.

Comment on lines +51 to +53
if ( GITHUB_TOKEN ) {
requestConfig.headers.Authorization = `Bearer ${ GITHUB_TOKEN }`;
}
Copy link
Contributor Author

@rodelgc rodelgc Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in this update-woocommerce.spec.js file centers around this particular line. This makes sure that the supplied token will be used when downloading the woocommerce.zip file from a draft release.

@rodelgc rodelgc marked this pull request as ready for review March 2, 2023 06:56
@rodelgc rodelgc requested review from a team and jonathansadowski and removed request for a team March 2, 2023 06:56
@rodelgc rodelgc added the focus: smoke tests These Issues/PRs relate to smoke testing. label Mar 2, 2023
Copy link
Contributor

@jonathansadowski jonathansadowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 nice job. I verified that this selected the correct asset with a draft release that I created. The smoke tests themselves failed for various reasons unrelated to this PR (such as my tag name, 7.4.1-test not matching the version from the asset I created, 7.4.1), but I believe that's expected given the package that I used.

types: [published]
types: [released, prereleased]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured it's because these releases may have already been published, and were just updated to be a pre-release or a release later on.

I'm not sure that this is the case, as I always make sure to select one of those two before publishing a release... but if released and prereleased trigger this workflow as expected, I think this should be okay.

Copy link
Contributor

@nigeljamesstevenson nigeljamesstevenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on Jonathan's validation, this is looking good!

Copy link
Contributor

@alopezari alopezari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me @rodelgc!

Based on this change, it would be great if we could confirm that smoke tests also run on the following scenarios:

  • Release is directly published, without any draft previous state.
  • Draft release is published, in other words, a draft release is created first (so smoke tests would be run considering the new changes from this PR), but once we remove the draft state and it becomes an actual release, smoke tests should be run again if I'm not wrong).

This way we would cover potential regression issues related to the event triggers change.

types: [published]
types: [released, prereleased]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw this in the release event documentation:

Note: The prereleased type will not trigger for pre-releases published from draft releases, but the published type will trigger. If you want a workflow to run when stable and pre-releases publish, subscribe to published instead of released and prereleased.

Might this conflict with this change? 🤔

group: ${{ github.workflow }}-${{ github.ref }}
group: ${{ github.workflow }}-${{ github.event.release.tag_name || inputs.tag }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! 🙌

@rodelgc
Copy link
Contributor Author

rodelgc commented Mar 6, 2023

I saw this in the release event documentation:

Note: The prereleased type will not trigger for pre-releases published from draft releases, but the published type will trigger. If you want a workflow to run when stable and pre-releases publish, subscribe to published instead of released and prereleased.

Might this conflict with this change? 🤔

It actually did @alopezari! Thanks for bringing this up. When I tested it in a private repo, the workflow indeed wasn't triggered when I saved the release as a draft first, then published it as a pre-release.

To fix this, I pushed 718ef83 so that the release types are now [released, prereleased, published]. When I tested these 3 release types on a private repo, the workflow was triggered as expected on these actions:

  • Save a draft first, then published as a Prerelease
  • Save a draft first, then published without checking any of the checkboxes in the GitHub UI
  • Save a draft first, then published as a Latest release
  • Published as a Prerelease directly, didn't create a draft
  • Published directly without checking any checkboxes, didn't create a draft
  • Published directly as a Latest release, didn't create a draft
  • Start with an already published release, then update it to a Prerelease
  • Start with an already published pre-release, then update it to a Latest release
  • Start with an already published pre-release, then update it to a release (without checking Latest)

Workflow wasn't triggered on the scenario where an already published non-latest release was changed to Latest, and vice-versa, and I think this is fine because there's really no need to re-run the tests when this happens.

Ready again for another review.

Copy link
Contributor

@alopezari alopezari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Save a draft first, then published as a Prerelease
Save a draft first, then published without checking any of the checkboxes in the GitHub UI
Save a draft first, then published as a Latest release
Published as a Prerelease directly, didn't create a draft
Published directly without checking any checkboxes, didn't create a draft
Published directly as a Latest release, didn't create a draft
Start with an already published release, then update it to a Prerelease
Start with an already published pre-release, then update it to a Latest release
Start with an already published pre-release, then update it to a release (without checking Latest)

Thanks for such extensive testing of this change @rodelgc!

I'm approving this PR 👍

@alopezari
Copy link
Contributor

If @jonathansadowski & @nigeljamesstevenson are happy with the latest change, I'm happy to merge this 👍

@nigeljamesstevenson
Copy link
Contributor

@alopezari - Great spot on noticing:

The prereleased type will not trigger for pre-releases published from draft releases

and amazing testing all the combinations @rodelgc ! - This is looking great 😊

@alopezari
Copy link
Contributor

Awesome, thanks @nigeljamesstevenson! I'll merge this 👍

@alopezari alopezari merged commit b60cc12 into trunk Mar 8, 2023
19 of 20 checks passed
@alopezari alopezari deleted the e2e/release-include-drafts branch March 8, 2023 09:23
@github-actions github-actions bot added this to the 7.6.0 milestone Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus: smoke tests These Issues/PRs relate to smoke testing. plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement]: In the Smoke test release workflow, support testing WooCommerce Zips in draft releases
4 participants