[STEP-2494] Migrate to Unified CI - #117
Conversation
Replaces the monolithic `test` workflow with `check` / `e2e` / `sample`, e2e cases under e2e/bitrise.yml with the `test_` prefix. `check` is included from steps-check/steps.bitrise.yml instead of the legacy git:: Step. The e2e run now builds the app and test APKs itself from bitrise-io/android-multiple-test-results-sample, the same sample the android-build-for-ui-testing Step's own e2e uses. The legacy project instead downloaded prebuilt APKs from two app-level uploaded files, which nothing in the repo described and which the shared Unified CI project does not have. The device test is gated to the latest Ubuntu stack. Its result comes from Firebase Test Lab's virtual devices and does not depend on the stack, and Ubuntu is the only selected stack where building an Android app is cheap. test_flaky_and_quarantined_tests wraps the real workflow in bitrise-run behind IS_LATEST_STACK_UBUNTU; locally the guard is `not .IsCI`, so it always runs. Adds test_device_catalog_up_to_date, comparing step.yml's device table against the live Firebase catalog. The check existed but nothing ever ran it — the app-side maintenance workflow had never been triggered. It needs no devices and no app build, so it runs on every stack, using GCP_SERVICE_ACCOUNT_JSON_VDT_DEVICE_FETCHER. It now sits behind a `maintenance` build tag so `go test ./...` in check skips it. The results assertion derives its filename from TEST_DEVICES instead of hardcoding it, and additionally asserts that the identically named test method in the sibling class still ran, so an over-broad exclusion cannot pass as a successful quarantine.
The shared check workflow runs golangci-lint with staticcheck enabled.
|
Verified end to end before review, since the legacy check on this PR cannot pass (the old project's I pushed a throwaway branch combining this PR with #118 and triggered Unified CI manually against it: build 339900 — green on all 17 stacks. The stack gate behaves as intended:
An earlier run on this branch alone is also worth noting: One finding for a follow-up, not a blocker: the controller assigns Merge order: #118 → this → bitrise-io/bitrise-github-steps#203. |
Its last usage was removed in #112, but the vendored copy stayed behind. `go mod vendor` drops it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The maintenance workflow only printed the fresh device list and failed, leaving someone to copy two blobs out of a build log by hand. It now writes them. `go test ./... -update` in the maintenance directory rewrites the expected list and the table in step.yml's test_devices input, and the workflow regenerates the README afterwards, leaving a reviewable diff in the working tree. Without -update the test only reports, which is what the e2e case does. The expected list moves from a 169-line const in the test to testdata/device_list.txt, so applying it is a file write rather than the test rewriting its own source. step.yml is edited by locating the description with a YAML parse and rewriting only those lines. Re-encoding the document would reformat every other block in the file and fight yamlfmt. The catalog check is tooling rather than part of the Step, so it is now its own module. gopkg.in/yaml.v3 stays out of the Step's go.mod and out of vendor/, and a nested module is invisible to the Step's `go test ./...`, so the check no longer needs a build tag to stay out of the check workflow. Also limits test_device_catalog_up_to_date to one stack. The catalog is the same everywhere, so the other 16 runs were duplicated work — and 17 gcloud sign-ins per build, any one of which failing takes the whole build down. That is exactly how build 339936 failed: one macOS worker could not reach oauth2.googleapis.com. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6cdd61d to
bb4f180
Compare
Review of #117 found that the block scalar's indentation was taken from the first content line whatever it was. A leading blank line in the description — legal YAML — made it the empty string, which every line has as a prefix, so the range ran to the end of the file and the rewrite replaced everything after the description with the description. Nothing exercised the path, because CI runs the check without -update, so the first real drift would have been the first execution. blockScalarRange now takes the indentation from the first non-blank line and refuses an empty one, which turns that case into a correct rewrite rather than a destroyed file. It also stops at the last indented line instead of consuming the blank lines after the block, which it used to swallow without putting them back. updateStepYML re-reads and re-parses the file afterwards and fails if the description does not hold what was intended, so a bad splice cannot reach a commit even unattended. Adds unit tests for both functions, including the leading-blank-line case. They need no credentials, so `go test ./...` covers them on every run of the e2e catalog check, not only when the catalog drifts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
grep's default regular expressions made the dots in a class name match any character, so a different class could satisfy the check. Worse, a parameterized test name's `[0: 4]` suffix reads as a bracket expression, and the pattern would then fail to match a test that is present — reporting the quarantine as successful when it was not. #115 exists because parameterized names do turn up on this path, so this is not hypothetical, only unexercised. -F fixes both directions. Notes on the case -F does not fix: the results XML keeps the suffix that the Step strips when building the exclusion, so a parameterized test needs the pattern reworked rather than just quoted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| # Needs the gcloud CLI, and either an already signed in account or | ||
| # $SERVICE_ACCOUNT_JSON. Without -update the test only reports the drift. | ||
| cd maintenance | ||
| go test -v ./... -update |
There was a problem hiding this comment.
Any reason this automation is written as a test and not as a plain Go file with a main function? Not a blocker, but might be a quick and easy refactor.
| // is located by parsing step.yml, but only its lines are rewritten: re-encoding the whole | ||
| // document would reformat every other block in the file. |
There was a problem hiding this comment.
Is that really an issue? We run ymlfmt + the Autofix CI step. I'd rather let it generate an imperfectly formatted YML instead of all this complex code.
| @@ -0,0 +1,123 @@ | |||
| package maintenance | |||
There was a problem hiding this comment.
Do we really need to test this? The maintenance workflow is run manually, and the result can be visually observed before opening a PR. This is also just an automation, it's not the production step code.
| workflows: | ||
| # `test_` prefix is used by unified CI to filter tests to run | ||
|
|
||
| # The devices are Firebase Test Lab's, so the result does not depend on the stack. Limited to |
There was a problem hiding this comment.
I think this comment should be next to the run_if expression
Checklist
step.ymlandREADME.mdis updated with the changes (if needed)Version
No version update — CI configuration and maintenance tooling only, the Step itself is unchanged apart from lowercased error strings.
Context
Migrates this repo to Unified CI, the way ios#53 did. The legacy project
9ed7723d7d1575dfwas the last custom CI for this Step.The Terraform switch (bitrise-github-steps#203) is already merged and applied, so this PR reports the
ci/bitrise/48fa8fbee698622c/prcheck and the legacy one is gone.Depends on #118 (merged) for the refreshed device list — without it the new catalog check fails.
Changes
CI structure
bitrise.yml: the monolithictestworkflow is replaced withcheck/e2e/sample.checkisincluded fromsteps-check/steps.bitrise.ymlinstead of the legacygit::steps-check.gitStep.e2e/bitrise.yml(new):test_flaky_and_quarantined_tests(the migrated legacytest) andtest_device_catalog_up_to_date.main.go: 10 error strings lowercased for staticcheck ST1005, which the sharedcheckenforces.vendor/: drops the vendoredgo-utils/sliceutil, left behind when its last usage was removed in remove sliceutils #112.The
maintenanceworkflow now applies the catalog change instead of reporting itgo test ./... -updateinmaintenance/rewritesmaintenance/testdata/device_list.txtand the device table instep.yml'stest_devicesdescription; the workflow then regenerates the README, leaving a reviewable diff in the working tree. Without-updatethe test only reports, which is what the e2e case does.constin the test totestdata/device_list.txt, so applying an update is a file write rather than a test rewriting its own source.step.ymlis edited by locating the description with a YAML parse and rewriting only those lines. Re-encoding the document would reformat the whole file and fight yamlfmt.maintenance/is its own Go module.gopkg.in/yaml.v3stays out of the Step'sgo.modand out ofvendor/, and a nested module is invisible to the Step'sgo test ./..., so the check no longer needs a build tag to stay out ofcheck.Investigation details
The legacy e2e never built its own APKs. The app-side
primaryworkflow downloaded prebuilt APKs from two app-level uploaded files ($BITRISEIO_APP_URL,$BITRISEIO_TEST_URL). Nothing in the repo described where they came from — the filenames suggest they were built fromgooglesamples/android-testingyears ago — and the shared Unified CI project does not have them. The e2e now buildsandroid-multiple-test-results-samplewithandroid-build-for-ui-testing, reusing the setup from that Step's own e2e (set-java-version→simple-git-clone→install-missing-android-tools→ build). That also means the e2e covers theandroid-build-for-ui-testing→ VDT handoff every real workflow has.The catalog check had never run. The app-side
maintenanceworkflow that would have invoked it has zero builds, here and in the iOS project. It is now an e2e case, and it caught real drift on its first ever execution — which is what #118 fixed.Verification. Green on all 17 selected stacks. On the latest run
linux-docker-android-22.04ran the full device test (~4.5 min) and the other 16 stacks ~20-45s each.Decisions
Both e2e cases are gated to one stack. Their results come from Firebase Test Lab, not from the stack, so running them everywhere only spends device minutes and gcloud sign-ins on the same assertions.
test_flaky_and_quarantined_testsis abitrise-runwrapper behindrun_if: {{ or (enveq "IS_LATEST_STACK_UBUNTU" "true") (not .IsCI) }}, so it always runs locally. Ubuntu is also the only selected stack where building an Android app is cheap.The catalog check was initially left ungated on the reasoning that it needs no devices or app build. That was wrong, and a build proved it: it made 17 gcloud sign-ins per build, and one macOS worker failing to reach
oauth2.googleapis.comtook the whole build down. It is now gated too.A build tag would not have been enough for the maintenance module.
checkrunsgo test ./...without secrets and the catalog test needs gcloud credentials. A separate module keeps it out of that run and out ofvendor/. Skipping at runtime on an empty$SERVICE_ACCOUNT_JSONwas the other option, and it lets the package report a pass without checking anything.The quarantine assertion is stricter than the legacy one. It derives the result XML filename from
TEST_DEVICESinstead of hardcodingMediumPhone.arm-33-en-portrait..., and it additionally asserts that the identically named method in the sibling test class did run — otherwise an exclusion that was too broad reads as a successful quarantine. The greps use-F: the class name's dots would otherwise match any character, and a parameterized test's[0: 4]suffix would be read as a bracket expression, which can make a failed quarantine report success.Review follow-ups
A review of this PR found that the
step.ymlrewriter took the block scalar's indentation from the first content line whatever it was. A leading blank line — legal YAML — made it the empty string, which every line has as a prefix, so the rewrite replaced everything after the description with the description. Nothing exercised the path, because CI runs the check without-update, so the first real drift would have been its first execution.Fixed in
0698049: the indentation comes from the first non-blank line and an empty one is refused, the range stops at the last indented line instead of swallowing the blank lines after the block, andupdateStepYMLre-reads and re-parses the file afterwards and fails if the description does not hold what was intended — so a bad splice cannot reach a commit even unattended.maintenance/rewrite_test.gocovers both functions, needs no credentials, and therefore runs on every execution of the catalog check rather than only when the catalog drifts.Known follow-up
IS_LATEST_STACK_UBUNTUcurrently resolves tolinux-docker-android-22.04, a frozen stack with a removal date of 2027-04-08. When it is removed, nothing carries the marker and both gated workflows would skip on every stack while still reporting success. Fixed controller-side in tooling-centralized-ci-pipeline-controller#111; no change needed here.