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

[iangmaia]

Description

This PR depends on Automattic/a8c-ci-toolkit-buildkite-plugin#148.

This PR updates the UI Test jobs to use the result of the command pr_changed_files from a8c-ci-toolkit-buildkite-plugin to decide if tests should run or not.

[dangermattic]

	1 Message
📖	This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

[wpmobilebot]

📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS
	Build Number	pr15086-39b07ac
	Version	21.7
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	39b07ac
	App Center Build	WooCommerce - Prototype Builds #12935

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

[AliSoftware]

Wait does this actually work using this syntax?

The .sh just calls pr_changed_files …, which always exit 0 and just prints "true" or "false"… so how would using a condition like if … >/dev/null 2>&1 detect the true/false case then?

Shouldn't you instead just compare the output of pr_changed_files to == "true" for this to work properly?
Either by updating the implementation of should-skip-ui-tests.sh to actually exit 0 or 1 depending on the output of pr_changed_files, or by keeping the helper unchanged and doing the test in this if here?

PS: I actually noticed this inconsistency while reading your P2 post announcing pr_changed_files before even opening this PR to comment in this diff here; so you might want to update the P2 too.

[iangmaia]

Oof, thanks a lot! When I returned to this PR and it seemed to be working I didn't get back to the details again.

I've now kept the output check in should-skip-ui-tests.sh so that callers can use the return code. Updated on cf8f8e1.

[AliSoftware]

🎗️ Reminder to revert the changes on this file before undrafting this PR and merging it, since those are only for temporary debugging purposes.

[AliSoftware]

Looks like this trick that I suggested in a previous review of using tee >(buildkite-agent annotate) doesn't work after all? 🤔 At least I don't see an annotation being added on the Buildkite build, despite the UI tests being skipped…

Again, once you end up finding how to fix that, you should not forget to update the P2 post accordingly to provide up-to-date examples / instructions for other projects to copy / use as inspiration 😅

Also, for the particular case of WooCommerce, the run-ui-tests.sh command is called by 2 different steps / jobs ("UI Tests (iPhone)" and "UI Tests (iPad)"). Which means that we should probably use a custom --context for the annotation so that the different jobs can do different annotations instead of one replacing the other.

[AliSoftware]

I'm still puzzled why the tee >(buildkite-agent annotate) trick wouldn't work.

If I test this with a fake executable for buildkite-agent, like this dummy script below:

$ cat ./fake-bk-agent
#/!bin/bash

if [ "$1" = "annotate" ]; then
    read -r -d '' BODY || true # read stdin
    echo "Annotation: $BODY" >annotation.txt
else
    echo "Unknown command: $1"
    exit 1
fi
# chmod +x ./fake-bk-agent

Then testing it works as expected 🤔

$ echo "Hello" | tee >(fake-bk-agent annotate)
Hello
$ cat annotation.txt
Annotation: Hello

So I'm not sure why it didn't work with the real | tee >(buildkite-agent annotate) 🤔

[iangmaia]

For now I broke it into separate statements on 379c4c7 -- not sure why tee didn't work though?

I've updated the P2 accordingly (will keep it in sync if other changes come along).

[AliSoftware]

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?

[iangmaia]

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.

[AliSoftware]

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)

[AliSoftware]

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?

[AliSoftware]

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?

[iangmaia]

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

[iangmaia]

@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 😅