Skip to content

Fix false positive Keystone and WordPressData tests in CI #24630

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

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Jun 26, 2025

The tests for Keystone and WordPressData used | xcbeautify but without set -o pipefail, meaning that if the test failed, the error would have been swallowed by the pipe. Example:

image

Case in point, this build where we can now see Keystone failing.

image

This PR addresses that. It also disables the Keystone tests from CI. Until the work in #24537 is completed, there's no point running them only to see them fail to build.

@mokagio mokagio added this to the 26.3 milestone Jun 26, 2025
@mokagio mokagio self-assigned this Jun 26, 2025
@mokagio mokagio added the Tooling Build, Release, and Validation Tools label Jun 26, 2025
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 26, 2025

App Icon📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress
ConfigurationRelease-Alpha
Build Number28042
VersionPR #24630
Bundle IDorg.wordpress.alpha
Commit62c6317
Installation URL7f23vinvh7ir0
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 26, 2025

App Icon📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack
ConfigurationRelease-Alpha
Build Number28042
VersionPR #24630
Bundle IDcom.jetpack.alpha
Commit62c6317
Installation URL7e3v9si7ffa18
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

Comment on lines 115 to 116
# Disabled till https://github.com/wordpress-mobile/WordPress-iOS/pull/24537 is completed
if: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm... this syntax didn't achieve the result I hoped

image

Copy link

@mokagio mokagio requested review from crazytonyli, kean and a team June 27, 2025 01:28
#
# The boolean value has to be a string explicitly.
# See validation error against schema.
if: "false"
Copy link
Contributor Author

@mokagio mokagio Jun 27, 2025

Choose a reason for hiding this comment

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

Notice the step for Keystone Tsets is no longer there

image

Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

Approving to unblock but left a small suggestion for consolidation of potential future edge case

@@ -0,0 +1,17 @@
#!/bin/bash -euo pipefail
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 worth extracting that on its dedicated set -euo pipefail line just to be sure.

This is because I think that if the command in the pipeline is a single string to a script with no parameter—and not a multiline string invoking multiple commands or a string invoking a command with parameters—then Buildkite might source that script (which makes the shebang ignored) instead of calling it as a command.

This is not the case currently because while the command: is now single line, it still passes a parameter to the script, so Buildkite with still call it not source it.

But in case in the future we end up not needing that parameter anymore and remove it and this ends up having the obscure side-effect of having the script sourced … better safe than sorry?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tooling Build, Release, and Validation Tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants