Skip to content

ci: add test workflow, split offline and live CLI tests - #17

Merged
mivds merged 2 commits into
mainfrom
ci/add-test-workflow
Sep 10, 2026
Merged

mivds merged 2 commits into
mainfrom
ci/add-test-workflow

Conversation

@mivds

@mivds mivds commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Nothing runs on pull requests. release.yml is the only workflow and it fires on tags, so go build and go test happen only on a contributor’s machine — a PR that breaks the build gets no signal.

The integration suite could not simply be pointed at CI. A single integration build tag covered both tests that need a live Cloud org and tests that need nothing at all, and skipIfNoCredentials partitioned them at runtime, per function. On a machine without credentials 54 of 82 subtests vanished silently, so "the suite passed" meant very little.

Change

The suite is split into two tiers by build tag, sharing one harness:

  • cli — builds the binary and drives it as a subprocess. No network, no credentials, no skips. This runs on every PR.
  • cloud — the same harness against a real Cloud org. Creates and deletes real resources, so it stays out of CI and keeps its credential gate.

test.yml runs unit tests and the cli tier on pull requests and pushes to main.

Pushing the credential gate down to the tests that actually need it recovers 19 subtests that were skipping for no reason — the offline tier goes from 28 to 47 passing. Most valuable among them: every blocked-command test. The "not yet available" guard runs before the auth check, so the exit-code-2 contract that CI consumers depend on is now enforced on each PR instead of only on a developer machine with live credentials.

No assertions were changed. Seven flag-validation tests were consolidated into one table-driven test, and the invariant they encode is now stated explicitly: validation precedes auth, so a bad flag exits 2 (execution error), not 3 (authentication error). Two candidates turned out to violate that — contract verify <missing file> and contract pull resolve the profile before reading arguments, so they exit 3 without credentials and stayed in the cloud tier.

Caveats for review

gofmt is deliberately not a CI step: 12 files on main are already unformatted, so a format gate would fail on arrival. Only files touched here were formatted. Worth a follow-up.

The cloud tier now runs nowhere automatically. It was never in CI, so this is not a regression, but a scheduled job with the SODA_TEST_* secrets is the obvious next step.

Testing

go test ./..., go test -tags cli ./tests/integration/... (47 pass, 0 skip), and go test -tags cloud ./tests/integration/... (skips cleanly with no credentials) all pass, and go vet is clean under both tags. To confirm nothing was lost in the move, the full before/after test-name sets were diffed: the four names that disappear all map to renames.

The only workflow was release.yml, so nothing ran on pull requests. The
integration suite could not fill that gap: one `integration` build tag
covered both offline tests and tests needing a live Cloud org, and
`skipIfNoCredentials` partitioned them invisibly at runtime.

Split the suite into two tiers by build tag — `cli` (builds the binary,
no network, no credentials) and `cloud` (live org) — and add test.yml
running unit plus `cli` on PRs and pushes to main.

Moving the credential gate down to the tests that need it recovers 19
subtests that were skipping for no reason, including every
blocked-command test: the "not yet available" guard runs before the auth
check, so the exit-code-2 behaviour is now enforced in CI.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mivds mivds self-assigned this Sep 10, 2026
Comment thread .github/workflows/test.yml
actions/checkout leaves GITHUB_TOKEN in the local git config by default,
where any later step or third-party action can read it. Neither test job
runs git operations, so there is nothing to keep it for.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mivds
mivds marked this pull request as ready for review September 10, 2026 08:38
@mivds
mivds requested a review from m1n0 September 10, 2026 08:39

@m1n0 m1n0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Checked the split rather than the summary. Build tags line up with the filename suffixes, and no _cli_test.go calls skipIfNoCredentials, so there's no runtime credential gating left in the offline tier to quietly skip things. Ran both jobs locally with SODA_TEST_* unset: unit green, offline tier green, 47 subtests, exit 0.

Nothing lost in the move either. Three names vanish from the suite, TestDatasourceCreateBadFile, TestRunnerCreateMissingName and TestSecretCreateMissingFlags, and all three turn up as subtests of the new table-driven TestFlagValidation. They were credential-gated before and now run on every PR, which is the part I like most here.

Workflow hygiene is right: pinned action SHAs matching release.yml, contents: read, persist-credentials: false, cancel-in-progress scoped to PRs. AGENTS.md matches what the workflow actually does and no -tags integration reference survives anywhere.

One to decide separately, not here: the cloud tier now has no automated home at all. Correct call for a PR workflow given it creates and deletes real resources, but tests that only run when someone remembers will rot. Worth a nightly with the secrets eventually.

@mivds
mivds merged commit 531af59 into main Sep 10, 2026
3 checks passed
mivds added a commit that referenced this pull request Sep 11, 2026
The offline CLI suite fails on main: TestContractLint/invalid_contract
expects `bogus_field` at the contract root to exit 2, but lint accepts it
and exits 0.

Two PRs merged a minute apart caused this. #16 synced the contract JSON
schema from the backend, which flips the root from
"additionalProperties": false to true — unknown top-level keys are now
allowed by design — and dropped its own TestLintFile_InvalidProperty
accordingly. #17 branched before that landed and added a CLI test
asserting the old behavior. Both were green against their own base; the
merge of the two is not.

The schema is generated backend-side, so pinning the root closed again
would be reverted by the next sync and would reject contracts the
backend accepts. Point the subtest at a check object instead: those are
still closed, so it keeps CLI-level coverage of the "additional
properties not allowed" path rather than deleting it.

Refs: #16, #17

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants