Add live-UAA end-to-end integration test suite - #333
Open
duanemay wants to merge 6 commits into
Open
Conversation
uaa-cli's existing tests all mock the UAA HTTP responses, so a real server's response shapes are never exercised -- which is exactly how `uaa list-clients` broke against a real server (a go-uaa unmarshal bug in cloudfoundry-community/ go-uaa#allowpublic-string-value). Add a Ginkgo suite (build-tag gated, so `make test` is untouched) that boots a real UAA and drives the compiled uaa binary through client/user/group/token/misc commands, plus a GitHub Actions workflow to run it on every PR. Also documents further coverage gaps found while auditing this (missing fields, and a UAA feature go-uaa still models after its removal) for later triage in go-uaa-coverage-gaps.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a build-tag-gated, live-UAA end-to-end integration suite to exercise uaa-cli against real UAA response shapes (vs mocked HTTP), along with local/CI automation to boot UAA and run the suite.
Changes:
- Introduces a
//go:build integrationGinkgo suite that compiles and drives theuaabinary against a running UAA and validates JSON output / parsing. - Adds a
scripts/start-uaa.shhelper and amake test-integrationtarget to boot UAA and run the new suite locally and in CI. - Adds a GitHub Actions workflow to run the integration suite on PRs/schedule, plus a markdown note capturing additional go-uaa/UAA modeling gaps.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/start-uaa.sh | Builds/boots a real UAA (hsqldb profile) and waits for readiness. |
| Makefile | Adds test-integration target to run the integration-tagged suite. |
| integration/uaa_test.go | New build-tagged Ginkgo integration suite that runs uaa-cli commands against live UAA. |
| go-uaa-coverage-gaps.md | Documents additional go-uaa/UAA modeling gaps found during audit. |
| .github/workflows/integration-test.yml | CI workflow to checkout/build UAA and run the integration suite. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Ginkgo's -r recursion treats a package whose build constraints exclude all files as a compilation failure, not a skip -- so adding the build-tag-gated integration suite broke the existing `make test` / go.yml CI job. Skip it explicitly; it has its own make test-integration target and CI job. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- scripts/start-uaa.sh: bound each readiness curl with --max-time so a stalled connect can't defeat the overall TIMEOUT_SECONDS loop. - integration/uaa_test.go waitForUAA: use an http.Client with a timeout for the same reason (a stalled request could otherwise hang past the 60s deadline). - integration/uaa_test.go groups suite: delete the created group via the raw SCIM endpoint in AfterAll (uaa-cli has no delete-group command yet), so rerunning against an already-running UAA doesn't collide on the group name. - go-uaa-coverage-gaps.md: replace a developer-local absolute path with a GitHub link to the same directory. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Split integration/uaa_test.go into one file per resource area (clients, users, groups, tokens, misc) plus a shared suite_test.go, each its own top-level Describe instead of one giant Ordered block. Ginkgo's Ordered containers skip every remaining spec once one fails, so a single failure anywhere was hiding the pass/fail status of everything unrelated to it. Verified against a live UAA: with the two known-bad specs left pending, splitting drops "22 skipped" to "0 skipped" for a real failure. While isolating those two pending specs against a freshly-booted UAA (confirming they're not local-state artifacts), found and fixed a real uaa-cli bug: RefreshTokenValidations rejected an explicitly-empty client_secret as "missing", breaking refresh-token for public clients (e.g. UAA's built-in "cf" client) entirely. get-password-token already allows an empty secret for the same reason; refresh-token now matches. The other pending spec (unlock-user) hits a real, ~9-year-old bug in cloudfoundry/uaa itself, not uaa-cli: ScimUserEndpoints.updateAccountStatus is missing @responsebody (dropped in commit 6159e2f4, 2016), so PATCH /Users/{userId}/status always 500s. Left pending with a comment until that's fixed upstream. Also fixed the "clients" area's own test fixture: the throwaway client needs the clients.secret authority to change its own secret via change-client-secret, which it didn't have. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
test-all runs both the unit suite and the live-UAA integration suite. go-uaa-coverage-gaps.md is exploratory notes from this session, not meant to ship as part of this PR -- removed from git tracking but left in place locally for reference. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment on lines
+57
to
60
| // client_secret is intentionally not required here: public clients (e.g. | ||
| // UAA's built-in "cf" client) have no secret, and get-password-token | ||
| // already allows an empty client_secret for the same reason. | ||
| if cfg.GetActiveContext().ClientId == "" { |
Comment on lines
+30
to
+39
| nohup java \ | ||
| -DCLOUDFOUNDRY_CONFIG_PATH="${UAA_DIR}/scripts/boot" \ | ||
| -DSECRETS_DIR="${UAA_DIR}/scripts/boot" \ | ||
| -Dserver.servlet.context-path=/uaa \ | ||
| -Dsmtp.host=localhost \ | ||
| -Dsmtp.port=2525 \ | ||
| -Dspring.profiles.active=hsqldb \ | ||
| -Djava.security.egd=file:/dev/./urandom \ | ||
| -jar "${WAR_FILE}" > uaa-boot.log 2>&1 & | ||
|
|
Comment on lines
+14
to
+16
| AfterEach(func() { | ||
| run("get-client-credentials-token", "admin", "-s", "adminsecret") | ||
| }) |
Comment on lines
+18
to
+22
| PIt("lists clients, including the legacy string-typed additionalInformation clients seeded by scripts/boot/uaa.yml", func() { | ||
| // login, client_federated_jwt_trust, client_with_allowpublic_and_jwks_uri_trust, | ||
| // and oauth_showcase_saml2_bearer are all seeded with allowpublic/autoapprove as | ||
| // YAML strings, which UAA stores verbatim and serializes back as JSON strings | ||
| // rather than booleans -- the exact shape that broke `uaa list-clients` originally. |
3 tasks
- refresh-token: update the Cobra Use string and help.RefreshToken() text to reflect that -s/--client_secret is no longer required (it's optional for public clients), matching the earlier validation fix. - scripts/start-uaa.sh: run gradle with --no-daemon (recommended for CI), and capture/print the launched java PID to uaa-boot.pid for easier local cleanup/debugging. Both added to .gitignore. - integration/suite_test.go: waitForUAA now only treats 2xx/3xx as ready, not any status < 500 -- a 404/401 from a misconfigured target no longer looks like a healthy server. - integration/tokens_test.go, clients_test.go: the admin-token restore in AfterEach/AfterAll now uses runOK instead of run -- other Describes assume that token is active, so a silent failure here would cause confusing failures elsewhere instead of a clear one here. - Documented (workflow file + PR description) that the spec reproducing the original regression is currently PIt pending the go-uaa release, since the workflow's "guard against regression" framing didn't match that reality. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
uaa list-clientsbroke against a real server (go-uaa incorrectly unmarshallingallowpublic— see cloudfoundry/go-uaa#175).integration/,//go:build integration, one file per resource area) that boots a real UAA server and drives the compileduaabinary through client/user/group/token/misc commands, asserting no unmarshal/response-parsing errors and valid JSON output.scripts/start-uaa.shto build and boot UAA's default (hsqldb, no Docker) profile and wait for readiness,make test-integration/make test-alltargets, and a GitHub Actions workflow (.github/workflows/integration-test.yml) to run it on every PR to master.go-uaa-coverage-gaps.md(local only, not tracked in this PR) captures further coverage gaps found while auditing this for later triage.Known gaps / currently disabled
get-authcode-token/get-implicit-tokenaren't exercised (need a real browser + local OAuth redirect callback). Zone-scoped (-z/--zone) command variants aren't exercised either.integration/clients_test.go, "lists clients...") is markedPIt. It will fail until cloudfoundry/go-uaa#175 is released and go.mod is bumped to it — there's no released go-uaa version with the fix yet. Until then, this workflow exercises everything else but not that specific regression; un-pend it once go-uaa#175 ships.integration/users_test.go's "unlocks the user" is alsoPIt: it hits a real, separate bug incloudfoundry/uaaitself (missing@ResponseBodyon the account-status endpoint — see cloudfoundry/uaa#4053), not a uaa-cli or go-uaa issue.Test plan
go build ./...andgo vet -tags integration ./integration/...passmake test(existing unit suite) unaffected — new suite is excluded via--skip-package=integration