Skip to content

SY-4167: Refactor Go Driver so linting and tests don't need build tags#2383

Open
pjdotson wants to merge 32 commits into
rcfrom
sy-4167-refactor-go-driver-usage-so-linter-and-tests-dont-need
Open

SY-4167: Refactor Go Driver so linting and tests don't need build tags#2383
pjdotson wants to merge 32 commits into
rcfrom
sy-4167-refactor-go-driver-usage-so-linter-and-tests-dont-need

Conversation

@pjdotson

@pjdotson pjdotson commented May 28, 2026

Copy link
Copy Markdown
Contributor

Issue Pull Request

Linear Issue

SY-4167

Description

The embedded driver package (core/pkg/driver) previously required the driver build
tag — and placeholder asset binaries — to lint and test. This forced CI to inject fake
assets/driver files before every lint/test run, kept the bulk of the driver lifecycle
logic out of the default build, and meant the subprocess lifecycle was only exercised in
a tag-gated suite.

This refactor removes that requirement:

  • All runtime logic (Config, the Driver struct, Open/start/close/setupCmd)
    moves into an untagged driver.go. Only a single defaultFS value remains
    build-tag-specific: nil in disabled.go (!driver) and the embedded filesystem in
    enabled.go (driver).
  • Config.BinaryPath string is replaced with an injectable Config.FS fs.FS. The
    binary is read from that filesystem and extracted before exec; when FS is nil
    (non-driver build), Open returns a no-op Driver. Behavior is preserved across
    both builds.
  • Tests drop their //go:build driver tag. They build a mock driver and inject it via
    os.DirFS, so the full subprocess lifecycle now runs in the default go test /
    ginkgo pass. The suite references the package's own driverName constant (via
    export_test.go) instead of re-deriving the binary name from runtime.GOOS.
  • CI/lint cleanup: .golangci.yaml drops the console/driver build-tags; the
    reusable test.go.yaml workflow drops the ginkgo_flags and prepare_assets inputs;
    test.core.yaml stops passing --tags driver; and the setup_assets.sh
    placeholder-binary script is deleted.

Release builds are unchanged — still built with -tags driver, with the real binary
dropped into the gitignored core/pkg/driver/assets/.

Basic Readiness

  • I have performed a self-review of my code.
  • I have added relevant, automated tests to cover the changes.
  • I have updated documentation to reflect the changes.

Greptile Summary

This PR refactors the Go driver package (core/pkg/driver) so that tests and linting work without the driver build tag. All runtime lifecycle logic moves into an untagged driver.go; only the defaultFS variable varies between build-tagged files (nil in disabled.go, embedded FS in the enabled_*.go pair). The old BinaryPath testing escape-hatch is replaced with an injectable Config.FS fs.FS, which tests satisfy by building a mock binary and injecting it via os.DirFS.

  • Build-tag cleanup: unix.go / windows.go drop the driver && constraint so driverName and configureSysProcAttr are always available; enabled_unix.go / enabled_windows.go introduce the new embed-only files gated behind driver && !windows / driver && windows.
  • CI/lint simplification: .golangci.yaml drops console/driver build-tags from the run section; test.go.yaml removes the ginkgo_flags and prepare_assets workflow inputs; setup_assets.sh is deleted entirely.
  • No-op path: When FS is nil (disabled build) or Enabled is false, Open returns a Driver whose Close is a no-op — behaviorally identical to the old stub type.

Confidence Score: 5/5

Safe to merge. The refactor is a straight code movement — no driver lifecycle logic changed, only where the logic lives in the source tree.

All lifecycle code is faithfully ported from the build-tagged enabled.go into the untagged driver.go. The enabled() guard replaces the dual-type approach with a nil-FS check, and config merging injects defaultFS as the base before user-supplied overrides. Both paths produce identical observable behaviour to before, and the test suite now exercises the full subprocess lifecycle without build tags.

No files require special attention.

Important Files Changed

Filename Overview
core/pkg/driver/driver.go Core refactor: all runtime driver logic moved here from tagged enabled.go. BinaryPath replaced with injectable FS; enabled() guards the subprocess path.
core/pkg/driver/enabled.go Stripped to var defaultFS = lo.Must(fs.Sub(embeddedAssets, assets)). ~280 lines of lifecycle code moved to driver.go.
core/pkg/driver/disabled.go Reduced to a single var defaultFS fs.FS (nil). All stubs removed — handled by the single Driver struct in driver.go.
.github/workflows/test.go.yaml Removed ginkgo_flags and prepare_assets inputs and the asset-placeholder step.
.golangci.yaml Removed build-tags run section; linter now covers all code in the default build.

Reviews (4): Last reviewed commit: "Remove unneeded context.Context" | Re-trigger Greptile

pjdotson added 28 commits May 8, 2026 21:04
…actor-go-driver-usage-so-linter-and-tests-dont-need
…actor-go-driver-usage-so-linter-and-tests-dont-need
…actor-go-driver-usage-so-linter-and-tests-dont-need
…actor-go-driver-usage-so-linter-and-tests-dont-need
…ver-usage-so-linter-and-tests-dont-need

# Conflicts:
#	core/pkg/driver/enabled_unix.go
#	core/pkg/driver/enabled_windows.go
#	core/pkg/driver/unix.go
#	core/pkg/driver/windows.go
@codecov

codecov Bot commented May 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.74%. Comparing base (69c2d98) to head (764562c).
⚠️ Report is 1 commits behind head on rc.

Additional details and impacted files
@@            Coverage Diff             @@
##               rc    #2383      +/-   ##
==========================================
+ Coverage   66.61%   66.74%   +0.13%     
==========================================
  Files        2477     2476       -1     
  Lines      116923   117419     +496     
  Branches     8695     8806     +111     
==========================================
+ Hits        77885    78377     +492     
- Misses      32734    32753      +19     
+ Partials     6304     6289      -15     
Flag Coverage Δ
alamos-go 55.25% <ø> (ø)
arc-go 77.45% <ø> (ø)
aspen 84.02% <ø> (-0.30%) ⬇️
cesium 82.73% <ø> (ø)
client-py 85.98% <ø> (ø)
client-ts 90.90% <ø> (ø)
console 23.08% <ø> (ø)
freighter-go 66.97% <ø> (+0.07%) ⬆️
freighter-integration 48.33% <ø> (ø)
oracle 63.05% <ø> (ø)
pluto 59.87% <ø> (+0.03%) ⬆️
x-go 82.24% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread core/pkg/driver/driver.go
Comment thread core/pkg/driver/enabled.go
@pjdotson pjdotson self-assigned this May 28, 2026
@pjdotson pjdotson changed the title SY-4167: Refactor Go Driver So Lint and Tests Don't Need Build Tags SY-4167: Refactor Go Driver so linting and tests don't need build tags May 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant