Repository navigation
ci: fix build workflow and add test workflow - #5
Ayush-Vish wants to merge 1 commit into
Conversation
- Updated `.github/workflows/main.yml`:
- Bumped Go version to 1.23 to match `go.mod`.
- Fixed artifact upload failure by ensuring unique artifact names per matrix job (`binaries-{os}-{arch}`).
- Added `.github/workflows/test.yml`:
- New workflow to run `go test -v ./...` on push and pull request events.
✅ Deploy Preview for shellsync canceled.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR fixes CI/CD workflow issues and introduces comprehensive test coverage for the shellsync project. The changes align the Go version with go.mod (1.23.9), resolve artifact upload conflicts in matrix builds, and establish a dedicated test workflow. Additionally, the PR adds the first unit tests for core components (client controller, websocket hub, and backend service), using mock implementations to test functionality in isolation.
Key Changes:
- Upgraded Go version from 1.21 to 1.23 in build workflow to match
go.mod - Fixed artifact name collisions by making them unique per OS/architecture combination
- Added automated test workflow that runs on push and pull requests
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/main.yml |
Updated Go version to 1.23 and fixed artifact naming to include matrix variables, preventing upload conflicts |
.github/workflows/test.yml |
New workflow for running go test -v ./... on push/PR to main branch |
client/controller/controller_test.go |
Added unit tests for NewAgent and getDefaultShell with cross-platform considerations |
backend/internal/websocket/hub_test.go |
Added comprehensive tests for websocket hub including connection handling, authentication, and message broadcasting |
backend/internal/service/service_test.go |
Added tests for core service methods including session management, terminal creation, and input forwarding |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Read auth success | ||
| var authResp types.Message | ||
| ws.ReadJSON(&authResp) |
There was a problem hiding this comment.
Error return values from ws.WriteJSON and ws.ReadJSON should be checked. While this is test code, ignoring errors can lead to confusing test failures if the WebSocket communication fails.
| ws.ReadJSON(&authResp) | |
| if err := ws.ReadJSON(&authResp); err != nil { | |
| t.Fatalf("ReadJSON (authResp) failed: %v", err) | |
| } |
|
|
||
| // Read session state (sent automatically after register) | ||
| var stateResp types.Message | ||
| ws.ReadJSON(&stateResp) |
There was a problem hiding this comment.
Error return value from ws.ReadJSON should be checked. Ignoring this error could cause the test to fail in confusing ways if the session state message is not received as expected.
| ws.ReadJSON(&stateResp) | |
| if err := ws.ReadJSON(&stateResp); err != nil { | |
| t.Fatalf("ReadJSON (session_state) failed: %v", err) | |
| } |
| // Read session state (sent automatically after register) | ||
| var stateResp types.Message | ||
| ws.ReadJSON(&stateResp) | ||
| if stateResp.Type != "session_state" { | ||
| // It might be possible we get the broadcast before the state if we are too fast? | ||
| // But in registerClient, session state is sent in a goroutine with sleep. | ||
| // So likely we receive it. |
There was a problem hiding this comment.
[nitpick] The comment mentions uncertainty about message ordering but doesn't handle the case where session_state is not received. Consider adding a more robust check or timeout mechanism to verify the session state message is received before proceeding with the broadcast test. For example:
// Read session state with timeout handling
var stateResp types.Message
if err := ws.ReadJSON(&stateResp); err != nil {
t.Logf("Warning: Failed to read session state: %v", err)
}| // Read session state (sent automatically after register) | |
| var stateResp types.Message | |
| ws.ReadJSON(&stateResp) | |
| if stateResp.Type != "session_state" { | |
| // It might be possible we get the broadcast before the state if we are too fast? | |
| // But in registerClient, session state is sent in a goroutine with sleep. | |
| // So likely we receive it. | |
| // Read session state (sent automatically after register) with timeout handling | |
| var stateResp types.Message | |
| ws.SetReadDeadline(time.Now().Add(1 * time.Second)) | |
| err := ws.ReadJSON(&stateResp) | |
| ws.SetReadDeadline(time.Time{}) // Clear deadline for further reads | |
| if err != nil { | |
| t.Fatalf("Failed to read session_state message: %v", err) | |
| } | |
| if stateResp.Type != "session_state" { | |
| t.Logf("Warning: Expected session_state, got %s", stateResp.Type) |
| // Wait a bit to ensure client is fully registered | ||
| time.Sleep(200 * time.Millisecond) | ||
|
|
There was a problem hiding this comment.
[nitpick] Using a fixed time.Sleep(200 * time.Millisecond) to wait for client registration can make tests flaky under load or in slower CI environments. Consider using a more deterministic synchronization mechanism, such as a channel-based signal or a polling loop with timeout. For example:
// Poll for client registration with timeout
timeout := time.After(1 * time.Second)
ticker := time.NewTicker(10 * time.Millisecond)
defer ticker.Stop()
registered := false
for !registered {
select {
case <-timeout:
t.Fatal("Timeout waiting for client registration")
case <-ticker.C:
hub.mu.RLock()
_, registered = hub.clients["client1"]
hub.mu.RUnlock()
}
}| // Wait a bit to ensure client is fully registered | |
| time.Sleep(200 * time.Millisecond) | |
| // Wait deterministically for client registration | |
| timeout := time.After(1 * time.Second) | |
| ticker := time.NewTicker(10 * time.Millisecond) | |
| defer ticker.Stop() | |
| registered := false | |
| for !registered { | |
| select { | |
| case <-timeout: | |
| t.Fatal("Timeout waiting for client registration") | |
| case <-ticker.C: | |
| hub.mu.RLock() | |
| _, registered = hub.clients["client1"] | |
| hub.mu.RUnlock() | |
| } | |
| } |
.github/workflows/main.yml:go.mod.binaries-{os}-{arch})..github/workflows/test.yml:go test -v ./...on push and pull request events.