test: testing standards + first-batch coverage climb (16% → 17.7%) + integration suite repair#273
Merged
Conversation
Establishes the testing standards needed to climb toward the OpenSSF Best Practices statement-coverage targets (Silver ≥ 80 %, Gold ≥ 90 %) and closes the test_invocation / tests_documented_added / test_policy_mandatory criteria with a single authoritative doc. What this lands: - docs/TESTING.md: framework choice (gomega; the dominant style at 77 % of existing test files), canonical test shape, sub-test + table-driven patterns, preferred matcher rank, mockery v2.53.4 generation pattern, unit-vs-integration separation (new convention: //go:build integration tag on *_integration_test.go), full test-invocation cheatsheet including the new coverage task, when-tests-are-required policy, file + variable naming, fixtures, coverage targets + per-PR no-regression rule, CI invocation map. - welder.yaml: new `coverage` task running the full suite with -coverprofile=atomic, filtering cmd/*/main.go entry points and pkg/**/mocks/ auto-generated files (which would otherwise hide real gains), printing per-package + aggregate breakdown. - docs/CONTRIBUTING.md: point the test rule at TESTING.md, add the coverage + integration test invocations to the local-dev block. What this does NOT change yet: - Existing *_integration_test.go files do not declare the //go:build integration tag; the tag is the new policy. A mechanical migration PR will follow once the standards are merged. - The 16 % baseline coverage is the starting point. A per-PR no-regression gate will follow in a separate CI workflow PR once there is something to enforce against. Closes the OpenSSF Best Practices documentation gap for test-suite governance. No code-path behaviour changes. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Semgrep Scan ResultsRepository:
Scanned at 2026-05-19 09:40 UTC |
Security Scan ResultsRepository:
Scanned at 2026-05-19 09:41 UTC |
Closes the gap in docs/TESTING.md's integration-test policy (//go:build integration tag). Three of the five existing *_integration_test.go files already carried the tag; this commit adds it to the last two: - pkg/security/integration_test.go - pkg/security/scan/integration_test.go After this, every *_integration_test.go file is excluded from default `go test ./...` runs and only executes under `go test -tags integration ./...`. This matches the convention documented in docs/TESTING.md and avoids accidental dependency on external binaries (cosign / syft / trivy) for the unit suite. No code changes — only the //go:build directive at the top of each file. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
First batch of unit tests in the coverage climb toward Silver (≥80%)
and Gold (≥90%) statement coverage. Three pure-function packages that
previously had no tests at all.
Per-package coverage:
- pkg/api/git/path_util: 0% → 92.9%
Tests ReplaceTildeWithHome across no-tilde paths, "~/foo" current-
user expansion, "~username/foo" named-user expansion via os/user
lookup, error path for unknown user (verifies the function returns
the original path + error per its contract), and basic path-shape
invariants on the expansion result.
- pkg/api/logger/color: 0% → 100%
Table-driven tests over every exported helper (Fmt / single-value /
String / Assistant chat colours / Bold variants). Uses
fatih/color.NoColor=false so the helpers actually wrap input in
escape sequences; asserts each helper returns a non-empty string
containing the input substring. Empty-input safety check covers
the nil/zero-value path.
- pkg/clouds/fs: 0% → 84.6%
Covers FileSystemStateStorage + PassphraseSecretsProvider getters
(StorageUrl / KeyUrl / IsProvisionEnabled / CredentialsValue /
ProjectIdValue / ProviderType) and pins the provider-type
constants ("fs", "passphrase") that drive config parsing.
All three follow the gomega + table-driven + sub-test pattern
codified in docs/TESTING.md. No production code changes; no behaviour
changes.
Coverage on the touched files in aggregate: 94.5%.
Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Second batch of unit tests in the coverage climb. Touches four packages — three were at 0% before this commit: - pkg/api/secrets/ciphers: 79.8% → 84.0% Adds tests for PrivateKeyToBytes (was 0%), MarshalRSAPrivateKey, PublicKeyToBytes, MarshalPublicKey, MarshalEd25519PrivateKey/ PublicKey round-trip, RSA encrypt/decrypt round-trip across short / max-sized / over-budget plaintexts (covers the OAEP error path), EncryptLargeString + DecryptLargeString chunked round-trip across multiple block counts, ParsePublicKey round-trip + garbage-input error path, ed25519 keypair shape checks. - pkg/api/logger: 0% → 100% Covers the Logger interface (Error/Warn/Info/Debug), SetLogLevel context propagation, Silent context, default-level Info-emits and Debug-suppressed behaviour, full level-gating matrix (5 levels × 4 methods). Uses a captureStdout helper because the logger writes directly to os.Stdout instead of taking an io.Writer. - pkg/clouds/cloudflare: 0% → 83.3% Covers AuthConfig and RegistrarConfig getters (CredentialsValue / ProjectIdValue / ProviderType / DnsRecords), zero-value behaviour, and the provider-type / registrar-type constants that drive config parsing. - pkg/assistant/utils: 0% → 79.2% Covers CheckAndWarnExistingSimpleContainerProject across the detection matrix (client.yaml in root, .sc/stacks/*/client.yaml, server.yaml, secrets.yaml, .sc/stacks/ alone), the forceOverwrite / skipConfirmation flag short-circuits, the empty-path-defaults-to-dot fallback, and the non-interactive blocked-for-safety path. Interactive-stdin path uncovered by design (would require io.Reader injection — separate refactor). All tests follow the gomega + table-driven + sub-test pattern from docs/TESTING.md. No production code changes. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
…ncate
Third batch of coverage work in the climb toward Silver/Gold.
- pkg/clouds/discord: 0% → 45.6%
Covers getIconForAlertType across all 6 alert-type → emoji mappings
plus the unknown / empty default case; intelligentTruncate across
short-text / boundary / long-text / very-small-maxLength edge
cases; New() constructor against invalid + empty webhook URLs and
a synthetic valid URL (Send() itself still uncovered — needs a
webhook-client mock, deferred).
Surfaced and fixed a latent panic in intelligentTruncate: when
maxLength is below ~175 bytes, the floor clamps in the function
push beginningLen + endLen negative, and the slice expressions
text[:beginningLen] and text[len(text)-endLen:] panic with
"slice bounds out of range". Not reachable from production today
(Send() guards against this via `availableSpace > 50`), but a
defensive fall-back to a simple end-trim with "..." suffix keeps
the helper safe for any future caller. maxLength <= 0 now returns
an empty string instead of panicking.
- pkg/clouds/mongodb: 0% → 83.3%
Covers AtlasConfig getters (CredentialsValue / ProjectIdValue /
DependencyProviders / ProviderType), the AtlasNetworkConfig +
PrivateLinkEndpoint + AtlasBackup field round-trips, and pins the
ProviderType / ResourceTypeMongodbAtlas constants ("mongodb-atlas")
that drive config parsing.
All tests follow the gomega + table-driven + sub-test pattern from
docs/TESTING.md.
Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Fourth batch of coverage work. Raises pkg/util from 5.9% to 32.6% by adding tests for the helpers most reached for in this codebase: - ToObjectViaJson: struct→struct round-trip, marshal-error (channel type), unmarshal type-mismatch error, empty source. - MapErr: happy path, stop-on-first-error semantics with call counter, empty-collection. - AddIfNotExist / CopyStringMap / CopyMap: full edge-case matrix (empty / nil / dup-skip) with mutation-isolation assertions for the Copy* helpers. - Data.AddAllIfNotExist: existing-key-preserves-original-value pinning. - GetValue: nil input, top-level key, deeply nested dot-path, missing key error. - TrimStringMiddle: shape + length-cap pinning across short / exact / long inputs. - ToSnakeCase / ToEnvVariableName: table of PascalCase / camelCase / acronym / digit / hyphen / underscore inputs covering the regex branches. - SanitizeGCPServiceAccountName: deterministic-hash stability, different-inputs-different-outputs check (catches the collision bug the FNV-1a switch was meant to fix). - SanitizeK8sResourceName long-input path with FNV hash suffix assertion. - SafeSplit: bare / single-quote / double-quote / unmatched-quote paths. Existing string_test.go already covered TrimStringWithHash + SanitizeK8sResourceName + the SanitizeGCPServiceAccountName + real- world cases; this batch fills the rest of the file plus json/map/ split. All tests follow the gomega + table-driven + sub-test pattern. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Fifth batch. Raises pkg/clouds/github from 0% to 13.4% by covering
the legacy + enhanced GitHub Actions CI/CD configuration types. The
remaining gap is in templates.go and workflow_generator.go (huge
template-string generators that warrant a dedicated test pass).
Tests added:
- ActionsCiCdConfig getters: CredentialsValue / ProjectIdValue /
ProviderType (legacy config; ProjectIdValue is intentionally empty
per the existing "// todo: figure out" comment).
- EnhancedActionsCiCdConfig getters: same three plus the
Organization.Name passthrough for ProjectIdValue.
- SetDefaults: full defaulter pass — DefaultBranch / DefaultRunner /
WorkflowGeneration.{OutputPath / Templates / SCVersion /
CustomActions} / Execution.{DefaultTimeout / Concurrency.Group /
Permissions["default"] / RetryPolicy.{MaxAttempts / BackoffDelay /
RetryOn}}.
- SetDefaults preserves explicit values when provided.
- SetDefaults rewrites CustomActions to use the SCVersion tag when
SCVersion is set to a CalVer (instead of @main fallback).
- Validate: 5-case error matrix (missing auth-token, missing org name,
env missing type, env missing runner, protected env without
reviewers) + happy-path with multi-env protection-vs-reviewers.
- ProviderType + CiCdTypeGithubActions constants pinned.
All tests use gomega + table-driven + sub-tests per docs/TESTING.md.
Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Sixth batch. Raises pkg/api from 3.4% to 10.8% by adding tests for the configuration-file and descriptor-serialization seams that every SC command path crosses. Tests added: - ConfigFilePath: table over (workDir, profile) → expected path ".sc/cfg.<profile>.yaml" shape, including the empty-workDir case. - ConfigFile.ToYaml: serialization-round-trip with content assertions on every persisted field. - ConfigFile write + ReadConfigFile round-trip via the real on-disk path, including .sc/ directory creation. - ReadConfigFile from SIMPLE_CONTAINER_CONFIG env variable: happy path (env-driven config bypasses workDir / profile entirely) + invalid-YAML error path (error message must mention the env var name so the operator knows where to look). - ReadConfigFile missing-file path with the "profile does not exist" error wrap. - ConfigDirectory / env-variable constant pins so a rename to SIMPLE_CONTAINER_CONFIG breaks compilation rather than silently shipping. - UnmarshalDescriptor[T] happy path + invalid-YAML error path. - ReadDescriptor[T] happy path, missing-file error, invalid-YAML error (error message includes "unmarshal" or "yaml" depending on which yaml library reports the failure). Gomega + table-driven + sub-tests per docs/TESTING.md. The ScConfigEnvVariable env propagation uses t.Setenv for test isolation. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
The integration suite was build-tag-gated but the gated files
referenced APIs that had moved or been removed, so
`go test -tags integration ./...` failed with three compile errors.
Nobody noticed because the standard CI path runs `go test ./...`
without the integration tag.
This is foundational work for the coverage climb. The integration
suite exercises broad code paths (real cosign / syft / signing flows),
so getting it to compile + run is the prerequisite for merging its
coverage profile into the aggregate (via gocovmerge, planned as a
follow-up). The current change does not run any integration test —
it only restores the suite's buildability.
Fixes:
- pkg/security/executor_integration_test.go: `installer.CheckInstalled("cosign")`
→ `installer.CheckInstalled(context.Background(), "cosign")`.
The method gained a leading context.Context parameter and now
returns a single error instead of `(bool, error)`.
- pkg/security/signing/integration_test.go: same CheckInstalled
signature update; plus `versionChecker.ValidateVersion("cosign", stdout)`
used to return `(bool, error)` and now returns `error` only —
rewrote the call site to branch on `err == nil` for the success
log line.
- pkg/security/sbom/integration_test.go: `signing.CheckCosignInstalled(ctx)`
was removed from the signing package. Switched to the canonical
probe `tools.NewToolInstaller().CheckInstalled(ctx, "cosign")`
and imported pkg/security/tools (the signing import stays — used
later in the same file for signing.Config).
Verification:
- `go vet -tags integration ./...` — clean (was 3 errors)
- `go test -tags integration -run='^$' -count=1 ./...` — all
packages compile and report "ok ... [no tests to run]"
- `go test -count=1 -short ./pkg/security/...` — default unit suite
unchanged and green
Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Cre-eD
changed the title
smecsia
previously approved these changes
May 19, 2026
Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Cre-eD
force-pushed
the
docs/testing-standards
branch
from
984adcd to
79750bd
Compare
The DCO workflow checked every commit in BASE..HEAD for the Signed-off-by trailer, including merge commits. GitHub's "Update branch" button generates a merge commit using GitHub's own commit identity and without a Signed-off-by trailer, which broke the workflow on every UI-driven branch sync (just hit us on this PR's head 984adcd, amended to 79750bd). The fix is to pass --no-merges to the git log that builds the commit list. Merge commits are integration points without authored content — their diff is computed automatically from the parents — so DCO does not apply to them. The Probot DCO app (the most-used DCO bot on GitHub) and the Linux-kernel checkpatch tooling both skip merge commits for the same reason; this change brings our hand-rolled check into line with that convention. Authored commits on both sides of the merge are still verified because they appear individually in BASE..HEAD. If someone managed to land an unsigned-off authored commit into main (impossible today via branch protection) it would still be caught the next time a PR's BASE..HEAD walk crossed it. Future "Update branch" UI clicks on PRs now just work without needing a local amend round-trip. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
Branch-protection was BLOCKED because two simultaneous pull_request webhook deliveries on c38148e left CANCELLED + FAILURE check_runs behind from the dedup'd twin's downstream Status aggregator jobs. The dual-fire likely came from a delayed webhook from the prior force-push (79750bd at 08:16) racing with the c38148e push event (08:28). Both arrived at GitHub's queue at the same millisecond, neither saw the other as in-progress yet, so cancel-in-progress could not pre-empt. This empty commit creates a new HEAD SHA, against which branch protection re-evaluates check_runs — the stale FAILUREs no longer apply because they were tied to c38148e. The permanent fix lives in the simple-container-com/actions reusable workflows (semgrep.yml, security-scan.yml): adding `if: ${{ !cancelled() }}` to the Status aggregator jobs so a cancelled scan does not cascade into a FAILURE status check. That work is a separate PR. Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
2 tasks
smecsia
approved these changes
May 19, 2026
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
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
Lays the foundation for climbing toward OpenSSF Best Practices statement-coverage targets (Silver ≥ 80 %, Gold ≥ 90 %). The repo was at ~16 % aggregate when this PR opened; this PR moves the needle to ~17.7 % with first-pass unit tests in 12 packages, codifies the conventions every future test must follow, and repairs the integration suite so its broader code-path coverage can be merged in via a follow-up
gocovmergestep.The PR is intentionally consolidated (no follow-up PRs spawned for each batch) — each
mainmerge cuts a CalVer release, so we batch into a single landing.What's in (9 commits)
b4c8667docs(testing):docs/TESTING.md(gomega + table-driven + sub-tests + mockery v2.53.4 +//go:build integrationpolicy + invocation cheatsheet + coverage targets), newwelder run coveragetask with entry-point + auto-gen-mock exclusion filter, link fromdocs/CONTRIBUTING.md.dfef382test(integration): tag the 2 remaining*_integration_test.gofiles (pkg/security/andpkg/security/scan/) so defaultgo test ./...doesn't accidentally pull in cosign/syft/trivy dependencies.555b733test: quick-win unit tests —pkg/api/git/path_util0→92.9 %,pkg/api/logger/color0→100 %,pkg/clouds/fs0→84.6 %.b968817test:pkg/api/secrets/ciphers79.8→84.0 %,pkg/api/logger0→100 %,pkg/clouds/cloudflare0→83.3 %,pkg/assistant/utils0→79.2 %.860eb5ctest+fix:pkg/clouds/discord0→45.6 %,pkg/clouds/mongodb0→83.3 %; fixed a latent panic inintelligentTruncate(negative-slice-bounds whenmaxLengthis very small — unreachable from production today, but a defensive fallback to a simple end-trim with"..."suffix lands the test that would have caught it).05e0c49test:pkg/util5.9→32.6 % acrossjson.go/map.go/split.go/string.gohelpers.d6b72b9test:pkg/clouds/github0→13.4 % coveringActionsCiCdConfig+EnhancedActionsCiCdConfiggetters,SetDefaults,Validate.5a5bb80test:pkg/api3.4→10.8 % —ConfigFilePath,ConfigFile.ToYaml,ReadConfigFile(file path + env-var paths + missing-file error),UnmarshalDescriptor[T],ReadDescriptor[T].40075eftest(integration): fix 3 stale API calls sogo test -tags integration ./...compiles cleanly for the first time in months. Was a dead suite — nobody runs the tag, soinstaller.CheckInstalled("cosign")/versionChecker.ValidateVersion(...)(signatures changed) /signing.CheckCosignInstalled(removed) had drifted out of sync silently.Per-package coverage delta
pkg/api/loggerpkg/api/logger/colorpkg/api/git/path_utilpkg/clouds/fspkg/api/secrets/cipherspkg/clouds/cloudflarepkg/clouds/mongodbpkg/assistant/utilspkg/clouds/discordpkg/utilpkg/clouds/githubpkg/apiOpenSSF Best Practices criteria closed by
docs/TESTING.mdtest_invocationtest_continuous_integrationwelder run test+ the newcoveragetask)tests_documented_addeddocs/CONTRIBUTING.mdcross-linktest_policy_mandatoryWhat this does NOT close yet
test_statement_coverage80/test_statement_coverage90: 17.7 % is a long way from Silver (80 %) and Gold (90 %). The realistic path is not more hand-written unit tests in isolation — it's wiring the existing integration + e2e suites into a merged coverage profile viagocovmerge. The integration suite is now compilable (commit 9); the next PR will:go test -tags integration -coverprofile=int.cov,unit.cov+ (optionally) the existinge2e_*_test.goruns against the file-system Pulumi backend,pkg/provisioner.Test_Init/happy_path/initial_commit_is_presentfailure onmainis not addressed by this PR — it was failing before any of this work landed.Test plan
go test -count=1 -short ./pkg/...— green (all newly-tested packages pass).go vet -tags integration ./...— clean (was 3 errors before commit 9).go test -tags integration -run='^$' -count=1 ./...— every integration package compiles and reports "no tests to run" (proves the tag-gated source is now buildable; full integration run requires cosign/syft/trivy installed).Related
welder run coverage+.github/workflows/coverage.ymlposting a delta comment on every PR.