Skip to content

Keep Unix connector tests out of Windows test builds - #961

Open
robzolkos wants to merge 1 commit into
mainfrom
fix/windows-connect-test-build
Open

robzolkos wants to merge 1 commit into
mainfrom
fix/windows-connect-test-build

Conversation

@robzolkos

@robzolkos robzolkos commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Move two connector-start tests into a Unix-tagged test file, alongside their Unix-only fixtures. Windows previously tried to compile those tests and failed on missing fixture helpers.

The two test bodies are retained unchanged. No production code, new harness, broad worker cleanup, or additional test system. This saves the narrow portability fix from the retired worker-cleanup work.

Verification

  • On current main, GOOS=windows GOARCH=amd64 CGO_ENABLED=0 go test -c ./internal/commands fails on the Unix-only fixture references.
  • The same Windows test compilation passes with this change.
  • Both moved tests pass in three race-enabled runs on Linux.
  • Full local bin/ci passes.

Windows compilation is verified; no Windows runtime test is claimed.


Summary by cubic

Fixes Windows test compilation by moving two connector-start tests into a Unix-tagged test file. The tests reference Unix-only fixture helpers, so Windows builds previously failed to compile them. Test bodies are unchanged; no production code is affected.

Verification

  • Windows test compilation now passes; it failed on main.
  • Both moved tests pass on Linux, and full bin/ci passes.

Written for commit 936a118. Summary will update on new commits.

View guided diff Turn on auto-fix

Copilot AI balanced review requested due to automatic review settings October 23, 2026 20:07
@robzolkos
robzolkos requested a review from a team as a code owner October 23, 2026 20:07
@github-actions github-actions Bot added commands CLI command implementations tests Tests (unit and e2e) labels Oct 23, 2026

Copilot AI 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.

🟢 Approved

The narrow test-only relocation matches the fixtures’ build constraint, preserves both test bodies, and leaves no unresolved findings.

No open issues 🎉

What changed in this PR

This PR isolates the Windows test-compilation fix from the retired worker-cleanup work in #829, without changing production code.

Changes:

  • Moves two unchanged connector-start tests into a file guarded by //go:build unix, matching their fixtures.
  • Removes the relocated tests and unused appctx import from the cross-platform test file.

[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

File Description
internal/​commands/​connect_start_wait_unix_test.go Houses the two existing tests behind a Unix build constraint.
internal/​commands/​connect_start_wait_test.go Removes Unix-dependent tests and the unused import.

Review effort: Balanced

@robzolkos

Copy link
Copy Markdown
Collaborator Author

Ready for review on 936a118: local CI passes and all 31 hosted checks are accounted for (26 successful, 4 conditionally skipped, 1 neutral; none failed or pending). Copilot approved this exact head. There are 0 unresolved review threads and 0 unanswered findings. Cubic's fresh review is unavailable because its monthly quota is exhausted. Nothing merged.

@jeremy

jeremy commented Oct 23, 2026

Copy link
Copy Markdown
Member

🤖 Local review at 936a118: clean. CI is green on it (27 passed, 4 skipped, 1 neutral).

The two moved tests are line-for-line the ones removed, and connect_start_wait_unix_test.go carries the same //go:build unix as newOperatorFixture (connect_operator_test.go L1). CI has no Windows job, so I checked that part by hand. GOOS=windows go test -c ./internal/commands/ compiles at this head. With the old file put back it fails with undefined: newOperatorFixture, newConnectSetupApp and usageError. The local review also ran the Windows test build of ./internal/commands and both moved tests on macOS, and all passed. It didn't run the full suite.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commands CLI command implementations tests Tests (unit and e2e)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants