[synackd]

Description

Fixes #232
Fixes #235
Fixes #236

Summary

These are the follow-ups to the test layout work in #218. They close a gap in the one-file-per-subject check, add a check that a subject's bucket files share a stem, and remove per-family error tests that repeat a row of a command-wide table. The change touches only tests, test fixtures, and CONTRIBUTING.md, so it needs no changelog entry.

The commits are meant to be reviewed one at a time. Each one passes make lint reuse mod test.

Changes

#232: test: ignore non-platform tags in subject grouping

platform() used the whole //go:build expression as a grouping key, so a file tagged linux && integration got its own group and could split a subject's tests. It now compares the GOOS/GOARCH pairs a file can build for, rather than the text of its constraints. A file name and a constraint that select the same platforms group together, whatever their order. Tags that don't name a platform don't make a file platform-specific: canBuild tries each setting of them, with one value per tag across the file. canBuild, appendOtherTags, and matchesPlatform replace namesPlatform.

This also fixes cases the issue doesn't name: amd64 && linux vs linux && amd64, windows_amd64 in a file name vs windows && amd64, and linux || darwin vs darwin || linux.

#235: test: keep subject bucket files on one stem

Test	From	To
TestLoad_ClusterShapeBoundaries, TestLoad_AcceptsNullClusterConfig	pkg/config/config_boundary_test.go	pkg/config/loader_boundary_test.go
TestValidateConfig_NullAndShapeBoundaries	pkg/config/config_boundary_test.go	pkg/config/validate_boundary_test.go
TestInitLogging_RejectsInvalidConfiguration	internal/cli/runtime_errors_test.go	internal/cli/runtime_logging_errors_test.go
TestRuntimeHandlePayload_StdinCancelled	internal/cli/runtime_errors_test.go	internal/cli/runtime_input_environment_errors_test.go

Each moved test is byte-identical to main. TestBSSHistoryGet_* becomes TestBSSHistory_*, since bss history is a leaf command.

The new TestTestFiles_SubjectBucketsShareStem flags a subject whose plain, _errors, and _boundary files have different stems. It compares only files for the same platforms, which covers TestWriteFile and TestSyncParentDirectory, and it leaves out cmd/commands_errors_test.go, which covers TestServiceCommands. CONTRIBUTING.md documents the rule.

#236: test: remove error tests that repeat table rows

This removes 19 tests that run the same command with the same failure as a table row and assert nothing beyond the error and exit code. cmd/bss_boot_script_errors_test.go held only one of them, so it is deleted.

Table	Removed
TestServiceCommands_NetworkError	TestSMDGroupGet_NetworkError, TestSMDCompepGet_NetworkError, TestSMDComponentGet_NetworkError, TestSMDIfaceGet_NetworkError, TestSMDRFEGet_NetworkError, TestBSSBootParamsGet_AllNetworkError, TestCloudInitGroupGet_NetworkError
TestWriteCommands_NetworkError	TestSMDGroupAdd_NetworkError
TestServiceCommands_HTTPError	TestSMDGroupGet_HTTPError, TestSMDCompepGet_AllHTTPError, TestSMDComponentGet_AllHTTPError, TestSMDIfaceGet_HTTPError, TestSMDRFEGet_HTTPError, TestBSSBootParamsGet_AllHTTPError, TestBSSBootScriptGet_HTTPError, TestCloudInitGroupGet_HTTPError, TestPCSTransitionList_HTTPError
TestCommands_MalformedPayload	TestCloudInitGroupAdd_MalformedPayload, TestCloudInitNodeSet_MalformedPayload

The policy is now in the "Command tests" section of CONTRIBUTING.md: a per-family error test stays when it uses a different selector, flag, or stub response than its row, or asserts more than the exit code.

Beyond the issues: test: cover deprecated status and vendor-data errors

Auditing #236 turned up three related problems, fixed here:

• TestSMDDeprecatedStatus_HTTPError ran smd service status, not the deprecated command, so it is now TestSMDServiceStatus_HTTPError.
• smd status and bss status hold their own copy of the status logic, and no test asserted their exit codes. Both now have rows in TestServiceCommands_NetworkError and TestServiceCommands_HTTPError.
• TestCloudInitNodeGetData_HTTPError repeated TestCloudInitNodeGet_MetadataHTTPError, while cloud-init node get vendor-data had no HTTP error test. It now tests vendor-data, as TestCloudInitNodeGet_VendordataHTTPError.

Review notes

• [DEV] Some subjects' error and boundary tests aren't in files named after their plain tests #235 missed a subject. TestRuntimeHandlePayload also had mismatched stems, because fix: cancel command context on SIGINT and SIGTERM #234 added _StdinCancelled after the issue was filed. The new check would fail on it, so it moved too.
• [DEV] Per-command error tests repeat rows of the command-wide error tables #236's count is out of date. It lists 15 _NetworkError tests, but there are 20, plus TestSMDGroupMember_NetworkErrors. Only 8 of them repeat a row. The rest stay: selector variants (ByXname, ByNID, ByID), discover variants, and commands with no table row (bss boot params writes, boot and metadata list, smd group member).
• Two tests were kept on purpose. TestCloudInitNodeGet_MetadataHTTPError uses a 404 and TestPCSStatusList_HTTPError uses a 503, while the table's stub returns 500. clientErrorCode ignores the status today, so they could count as duplicates. They stay in case status-specific behavior appears.
• Fixture layout. testdata/violations now has a cmd/ directory so that the check's path exception is covered by a fixture.

Manual testing

make lint reuse mod test   # passes, 0 lint issues, no go.mod/go.sum changes

# 1024 on main: 2 new tests, 19 removed
go test ./... -list . | grep -c '^Test'   # 1007

The stem script from #235 now prints only the intentional cases:

cmd TestServiceCommands ['commands', 'service_status']
pkg/config TestSyncParentDirectory ['sync', 'sync_windows']
pkg/config TestWriteFile ['owner', 'write']

The rest was checked once by hand:

• [Bug]: Convention check permits non-platform tags to split test subjects #232: the linux && integration fixture gave 3 split violations with the old platform() and gives 4 now. matchesPlatform agrees with go/build on 611 tag and platform combinations from go tool dist list, and platform() matched go/build on 400 random constraints. 95 of those can never build, so they compare nothing.
• [DEV] Some subjects' error and boundary tests aren't in files named after their plain tests #235: renaming loader_boundary_test.go to a mismatched stem makes the new check fail.
• [DEV] Per-command error tests repeat rows of the command-wide error tables #236: block-level coverage from the cmd tests, across ./cmd/..., ./internal/cli/..., and ./pkg/..., is 5,042 covered blocks before and after, with none lost.
• Deprecated commands: against a stub returning 503, smd status and bss status exit 6, and against a closed port they exit 7. Removing the classification from smd status fails its NetworkError row. The HTTPError row doesn't fail on that change, because ExitCode maps the HTTP sentinel error to CodeHTTP by itself.

Checklist

• My code follows the style guidelines of this project
• I have added/updated comments where needed
• I have added tests that prove my fix is effective or my feature works
• I have run make test (or equivalent) locally and all tests pass
• I have updated the relevant documentation (CLI examples, man pages, README, other docs, etc.) (CONTRIBUTING.md)
• DCO Sign-off: All commits are signed off (git commit -s) with my real name and email
• REUSE Compliance:
  • Each new/modified source file has SPDX copyright and license headers
  • Any non-commentable files include a <filename>.license sidecar
  • All referenced licenses are present in the LICENSES/ directory

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update
• Dependency update
• Build system/CI

---

• I agree to follow the Code of Conduct.

[davidallendj]

LGTM. I tested this and all tests passed with the exception of the few that deliberately fail due to the changes.

=== RUN   TestTestFiles_SubjectInOneFile
    conventions_test.go:933: internal/conventions/pr243probe.ZVEw0Z/widget_linux_test.go:11: TestWidget tests (plain) are split between widget_integration_test.go and widget_linux_test.go
    conventions_test.go:933: Move the test into its subject's file for that bucket, or give a too-broad subject a more specific name; see CONTRIBUTING.md.
--- FAIL: TestTestFiles_SubjectInOneFile (0.08s)
FAIL
FAIL    github.com/openchami/ochami/internal/conventions        0.330s
FAIL

=== RUN   TestTestFiles_SubjectBucketsShareStem
    conventions_test.go:941: pkg/config/pr243_wrong_boundary_test.go:17: TestLoad tests are in loader_errors_test.go and pr243_wrong_boundary_test.go, which have different stems
    conventions_test.go:941: Name a subject's bucket files <stem>_test.go, <stem>_errors_test.go, and <stem>_boundary_test.go. See CONTRIBUTING.md.
--- FAIL: TestTestFiles_SubjectBucketsShareStem (0.08s)
FAIL
FAIL    github.com/openchami/ochami/internal/conventions        0.746s
FAIL