Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .github/workflows/lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,22 @@ jobs:
with:
files: coverage.out

test-sandbox-darwin:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[medium] architectural-fit

Adding a macOS runner for darwin-specific tests incurs significant CI cost (macOS runners are ~10x more expensive than Linux). The existing unit test at sandbox_test.go:418 verifies COPYFILE_DISABLE=1 is set via a fake tar shim. The new integration test validates real bsdtar behavior on macOS, which is orthogonal coverage. Consider whether this should run on every PR or on a schedule to reduce cost.

# Run Go tests on macOS to exercise darwin-specific behavior (e.g. bsdtar
# AppleDouble suppression via COPYFILE_DISABLE=1 in UploadDir).
runs-on: macos-latest
steps:
Comment thread
rh-hemartin marked this conversation as resolved.
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0

- uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6.4.0
with:
go-version-file: go.mod
Comment thread
rh-hemartin marked this conversation as resolved.

- run: go test -race ./internal/sandbox/...
env:
GH_TOKEN: ""
GITHUB_TOKEN: ""

commit-lint:
# Lint the PR title and individual commits on pull_request.
# Lint each commit on push/merge_group.
Expand Down
94 changes: 94 additions & 0 deletions internal/sandbox/sandbox_darwin_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
//go:build darwin

package sandbox

import (
"archive/tar"
"compress/gzip"
"io"
"os"
"os/exec"
"path/filepath"
"strings"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

// TestDarwinBsdtar_CopyfileDisableSuppressesAppleDouble exercises real macOS bsdtar
// to verify COPYFILE_DISABLE=1 prevents ._* files in tarballs. This validates OS-level

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] test-naming-convention

Test name TestDarwinBsdtar_CopyfileDisableSuppressesAppleDouble deviates from the dominant TestFunctionName_Scenario pattern in sandbox_test.go. The deviation is partially justified since this test exercises raw bsdtar behavior rather than a specific exported function.

// behavior; the companion TestUploadDir_TarIncludesCopyfileDisable (sandbox_test.go:418)
// verifies UploadDir sets the env var.
func TestDarwinBsdtar_CopyfileDisableSuppressesAppleDouble(t *testing.T) {
srcDir := t.TempDir()
testFile := filepath.Join(srcDir, "pack-abc.idx")
require.NoError(t, os.WriteFile(testFile, []byte("idx content"), 0o644))

xattrCmd := exec.Command("xattr", "-w", "com.apple.quarantine",
"0083;00000000;Safari;", testFile)
if out, err := xattrCmd.CombinedOutput(); err != nil {
t.Fatalf("xattr command failed (unexpected on macOS): %v: %s", err, out)
}

listTarMembers := func(tarPath string) []string {
t.Helper()
f, err := os.Open(tarPath)
require.NoError(t, err)
defer f.Close()
gz, err := gzip.NewReader(f)
require.NoError(t, err)
defer gz.Close()
tr := tar.NewReader(gz)
var members []string
for {
hdr, err := tr.Next()
if err == io.EOF {
break
}
require.NoError(t, err)
members = append(members, hdr.Name)
}
return members
}

hasAppleDouble := func(members []string) bool {
for _, m := range members {
if strings.HasPrefix(filepath.Base(m), "._") {
return true
}
}
return false
}

// Negative control: tar WITHOUT COPYFILE_DISABLE should produce ._* files.
controlEnv := make([]string, 0, len(os.Environ()))
for _, e := range os.Environ() {
if !strings.HasPrefix(e, "COPYFILE_DISABLE=") {
controlEnv = append(controlEnv, e)
}
}
controlTar := filepath.Join(t.TempDir(), "control.tar.gz")
controlCmd := exec.Command("tar", "-czf", controlTar, "-C", srcDir, ".")
controlCmd.Env = controlEnv
if out, err := controlCmd.CombinedOutput(); err != nil {
t.Fatalf("control tar failed: %v: %s", err, out)
}
// Subject: tar WITH COPYFILE_DISABLE=1 (matching UploadDir) must produce no ._* files.
// Run unconditionally — this is the actual assertion under test.
subjectTar := filepath.Join(t.TempDir(), "subject.tar.gz")
subjectCmd := exec.Command("tar", "-czf", subjectTar, "-C", srcDir, ".")
subjectCmd.Env = append(controlEnv, "COPYFILE_DISABLE=1")
Comment thread
rh-hemartin marked this conversation as resolved.
out, err := subjectCmd.CombinedOutput()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] test-adequacy

The subject assertion can pass trivially when the negative control also produces no ._* files. If the xattr does not trigger AppleDouble generation on the CI runner, the test provides zero signal. The negative control only emits t.Log (warning) rather than t.Skip, making the no-signal condition invisible in CI output.

Suggested fix: Replace t.Log("warning: ...") with t.Skip("control tar without COPYFILE_DISABLE produced no ._* files — skipping") so CI clearly reports when the test was not exercised.

require.NoError(t, err, "tar with COPYFILE_DISABLE=1 failed: %s", out)

subjectMembers := listTarMembers(subjectTar)
assert.False(t, hasAppleDouble(subjectMembers),
"tarball must contain no ._* members when COPYFILE_DISABLE=1; got: %v", subjectMembers)

// Bonus: verify the negative control produced ._* files, proving the test can detect regressions.
controlMembers := listTarMembers(controlTar)
if !hasAppleDouble(controlMembers) {
t.Log("warning: control tar without COPYFILE_DISABLE produced no ._* files — xattr may not have applied")
}
}
Loading