Add cross-platform CI, unit tests, and testing documentation#27
Add cross-platform CI, unit tests, and testing documentation#27
Conversation
Introduced TESTING.md with a comprehensive index of all tests, usage instructions, and CI integration details. Updated CLAUDE.md to require tests to be easily findable and a README.md to be maintained in the test directory.
Added multiple test targets to the Makefile for more granular control, including race detection, coverage, CI, and package-specific tests. Improved cross-platform support by differentiating test execution on macOS and Linux. Also added a target to list all test files and updated the default test and lint targets.
Introduces tests for getKeyTitle, getLegacyKeyTitle, and needsKeyRotation functions in cassh-menubar. These tests cover key title formatting, hostname truncation, key rotation timing, and consistency between legacy and new title formats.
Split tests into separate Linux and macOS jobs, ensuring platform-specific code is tested appropriately. Add concurrency control, aggregate coverage from both platforms, and upload to Codecov. Update build jobs to depend on both test jobs, and add a final CI success check for branch protection.
shawntz
added
documentation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR aims to establish comprehensive testing infrastructure for cassh by adding cross-platform CI, unit tests for key management functionality, enhanced Makefile test targets, and centralized testing documentation. However, there is a critical compilation issue that must be resolved before the PR can be merged.
Key changes:
- Adds unit tests for SSH key title generation and rotation logic in the menubar application
- Refactors GitHub Actions workflow to run tests separately on Linux and macOS with combined coverage reporting
- Expands Makefile with granular test targets (test-unit, test-coverage, test-ci, test-list, and package-specific targets)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
cmd/cassh-menubar/keytitle_test.go |
Adds unit tests for key title generation and rotation logic (but references non-existent functions) |
TESTING.md |
Creates comprehensive testing documentation with test index, CI/CD integration guide, and local testing instructions |
Makefile |
Enhances build system with platform-aware test targets and granular package-specific test commands |
CLAUDE.md |
Adds informal note about test indexing requirements |
.github/workflows/build.yml |
Refactors CI to separate Linux/macOS test jobs with combined coverage reporting and adds ci-success gate job |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Makefile
Outdated
| ifeq ($(shell uname),Darwin) | ||
| @echo "Running all tests (macOS)..." | ||
| CGO_ENABLED=1 go test -v ./... | ||
| else | ||
| @echo "Running cross-platform tests (Linux)..." | ||
| go test -v $$(go list ./... | grep -v /cmd/cassh-menubar) | ||
| endif |
There was a problem hiding this comment.
The Makefile uses uname to detect the platform, which works on Linux and macOS but will fail on Windows. While the PR description mentions Windows testing, the Makefile doesn't handle Windows at all (it would need different commands and uname isn't available by default). If Windows support is intended, consider adding detection for Windows or clarifying that Windows is not supported for local testing via the Makefile.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| @@ -0,0 +1,179 @@ | |||
| # cassh Test Documentation | |||
There was a problem hiding this comment.
The PR description mentions "Bug fix: Fixed null values in About dialog for version and commit hash display" but there are no changes to about_darwin.go or the version/buildCommit variable initialization in main.go shown in this PR. Either the bug fix is missing from this PR, or it was already fixed previously and should not be mentioned in this PR's description.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
cmd/cassh-menubar/keytitle_test.go
Outdated
| //go:build darwin | ||
|
|
||
| package main | ||
|
|
||
| import ( | ||
| "os" | ||
| "strings" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/shawntz/cassh/internal/config" | ||
| ) | ||
|
|
||
| func TestGetKeyTitle(t *testing.T) { | ||
| hostname, _ := os.Hostname() | ||
| expectedHostname := strings.ReplaceAll(hostname, ".", "-") | ||
| if len(expectedHostname) > 30 { | ||
| expectedHostname = expectedHostname[:30] | ||
| } | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| connID string | ||
| contains []string | ||
| }{ | ||
| { | ||
| name: "personal connection", | ||
| connID: "personal-1234567890", | ||
| contains: []string{"cassh-personal-1234567890@", expectedHostname}, | ||
| }, | ||
| { | ||
| name: "enterprise connection", | ||
| connID: "enterprise-9876543210", | ||
| contains: []string{"cassh-enterprise-9876543210@", expectedHostname}, | ||
| }, | ||
| { | ||
| name: "short ID", | ||
| connID: "test", | ||
| contains: []string{"cassh-test@", expectedHostname}, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| result := getKeyTitle(tt.connID) | ||
|
|
||
| for _, substr := range tt.contains { | ||
| if !strings.Contains(result, substr) { | ||
| t.Errorf("getKeyTitle(%q) = %q, should contain %q", tt.connID, result, substr) | ||
| } | ||
| } | ||
|
|
||
| // Verify format: cassh-{connID}@{hostname} | ||
| if !strings.HasPrefix(result, "cassh-") { | ||
| t.Errorf("getKeyTitle(%q) = %q, should start with 'cassh-'", tt.connID, result) | ||
| } | ||
|
|
||
| if !strings.Contains(result, "@") { | ||
| t.Errorf("getKeyTitle(%q) = %q, should contain '@'", tt.connID, result) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestGetLegacyKeyTitle(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| connID string | ||
| expected string | ||
| }{ | ||
| { | ||
| name: "personal connection", | ||
| connID: "personal-1234567890", | ||
| expected: "cassh-personal-1234567890", | ||
| }, | ||
| { | ||
| name: "enterprise connection", | ||
| connID: "enterprise-9876543210", | ||
| expected: "cassh-enterprise-9876543210", | ||
| }, | ||
| { | ||
| name: "short ID", | ||
| connID: "test", | ||
| expected: "cassh-test", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| result := getLegacyKeyTitle(tt.connID) | ||
| if result != tt.expected { | ||
| t.Errorf("getLegacyKeyTitle(%q) = %q, want %q", tt.connID, result, tt.expected) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestGetKeyTitleHostnameTruncation(t *testing.T) { | ||
| // Test that the function doesn't panic with any input | ||
| // and produces reasonable output | ||
| result := getKeyTitle("test-connection") | ||
|
|
||
| // Should not be empty | ||
| if result == "" { | ||
| t.Error("getKeyTitle() returned empty string") | ||
| } | ||
|
|
||
| // Should not exceed reasonable length (cassh- + connID + @ + 30 char hostname) | ||
| maxExpectedLen := len("cassh-test-connection@") + 30 | ||
| if len(result) > maxExpectedLen+10 { // Allow some margin | ||
| t.Errorf("getKeyTitle() result too long: %d chars, expected max ~%d", len(result), maxExpectedLen) | ||
| } | ||
| } | ||
|
|
||
| func TestNeedsKeyRotation(t *testing.T) { | ||
| now := time.Now() | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| conn *config.Connection | ||
| expected bool | ||
| }{ | ||
| { | ||
| name: "enterprise connection - never needs rotation", | ||
| conn: &config.Connection{ | ||
| Type: config.ConnectionTypeEnterprise, | ||
| KeyRotationHours: 4, | ||
| KeyCreatedAt: now.Add(-5 * time.Hour).Unix(), | ||
| }, | ||
| expected: false, | ||
| }, | ||
| { | ||
| name: "personal - no rotation configured", | ||
| conn: &config.Connection{ | ||
| Type: config.ConnectionTypePersonal, | ||
| KeyRotationHours: 0, | ||
| KeyCreatedAt: now.Add(-100 * time.Hour).Unix(), | ||
| }, | ||
| expected: false, | ||
| }, | ||
| { | ||
| name: "personal - no creation time recorded", | ||
| conn: &config.Connection{ | ||
| Type: config.ConnectionTypePersonal, | ||
| KeyRotationHours: 4, | ||
| KeyCreatedAt: 0, | ||
| }, | ||
| expected: false, | ||
| }, | ||
| { | ||
| name: "personal - key is fresh (created 1 hour ago, 4h rotation)", | ||
| conn: &config.Connection{ | ||
| Type: config.ConnectionTypePersonal, | ||
| KeyRotationHours: 4, | ||
| KeyCreatedAt: now.Add(-1 * time.Hour).Unix(), | ||
| }, | ||
| expected: false, | ||
| }, | ||
| { | ||
| name: "personal - key needs rotation (created 5 hours ago, 4h rotation)", | ||
| conn: &config.Connection{ | ||
| Type: config.ConnectionTypePersonal, | ||
| KeyRotationHours: 4, | ||
| KeyCreatedAt: now.Add(-5 * time.Hour).Unix(), | ||
| }, | ||
| expected: true, | ||
| }, | ||
| { | ||
| name: "personal - key exactly at rotation boundary", | ||
| conn: &config.Connection{ | ||
| Type: config.ConnectionTypePersonal, | ||
| KeyRotationHours: 4, | ||
| KeyCreatedAt: now.Add(-4 * time.Hour).Unix(), | ||
| }, | ||
| expected: true, | ||
| }, | ||
| { | ||
| name: "personal - key created 1 day ago with 24h rotation", | ||
| conn: &config.Connection{ | ||
| Type: config.ConnectionTypePersonal, | ||
| KeyRotationHours: 24, | ||
| KeyCreatedAt: now.Add(-24 * time.Hour).Unix(), | ||
| }, | ||
| expected: true, | ||
| }, | ||
| { | ||
| name: "personal - key created 23 hours ago with 24h rotation", | ||
| conn: &config.Connection{ | ||
| Type: config.ConnectionTypePersonal, | ||
| KeyRotationHours: 24, | ||
| KeyCreatedAt: now.Add(-23 * time.Hour).Unix(), | ||
| }, | ||
| expected: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| result := needsKeyRotation(tt.conn) | ||
| if result != tt.expected { | ||
| t.Errorf("needsKeyRotation() = %v, want %v", result, tt.expected) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestKeyTitleConsistency(t *testing.T) { | ||
| // Test that calling getKeyTitle multiple times returns the same result | ||
| connID := "personal-1234567890" | ||
|
|
||
| first := getKeyTitle(connID) | ||
| second := getKeyTitle(connID) | ||
|
|
||
| if first != second { | ||
| t.Errorf("getKeyTitle() not consistent: first=%q, second=%q", first, second) | ||
| } | ||
| } | ||
|
|
||
| func TestLegacyAndNewTitlesDifferent(t *testing.T) { | ||
| connID := "personal-1234567890" | ||
|
|
||
| newTitle := getKeyTitle(connID) | ||
| legacyTitle := getLegacyKeyTitle(connID) | ||
|
|
||
| if newTitle == legacyTitle { | ||
| t.Errorf("new and legacy titles should be different: new=%q, legacy=%q", newTitle, legacyTitle) | ||
| } | ||
|
|
||
| // Legacy should be a prefix of new (without the @hostname part) | ||
| if !strings.HasPrefix(newTitle, legacyTitle) { | ||
| t.Errorf("new title %q should start with legacy title %q", newTitle, legacyTitle) | ||
| } | ||
| } |
There was a problem hiding this comment.
This test file references functions that don't exist in the codebase. The tests call getKeyTitle(), getLegacyKeyTitle(), and needsKeyRotation(), but only needsKeyRotation() exists in main.go. The functions getKeyTitle() and getLegacyKeyTitle() are not defined anywhere, which will cause compilation failures. Either these functions need to be added to the codebase, or these tests should be removed/modified to test existing functionality.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
…n permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: shawntz <5200266+shawntz@users.noreply.github.com>
Co-authored-by: shawntz <5200266+shawntz@users.noreply.github.com>
Co-authored-by: shawntz <5200266+shawntz@users.noreply.github.com>
Co-authored-by: shawntz <5200266+shawntz@users.noreply.github.com>
Address PR #27 feedback: About dialog bug fix claim is incorrect
Remove test file referencing non-existent functions
|
fizzy |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Document Windows platform limitations in Makefile and add helpful error messages
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | Test Function | Description | | ||
| |--------------|-------------| | ||
| | `TestGetKeyTitle` | Tests SSH key title generation with hostname | | ||
| | `TestGetLegacyKeyTitle` | Tests legacy key title format (without hostname) | | ||
| | `TestGetKeyTitleHostnameTruncation` | Tests hostname truncation to 30 chars | | ||
| | `TestNeedsKeyRotation` | Tests key rotation detection logic | | ||
| | `TestKeyTitleConsistency` | Tests that key title generation is deterministic | | ||
| | `TestLegacyAndNewTitlesDifferent` | Tests that new format differs from legacy | | ||
|
|
||
| **Test Cases for `TestNeedsKeyRotation`:** | ||
| - Enterprise connection (never rotates) | ||
| - Personal with no rotation configured | ||
| - Personal with no creation time recorded | ||
| - Personal with fresh key (within rotation window) | ||
| - Personal with expired key (past rotation window) | ||
| - Personal at exact rotation boundary | ||
| - Various rotation hour configurations (4h, 24h) | ||
|
|
There was a problem hiding this comment.
The documentation references test file 'cmd/cassh-menubar/keytitle_test.go' and describes multiple test functions (TestGetKeyTitle, TestGetLegacyKeyTitle, TestGetKeyTitleHostnameTruncation, TestNeedsKeyRotation, TestKeyTitleConsistency, TestLegacyAndNewTitlesDifferent), but this test file does not exist in the repository. This section should either be removed or the actual test file should be added as part of this PR.
| | Test Function | Description | | |
| |--------------|-------------| | |
| | `TestGetKeyTitle` | Tests SSH key title generation with hostname | | |
| | `TestGetLegacyKeyTitle` | Tests legacy key title format (without hostname) | | |
| | `TestGetKeyTitleHostnameTruncation` | Tests hostname truncation to 30 chars | | |
| | `TestNeedsKeyRotation` | Tests key rotation detection logic | | |
| | `TestKeyTitleConsistency` | Tests that key title generation is deterministic | | |
| | `TestLegacyAndNewTitlesDifferent` | Tests that new format differs from legacy | | |
| **Test Cases for `TestNeedsKeyRotation`:** | |
| - Enterprise connection (never rotates) | |
| - Personal with no rotation configured | |
| - Personal with no creation time recorded | |
| - Personal with fresh key (within rotation window) | |
| - Personal with expired key (past rotation window) | |
| - Personal at exact rotation boundary | |
| - Various rotation hour configurations (4h, 24h) | |
| _Note: Detailed tests for macOS menubar key title behavior are not currently present in the repository. This section will be updated when those tests are added._ |
| ## Testing Guidelines | ||
|
|
||
| - Ensure that all tests are easily discoverable and indexable. | ||
| - Maintain an up-to-date `README.md` in the test directory that documents existing tests and is updated whenever new tests are added. No newline at end of file |
There was a problem hiding this comment.
The comment indicates this guideline refers to maintaining 'README.md in the test directory', but this PR adds TESTING.md in the root directory. This should be updated to reference TESTING.md or clarify the intended location.
| - Maintain an up-to-date `README.md` in the test directory that documents existing tests and is updated whenever new tests are added. | |
| - Maintain an up-to-date `TESTING.md` in the repository root that documents existing tests and is updated whenever new tests are added. |
| --- | ||
|
|
||
| *Last updated: 2024-12-10* | ||
| *Test count: 35 tests across 4 packages* |
There was a problem hiding this comment.
The test count claims '35 tests across 4 packages', but the documented tests only include internal/ca (12 tests), internal/config (11 tests), internal/memes (6 tests), and the non-existent cmd/cassh-menubar/keytitle_test.go (6 tests). Without the menubar tests that don't exist, this would be 29 tests across 3 packages. The count should be verified and corrected.
| *Test count: 35 tests across 4 packages* | |
| *Test count: 29 tests across 3 packages* |
| | `make test-ca` | Certificate Authority | | ||
| | `make test-config` | Configuration | | ||
| | `make test-memes` | Memes/Quotes | | ||
| | `make test-menubar` | macOS Menubar (macOS only) | |
There was a problem hiding this comment.
The Quick Reference table mentions 'make test-menubar' for testing the macOS Menubar package, but there are currently no test files in the cmd/cassh-menubar directory. While the Makefile target will succeed (go test returns 0 when there are no test files), users expecting to run menubar tests will not actually execute any tests. Consider either removing this entry until tests are added, or adding a note that no tests currently exist for this package.
| | `make test-menubar` | macOS Menubar (macOS only) | | |
| | `make test-menubar` | macOS Menubar (macOS only; no tests currently exist for this package) | |
Description
This PR significantly enhances the testing infrastructure for cassh by:
Type of Change
Related Issues
Closes #
How Has This Been Tested?
Test environment:
Checklist
Security Considerations
Screenshots (if applicable)