ci: Fix workflow to update screenshots #4993
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The initial commit was untested, but having the workflow in the main branch allowed me to iterate on the workflow and test it in a branch. The following changes were made: - Standardize how workflow inputs are read (using inputs, which works for all triggers, instead of github.event.inputs, which only works for some triggers) - Improve the description of test_filter in selenium-lab-tests. - Add missing fields in the workflow_call trigger of selenium-lab-tests.yaml. (Unfortunately, the two triggers need to have the inputs defined separately, and with slightly different fields.) - Log the chosen ref in selenium-lab-tests, to help debug inputs. - Fix variable expansion in selenium-lab-tests so that test_filter can contain spaces. - Fix ignore_test_status, which did not work at all and broken the nightly tests last night. - Fix dependencies between jobs in update-screenshots. - Do not set status for the subordinate selenium-lab-tests job if started from update-screenshots. - Always upload screenshots from selenium-lab-tests if started from update-screenshots (normally only uploaded on failure). - Set the PR status from upload-screenshots. - Fix unpacking of screenshots from artifacts in upload-screenshots. - Fix updating of changed screenshots. - Fix handling of a lack of screenshot changes. - Split up the jobs for easier debugging in upload-screenshots. Still to do: - Fix setting of final PR status - Use a personal access token so that tests can be triggered from the PR push
joeyparrish
requested review from
littlespex,
avelad,
JulianDomingo and
theodab
|
Incremental code coverage: No instrumented code was changed. |
avelad
previously approved these changes
Feb 11, 2023
theodab
previously requested changes
Feb 17, 2023
| @@ -269,7 +290,7 @@ jobs: | |||
| # Upload new screenshots and diffs on failure; ignore if missing | |||
| - name: Upload screenshots | |||
| uses: actions/upload-artifact@v3 | |||
| if: ${{ failure() }} | |||
| if: ${{ failure() || inputs.ignore_test_status }} | |||
There was a problem hiding this comment.
The description of ignore_test_status says "If true, ignore test failures and treat the workflow as a success"
However, this seems to be treating the workflow as a failure. Is the description inaccurate?
There was a problem hiding this comment.
This means the job runs on failure, or if ignore_test_status is true. Normally, we would only upload screenshot artifacts if the tests failed. But if we're running the workflow specifically to get the screenshots updated, we upload them no matter what.
There was a problem hiding this comment.
Comment updated. ignore_test_status modifies many things that normally depend on test status.
| @@ -197,25 +204,28 @@ jobs: | |||
| # the container. | |||
| - name: Test Player | |||
| run: | | |||
| # Use of an array keeps elements in tact, and allows an element to | |||
There was a problem hiding this comment.
"in tact" -> "intact"
Unless you're saying that using an array makes the elements remain tactful and sensitive.
| ref: refs/pull/${{ github.event.inputs.pr }}/head | ||
| ref: refs/pull/${{ inputs.pr }}/head | ||
|
|
||
| - name: Set commit status to pending |
There was a problem hiding this comment.
Say, the first two steps of this workflow are the exact same as the steps of set-pending-status.
This needs run-lab-tests, which needs set-pending-status.
So shouldn't set-pending-status have already been run by the time this is run?
There was a problem hiding this comment.
No, those are separate jobs. Each job runs with a clean slate. You're thinking of "steps" within jobs. Calling another workflow (run-lab-tests) requires its own job. So the jobs are: 1. Set pending status, 2. run test workflow, 3. update PR, 4. set final status
avelad
approved these changes
Feb 23, 2023
joeyparrish
added a commit
that referenced
this pull request
Mar 1, 2023
The initial commit was untested, but having the workflow in the main branch allowed me to iterate on the workflow and test it in a branch. The following changes were made: - Standardize how workflow inputs are read (using inputs, which works for all triggers, instead of github.event.inputs, which only works for some triggers) - Improve the description of test_filter in selenium-lab-tests. - Add missing fields in the workflow_call trigger of selenium-lab-tests.yaml. (Unfortunately, the two triggers need to have the inputs defined separately, and with slightly different fields.) - Log the chosen ref in selenium-lab-tests, to help debug inputs. - Fix variable expansion in selenium-lab-tests so that test_filter can contain spaces. - Fix ignore_test_status, which did not work at all and broken the nightly tests last night. - Fix dependencies between jobs in update-screenshots. - Do not set status for the subordinate selenium-lab-tests job if started from update-screenshots. - Always upload screenshots from selenium-lab-tests if started from update-screenshots (normally only uploaded on failure). - Set the PR status from upload-screenshots. - Fix unpacking of screenshots from artifacts in upload-screenshots. - Fix updating of changed screenshots. - Fix handling of a lack of screenshot changes. - Split up the jobs for easier debugging in upload-screenshots. Still to do: - Fix setting of final PR status - Use a personal access token so that tests can be triggered from the PR push
joeyparrish
added a commit
that referenced
this pull request
Mar 1, 2023
The initial commit was untested, but having the workflow in the main branch allowed me to iterate on the workflow and test it in a branch. The following changes were made: - Standardize how workflow inputs are read (using inputs, which works for all triggers, instead of github.event.inputs, which only works for some triggers) - Improve the description of test_filter in selenium-lab-tests. - Add missing fields in the workflow_call trigger of selenium-lab-tests.yaml. (Unfortunately, the two triggers need to have the inputs defined separately, and with slightly different fields.) - Log the chosen ref in selenium-lab-tests, to help debug inputs. - Fix variable expansion in selenium-lab-tests so that test_filter can contain spaces. - Fix ignore_test_status, which did not work at all and broken the nightly tests last night. - Fix dependencies between jobs in update-screenshots. - Do not set status for the subordinate selenium-lab-tests job if started from update-screenshots. - Always upload screenshots from selenium-lab-tests if started from update-screenshots (normally only uploaded on failure). - Set the PR status from upload-screenshots. - Fix unpacking of screenshots from artifacts in upload-screenshots. - Fix updating of changed screenshots. - Fix handling of a lack of screenshot changes. - Split up the jobs for easier debugging in upload-screenshots. Still to do: - Fix setting of final PR status - Use a personal access token so that tests can be triggered from the PR push
joeyparrish
added a commit
that referenced
this pull request
Mar 1, 2023
The initial commit was untested, but having the workflow in the main branch allowed me to iterate on the workflow and test it in a branch. The following changes were made: - Standardize how workflow inputs are read (using inputs, which works for all triggers, instead of github.event.inputs, which only works for some triggers) - Improve the description of test_filter in selenium-lab-tests. - Add missing fields in the workflow_call trigger of selenium-lab-tests.yaml. (Unfortunately, the two triggers need to have the inputs defined separately, and with slightly different fields.) - Log the chosen ref in selenium-lab-tests, to help debug inputs. - Fix variable expansion in selenium-lab-tests so that test_filter can contain spaces. - Fix ignore_test_status, which did not work at all and broken the nightly tests last night. - Fix dependencies between jobs in update-screenshots. - Do not set status for the subordinate selenium-lab-tests job if started from update-screenshots. - Always upload screenshots from selenium-lab-tests if started from update-screenshots (normally only uploaded on failure). - Set the PR status from upload-screenshots. - Fix unpacking of screenshots from artifacts in upload-screenshots. - Fix updating of changed screenshots. - Fix handling of a lack of screenshot changes. - Split up the jobs for easier debugging in upload-screenshots. Still to do: - Fix setting of final PR status - Use a personal access token so that tests can be triggered from the PR push
joeyparrish
added a commit
that referenced
this pull request
Mar 2, 2023
The initial commit was untested, but having the workflow in the main branch allowed me to iterate on the workflow and test it in a branch. The following changes were made: - Standardize how workflow inputs are read (using inputs, which works for all triggers, instead of github.event.inputs, which only works for some triggers) - Improve the description of test_filter in selenium-lab-tests. - Add missing fields in the workflow_call trigger of selenium-lab-tests.yaml. (Unfortunately, the two triggers need to have the inputs defined separately, and with slightly different fields.) - Log the chosen ref in selenium-lab-tests, to help debug inputs. - Fix variable expansion in selenium-lab-tests so that test_filter can contain spaces. - Fix ignore_test_status, which did not work at all and broken the nightly tests last night. - Fix dependencies between jobs in update-screenshots. - Do not set status for the subordinate selenium-lab-tests job if started from update-screenshots. - Always upload screenshots from selenium-lab-tests if started from update-screenshots (normally only uploaded on failure). - Set the PR status from upload-screenshots. - Fix unpacking of screenshots from artifacts in upload-screenshots. - Fix updating of changed screenshots. - Fix handling of a lack of screenshot changes. - Split up the jobs for easier debugging in upload-screenshots. Still to do: - Fix setting of final PR status - Use a personal access token so that tests can be triggered from the PR push
github-actions
bot
added
the
status: archived
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The initial commit was untested, but having the workflow in the main branch allowed me to iterate on the workflow and test it in a branch.
The following changes were made:
Still to do: