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.

Sign up for GitHub

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

[Tooling] Skip UI Tests if there are no relevant changes #15086

Draft
wants to merge 10 commits into
base: trunk
Choose a base branch
from
+15 −7
Draft

[Tooling] Skip UI Tests if there are no relevant changes #15086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
195158c
Skip running UI tests if there are no relevant changes
iangmaia Feb 6, 2025
d38ed58
[to revert] Test running on this branch
iangmaia Feb 6, 2025
4b9ddfe
Install CI_TOOLKIT before running pr_changed_files
iangmaia Feb 7, 2025
b3bb96b
Move call to `pr_changed_files` to a separate file
iangmaia Feb 7, 2025
ce6ea77
Annotate Buildkite build when UITests were skipped
iangmaia Feb 12, 2025
3c1b30c
Update a8c-ci-toolkit version
iangmaia Feb 12, 2025
131f7b0
Adjust `pr_changed_files` usage
iangmaia Feb 12, 2025
168e65f
Split `buildkite-agent annotate` to a separate statement
iangmaia Feb 12, 2025
d73af98
[to revert] Add test code change to trigger ui tests
iangmaia Feb 12, 2025
e5a038d
Revert "[to revert] Add test code change to trigger ui tests"
iangmaia Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .buildkite/commands/run-ui-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
#!/bin/bash -eu

if .buildkite/commands/should-skip-ui-tests.sh; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AliSoftware what do you think of replicating this to .buildkite/commands/run-unit-tests.sh? I gather the conditions should be very similar if not identical. This crossed my mind when I saw it kept running for additional 12 minutes while only these files are changed.

And thinking about it... we should probably allow .buildkite/commands/* to trigger tests, but that of course would break our testing in this very PR 😅

message="Skipping UI Tests as only documentation, tooling and/or non-code files were changed"
echo "$message" | buildkite-agent annotate --style "info" --context "skip-ui-tests"
echo "$message"
exit 0
fi

TEST_NAME=$1
DEVICE=$2

Expand Down
5 changes: 5 additions & 0 deletions .buildkite/commands/should-skip-ui-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#!/bin/bash

# Check if changes are limited to documentation, tooling and non-code files
result=$(pr_changed_files --all-match "*.md" "docs/**" "*.txt" "*.pot" "fastlane/**" ".github/**" ".buildkite/**")
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be a bit too late for that… but now that we've discovered that our primary usage for pr_changed_files will mostly be directly inside the if of .buildkite/commands/*.sh scripts—as opposed to its output being stored in n env var from shared-pipeline-vars for the pipeline.yml to use in if: build.env(…) conditions—then maybe we should have added some flag to toggle output mode of the command after all…?

e.g:

  • By default: command exits with 0 and prints true or false in stdout.
  • With -e flag: command don't print true or false to stdout, and instead exit 0 on match and exit 1 on non-match
  • In case of bad usage error (not in PR, bad params, …), use negative exit code (-1, -2, …) or a large exit code (e.g. 255), to make those more easily differentiable from the true/false aspect of 0/1 codes when using -e.

Could be an easy-enough quick improvement iteration on the tool, to make it way easier to use at call side in the situations like in this PR? wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be a bit too late for that

Actually... do you think it's too late to make the 0 / 1 in match or non match the default mode instead?
I mean, this is something that's clearer now to be the default, so worth considering... -e could make it print true / false (the current behavior). I don't think we have a lot of users yet 😅 but yeah, might be too much to squeeze in.

But otherwise doing as you suggest is also fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, exit 0/1 as the default and a flag to switch back to mode could actually make more sense. I think it's not too late for that (technically it'd be consider a breaking change so would need a major version bump on the plugin though, but 🤷 I'm ok with that)

Copy link
Contributor

Choose a reason for hiding this comment

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

PS: Not sure what the flag should be named btw. I suggested -e when I suggested the other way around to mean e as use [e]xit code… but if we go with the opposite approach and exit code being the new default, then the flag name should probably get inspired from either "print the result in stdout" or "to use in the context of a variable assignment". Maybe --stdout as a name for such a flag then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually it seems that X=$(echo "false"; false) still assigns the value false to X, despite the false command ultimately exiting with non-zero… At least as long as you don't have set -e enabled.

This means that if we end up making the default behavior of the command using the exit code by default… but also not print the result to stdout in that case, then there's a risk that someone would use RUN_UI_TESTS=$(pr_changed_files --all-match …) and not realizing they should use the --stdout flag in this case, which means in such case RUN_UI_TESTS will always be assigned the empty string… and if: build.env('RUN_UI_TESTS') will probably always evaluate to false regardless of the exit code that pr_changed_files returned… which could lead to surprises just because one would have forgotten to use --stdout when using it in the context of assigning a variable…

So maybe we should print the result to stdout in both scenarios (and the caller can be free to use if pr_changed_files --all-match *.md *.txt >/dev/null; then … if they want to hide that stdout) still, regardless of if we use the flag to tell it to always exit 0 (for usage in variable assignment) or if we don't and have it exit 1 on failure (for usage in if)…

… in which case we'd need to find another name for that flag than --stdout…

Or final idea, no flag, always print + use exit code both… and up to the caller to use RUN_UI_TESTS=$(pr_changed_files … || true) in their script (especially for cases where the script has set -e enabled) when assigning to a variable, and if pr_changed_files … >/dev/null; then… when using in a condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Toyed with this idea (exit codes AND output) here: Automattic/a8c-ci-toolkit-buildkite-plugin#151

[[ "$result" == "true" ]]
8 changes: 2 additions & 6 deletions .buildkite/pipeline.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,7 @@ steps:
#################
- label: ":microscope: UI Tests (iPhone)"
command: .buildkite/commands/run-ui-tests.sh UITests 'iPhone 16'
depends_on: build
# Only run on `trunk` and `release/*` -- See p91TBi-cBM-p2#comment-13736
if: build.branch == 'trunk' || build.branch =~ /^release\//
if: build.branch == 'trunk' || build.branch =~ /^release\// || build.branch =~ /^iangmaia\//
plugins: [$CI_TOOLKIT]
artifact_paths:
- fastlane/test_output/*
Expand All @@ -128,9 +126,7 @@ steps:

- label: ":microscope: UI Tests (iPad)"
command: .buildkite/commands/run-ui-tests.sh UITests "iPad (10th generation)"
depends_on: build
# Only run on `trunk` and `release/*` -- See p91TBi-cBM-p2#comment-13736
if: build.branch == 'trunk' || build.branch =~ /^release\//
if: build.branch == 'trunk' || build.branch =~ /^release\// || build.branch =~ /^iangmaia\//
plugins: [$CI_TOOLKIT]
artifact_paths:
- fastlane/test_output/*
Expand Down
2 changes: 1 addition & 1 deletion .buildkite/shared-pipeline-vars
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
# to set up some variables that will be interpolated in the `.yml` pipeline before uploading it.

XCODE_VERSION=$(sed 's/^~> *//' .xcode-version)
CI_TOOLKIT_PLUGIN_VERSION="3.9.1"
CI_TOOLKIT_PLUGIN_VERSION="3.11.0"

# Note: `-v4` suffix was added to use xcode-16.1-v4 image; remember to remove that suffix during the next Xcode update
export IMAGE_ID="xcode-$XCODE_VERSION-v4"
Expand Down