Skip to content

Fix Windows-incompatible tests and hook execution - #53

Merged
TimothyYe merged 2 commits into
TimothyYe:masterfrom
R0GUEEE:r0gueee-studious-guacamole
Aug 2, 2026
Merged

Fix Windows-incompatible tests and hook execution#53
TimothyYe merged 2 commits into
TimothyYe:masterfrom
R0GUEEE:r0gueee-studious-guacamole

Conversation

@R0GUEEE

@R0GUEEE R0GUEEE commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

The test suite previously failed on Windows, and the pluggable hook feature silently never fired there either. This change makes the whole suite pass on Windows while keeping Unix behavior intact.

Approach

  • Hook scripts are now run through a POSIX shell (sh) on Windows, where shebang scripts cannot be executed directly by the OS. On Unix, hooks execute directly as before.
  • isExecutableFile treats any regular file as a hook on Windows, since Windows has no POSIX executable permission bits. This is what made hooks (and several hook tests) silently no-op before.
  • Tests that relied on Unix-only tools (ls, touch, mkdir, printf, /etc/passwd) were rewritten to be platform-neutral:
    • TestExecute uses cmd /c exit 0/1 on Windows, true/false on Unix.
    • TestParsePath symlinks a temp file instead of /etc/passwd.
    • Key-setup tests use os.MkdirAll/os.WriteFile instead of shelling out.
    • Hook test scripts redirect to forward-slash paths (filepath.ToSlash) so they work when run through sh.
    • TestBuildKeygenArgs_DefaultsAndShape computes expected paths with filepath.Join.
    • TestResolveToken uses cmd /c echo on Windows instead of the printf shell builtin.

Note for reviewers

  • Hook execution on Windows requires a POSIX sh on PATH (e.g. Git Bash / MSYS / WSL). If none is present, hook scripts fall back to direct execution and report their failure via the existing error path.
  • Verified with go build ./..., go vet ./..., and go test ./... (all packages pass), plus an skm.exe --version smoke test.

Make the test suite pass on Windows:
- Run hook scripts through a POSIX shell on Windows, where shebang
  scripts cannot be executed directly, and treat regular hook files as
  executable since Windows has no exec permission bits
- Use Windows-compatible commands in tests (cmd /c instead of ls,
  mkdir, touch, printf) and replace /etc/passwd with a temp file
- Compute expected keygen paths with filepath.Join so expectations match
  output on any platform

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 2, 2026 05:09

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR aims to make hook execution and the Go test suite behave correctly on Windows, while preserving Unix behavior—primarily by adjusting hook executability detection/dispatch and rewriting several tests to avoid Unix-only dependencies.

Changes:

  • Treat hook files as runnable on Windows (no POSIX exec bits) and run shebang hooks via sh when available.
  • Refactor multiple tests to use platform-neutral commands and filesystem setup (cmd /c ... on Windows; os.MkdirAll/os.WriteFile instead of shelling out).
  • Normalize hook test script paths (e.g., filepath.ToSlash) and expected path construction (filepath.Join) to reduce OS-specific assumptions.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
internal/utils/utils.go Updates hook executability rules on Windows and routes hook execution through sh when present.
internal/utils/utils_test.go Reworks tests to be platform-neutral and updates hook script generation for Windows/path handling.
internal/publishers/publishers_test.go Makes CLI fallback token resolution test work on Windows by avoiding POSIX printf.
internal/actions/actions_test.go Uses filepath.Join for expected keygen output paths to match OS-specific separators.
Suppressed comments (2)

internal/utils/utils_test.go:481

  • These shell snippets redirect to the log file without quoting it. If the path contains spaces, the hook will fail when run via sh (common on Windows). Quote the redirection target.
	writeGlobal := "#!/bin/sh\necho G $SKM_EVENT $SKM_ALIAS >> " + logSlash + "\n"
	writePerKey := "#!/bin/sh\necho K $SKM_EVENT $SKM_ALIAS >> " + logSlash + "\n"

internal/utils/utils_test.go:568

  • This hook body redirects to the log file without quoting the path, which can fail when the temp/store path contains spaces. Quote the redirection target so the test is robust across platforms.
	body := "#!/bin/sh\necho \"$SKM_REMOTE_HOST $SKM_REMOTE_PORT\" >> " + filepath.ToSlash(log) + "\n"

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/utils/utils_test.go
Comment thread internal/utils/utils_test.go
Comment thread internal/utils/utils_test.go
- Include expected/actual values in the ParsePath assertion failure.
- Skip the symlink-resolution check on Windows when symlink creation fails.
- Quote the log file path in generated hook script bodies so paths containing spaces work under sh.
@TimothyYe
TimothyYe merged commit f7e87b8 into TimothyYe:master Aug 2, 2026
1 check passed
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.

3 participants