SYN-6889: add missing ut and integration tests, report coverage and test results in CI, add README badges - #53
Merged
Merged
Conversation
Run both syntheticsclient (v1) and syntheticsclientv2 (v2) in CI test execution instead of just v2, rename the misleadingly-named testacc target (there is no live acceptance suite in this repo) to test-cover, and publish a combined coverage profile plus go-test-coverage/dorny test-reporter checks so pass/fail counts and coverage % are visible directly on the PR. Coverage threshold is set to the current combined baseline (71%) as a regression guard; raise to 90% once outstanding test coverage work lands separately. Remove sonar-project.properties and the dead sonarqube Make target: no SonarQube project exists for this repo, and the file was internally inconsistent (v1 sources scored against v2-only coverage). Add Release/CI/Build/License badges to README.md. Co-Authored-By: Claude <noreply@anthropic.com>
v1 is slated for deprecation, so it should not run in tests, contribute to the build, or count toward the coverage gate. Scope Makefile/CI back to syntheticsclientv2 only and re-baseline the coverage threshold at its current 73.5%. Verified locally that go test's non-zero exit on a failing test propagates through `make test-cover`'s output redirection, so the CI job fails on any test failure independent of the coverage gate.
The coverage gate ran but never surfaced its number on the PR itself. Wire vladopajic/go-test-coverage's built-in diff/comment support: upload a coverage breakdown artifact on every v2 push, download it on PR runs to diff against, and post the resulting report via thollander/actions-comment-pull-request. The coverage step now continues on error so the comment posts before a threshold miss fails the job.
|
Code Coverage Report: |
Switch dorny/test-reporter to only-summary so the check/step summary shows pass/fail/skip counts without enumerating every individual test. Rename the coverage PR comment header to "Code Coverage Report".
Fills the syntheticsclientv2 coverage gaps identified in SYN-6889's baseline analysis: connection-failure, malformed-response, empty-body, and non-2xx-status branches across the CRUD clients, plus the redaction/query-param helpers in synthetics.go and get_checksv2.go. Coverage moves from 73% to 94.4%, so the CI gate's ratchet floor is raised from 73 to 90 (the ticket's target) rather than left as a regression-only guard. Co-Authored-By: Claude <noreply@anthropic.com>
The repo only has unit tests (integration_test.go already carries its own integration tag), so gating every test file behind -tags=unit_tests served no purpose beyond requiring the flag on every go test/build invocation. Co-Authored-By: Claude <noreply@anthropic.com>
…emoval Removing the unit_tests build tag from Makefile lint invocations exposed these test files to golangci-lint's errcheck for the first time, which flagged unchecked httptest handler w.Write return values.
Ports the privilege-separated pr-metadata + workflow_run pattern from splunk/terraform-provider-synthetics so the live suite's API_ACCESS_TOKEN can run in CI without ever being reachable from an untrusted fork-PR context, and only executes after a maintainer approves via the integration-test GitHub Environment. - .github/workflows/pr-metadata.yml: untrusted, no-secrets workflow that records the PR number/head SHA as an artifact. - .github/workflows/integration-test.yml: trusted workflow_run workflow. `resolve` validates the artifact against forgery (cross-checked against workflow_run's own head_sha and an independent pulls.get lookup) and opens a queued Check Run; `integration-test` is gated on the integration-test environment's required-reviewer approval and runs `make test-integration`; `report` posts the parsed result table and closes out the Check Run, kept as its own job so pull-requests:write/ checks:write never coexist on a runner that just ran fork PR code. - Makefile: add test-integration, scrubbing the X-Sf-Token header line from the go test -json log before it's captured. - integration_test.go: TestMain now reclaims any leftover beep-var / terraform-test-foo-301 fixtures before creating them, so a cancelled run doesn't wedge every subsequent run behind a duplicate-name error. - .gitignore: ignore .env.testacc (local live-test credentials) and integration.jsonl. Co-Authored-By: Claude <noreply@anthropic.com>
jcsco
marked this pull request as ready for review
August 10, 2026 17:51
jcsco
requested review from
bchhay-splunk and
dsanders-splunk
and removed request for
a team
August 10, 2026 17:51
jcsco
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4fe307cbb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…package v1 is slated for deprecation, but it was silently absent from `go test` output rather than reported at all -- FILES scoped `test`/`test-cover` to syntheticsclientv2 only, so the v1 package was invisible in CI, not skipped. Add skipDeprecated(t), called first in each of the 9 v1 tests, so `go test ./syntheticsclient/...` now reports every one of them as an explicit SKIP with a deprecation note. Makefile: run both packages in test/test-cover (TEST_FILES) so the SKIPs show up in the test-reporter summary, but keep -coverpkg scoped to syntheticsclientv2 only so v1 still doesn't count toward the coverage gate. build stays v2-only; v1 isn't being changed, just deprecated. Co-Authored-By: Claude <noreply@anthropic.com>
Addresses two review comments on PR #53: - ci.yml: GitHub downgrades GITHUB_TOKEN to read-only for pull_request runs from forks regardless of the checks:write/pull-requests:write requested at the workflow level, so the test-reporter check-run and the coverage PR-comment step would fail on any fork PR with "Resource not accessible by integration". Skip both steps when the PR's head repo isn't this repo; the job itself still fails on a real test/coverage regression either way. - integration-test.yml: the concurrency group was keyed by head repo + branch, so two different approved PRs could run the integration-test job at the same time even though they hit the same live org (sg0) and create the same fixed-name resources (beep-var, terraform-test-foo-301, and fixtures like the one TestLiveCreateVariableV2ReturnsErrorOnDuplicateName exercises). Use one fixed group for all PRs instead. Moved the concurrency key from the workflow level to just the integration-test job, so a second PR's resolve job (artifact validation, opening the queued check) still runs immediately rather than queuing behind however long the first PR's environment approval takes. Co-Authored-By: Claude <noreply@anthropic.com>
Commenting out `environment: integration-test` so the author can exercise the full PR Metadata -> resolve -> integration-test -> report flow without waiting on maintainer approval. This removes the pipeline's only defense against running fork-PR code with the live API token and MUST be reverted before merge.
…ting" This reverts commit 9ada2b1.
bchhay-splunk
approved these changes
Aug 12, 2026
report was resolving the check run to complete via checks.listForRef(...) .check_runs[0], which has no guarantee of returning the run created by this specific workflow run. A rerun of PR Metadata or this workflow for the same head SHA creates a second integration-test check run, and the wrong one could get completed, leaving the other stuck at queued.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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
syntheticsclient(v1) andsyntheticsclientv2(v2) viago testin CI (previously the Makefile scopedtest/test-coverto v2 only, so v1 was silently absent from CI output). v1 is slated for deprecation, so rather than actually running its suite, each of its 9 tests now calls a sharedskipDeprecated(t)helper and reports as an explicitSKIPwith a deprecation note — visible in thedorny/test-reportersummary — instead of the package quietly not appearing at all. Coverage (-coverpkg) andgo buildremain scoped tosyntheticsclientv2only, so v1 still doesn't count toward the 90% gate. Also rename the misleadingly-namedtestaccMake target totest-cover(no live acceptance suite exists in this repo).dorny/test-reporter+vladopajic/go-test-coveragechecks so pass/fail counts and a coverage gate are visible without opening the Actions log.syntheticsclientv2covering connection failures, malformed/empty API responses, non-2xx status codes, and the request/response redaction and query-param-building helpers — closing the gaps from SYN-6889's baseline analysis. Coverage moves from 71% (combined) / 73% (v2) to 94.4% (v2)..testcoverage.yml) is set at 90% — the ticket's target — now that the section 2 test work landed in this same PR, rather than staying at the interim 71% regression floor.unit_testsbuild tag from allsyntheticsclientv2test files and the-tags=unit_testsflag from the Makefile — the tag was pure overhead. This also fixed a latent gap:make lintnever passed-tags=unit_tests, so every test file was previously invisible togolangci-lint. Removing the tag surfaced realerrcheckviolations (uncheckedw.Writereturn values in httptest handlers, both pre-existing and newly added) — all fixed.sonar-project.propertiesand the deadsonarqubeMake target — no SonarQube project is registered for this repo, and the file was internally inconsistent (v1 sources scored against v2-only coverage).README.md.paths-ignore: README.mdfrom the workflow triggers — once this check is required, a README-only PR (including this one) would otherwise never get a status and become unmergeable.syntheticsclientv2/integration_test.go(the//go:build integration-gated live suite, run against a real Synthetics org) with 29 new tests covering SSL checks, TOTP variables, CA/client certificates, locations, downtime configurations, excluded file types, HTTP checks with nullable ports, and error paths for nonexistent-ID lookups and duplicate-name creation — filling in coverage gaps the suite had before this PR. Also renamed the harness's ad-hocmain-style setup into a properTestMain.API_ACCESS_TOKENto untrusted PR code:pr-metadata+workflow_runpattern fromsplunk/terraform-provider-synthetics. A new untrusted.github/workflows/pr-metadata.yml(no secrets, no checkout) records the PR number/head SHA as an artifact on everypull_requestevent..github/workflows/integration-test.yml, triggered viaworkflow_run, runs in the base repo's security context so secrets are reachable even for fork PRs, but never checks out or executes fork code until after validation and approval:resolve(ungated) downloads and independently verifies the artifact against forgery — checks its recorded SHA againstworkflow_run's ownhead_sha(unforgeable by a rewrittenpr-metadata.yml), then re-fetches the PR by number via the API and cross-checks state/draft/head/base — before opening aqueuedCheck Run.integration-testis gated behind the pre-provisionedintegration-testGitHub Environment, which requires approval from@splunk/o11y-synthetics-maintainersbefore it runs. It checks out the PR by exact SHA (not a mutable ref), re-verifies the checkout, and runs the suite via a newmake test-integrationtarget with the token passed only through step-scopedenv:.report(ungated, separate job) parses the results and posts a PR comment + finalizes the Check Run — kept separate from the gated job sopull-requests:write/checks:writenever coexist on a runner that just executed fork PR code (a hardening improvement over the sibling repo's design, where the gated job holds both).Makefile: newtest-integrationtarget scrubs theX-Sf-Tokenheader line from thego test -jsonlog before capturing it tointegration.jsonl.TestMainnow reclaims any leftoverbeep-var/terraform-test-foo-301fixtures before creating them, so a cancelled CI run doesn't wedge every subsequent run behind a duplicate-name error..gitignore: ignore.env.testacc(local live-test credentials — previously untracked but not ignored, a real risk for a public repo) andintegration.jsonl.harden-runneregress blocking is added (approval gate is the only technical defense against a malicious fork PR exfiltrating the token once approved — matches the sibling repo's actual behavior); the client library's unredactedResponseBodyfield (synthetics.go) is left alone here and filed as a follow-up, not fixed in this PR.Follow-up required (not done here, by design)
synthetics.go'sRequestDetails.ResponseBodyis stored unredacted whileRequestBodygoes throughsanitizeRequestDump. Route it through the existingredactSensitiveJSONFieldshelpers, and considerjson:"-"onRequestDetails.RawResponseso a futureString()/MarshalJSONmethod can't expose the token. Verified this is not a live-credential leak today (the structs actually printed carry no auth/header/password fields, andfmt.Println(*RequestDetails)rendersRawResponseas a bare pointer address), but it's fragile.sg0org, as defense-in-depth beyondTestMain's own reclaim logic.Test plan
Unit tests (
go test ./syntheticsclient/... ./syntheticsclientv2/... -coverpkg=./syntheticsclientv2/..., run locally 2026-08-10):syntheticsclientv2(v2)syntheticsclient(v1, deprecated)All 9 v1 skips report the
skipDeprecatednote; zero failures across both packages.Live integration tests (
make test-integrationagainst the realsg0org, run locally with.env.testaccsourced, 2026-08-10):TestLive*Confirmed zero occurrences of the real
API_ACCESS_TOKENvalue in the capturedintegration.jsonllog, and zero orphaned test/variable resources left insg0after the run (including a manual orphan-recovery check: created a straybeep-var, reran, confirmedTestMainreclaimed it instead of failing on duplicate-name).Other checks:
go build ./...passesgo vet ./...clean (includinggo vet -tags=integration ./syntheticsclientv2/...)gofmt -l .cleanGOGC=30 golangci-lint run ./syntheticsclientv2/...clean (confirmed with tag removed — the previously-hidden test files now lint cleanly too)make test-coverruns both packages, producescoverage.txt+test-results.jsonactionlint .github/workflows/ci.yml .github/workflows/pr-metadata.yml .github/workflows/integration-test.ymlcleanBuildjob passes, coverage summary appears, coverage-gate step passes at 90% threshold (94.4% actual)PR Metadataruns and uploads its artifact successfully on every pushintegration-test.yml'sworkflow_runtrigger itself (resolve → queued check → approval gate → live run → report comment) is not verifiable pre-merge: GitHub only firesworkflow_runfor workflow files already present on the default branch (v2), andintegration-test.ymlis new in this PR.PR Metadata(also new) already runs successfully on every push to this PR, confirming that half of the pipeline. Theintegration-test.ymljob body itself was validated by the localmake test-integrationrun above (identical command to the CI step) plusactionlint. Full wiring will be exercised on the first PR merged after this one lands onv2— recommend a maintainer watch that first run closely.🤖 Generated with Claude Code