Enhance OS-specific browser tests, PR feedback

This commit addresses @babakks feedback in the PR of splitting out Windows and Unix/Linux specific browser tests from the shared common tests as well as minor restructuring of test setup / teardown logic.

Author: andyfeller

pkg/browser/browser.go

@@ -101,13 +101,13 @@ func isPossibleProtocol(u string) bool {
 		return true
 	}
 
-	// Disallow URLs that match existing files or directorys on the filesystem
-	if fileInfo, _ := os.Lstat(u); fileInfo != nil {
+	// Disallow URLs using alternative `file:` protocol not located previously
+	if strings.HasPrefix(u, "file:") {
 		return false
 	}
 
-	// Disallow URLs using alternative `file:` protocol not located previously
-	if strings.HasPrefix(u, "file:") {
+	// Disallow URLs that match existing files or directories on the filesystem
+	if fileInfo, _ := os.Lstat(u); fileInfo != nil {
 		return false
 	}
 

pkg/browser/browser_other_test.go

@@ -0,0 +1,63 @@
+//go:build !windows
+
+package browser
+
+import (
+	"bytes"
+	"fmt"
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestBrowseOthers covers unique Unix and Linux specific test cases that go beyond TestBrowse.
+func TestBrowseOthers(t *testing.T) {
+	tests := []struct {
+		name     string
+		url      string
+		launcher string
+		expected string
+		setup    func(*testing.T) error
+		wantErr  bool
+	}{
+		{
+			name:    "Explicit absolute `file://` URL errors",
+			url:     "file:///System/Applications/Calculator.app",
+			wantErr: true,
+		},
+		{
+			name:    "Implicit absolute file URL errors",
+			url:     "/bin/sh",
+			wantErr: true,
+		},
+		{
+			name:    "Implicit absolute directory URL errors",
+			url:     "/System/Applications/Calculator.app",
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.setup != nil {
+				err := tt.setup(t)
+				require.NoError(t, err)
+			}
+
+			stdout := &bytes.Buffer{}
+			stderr := &bytes.Buffer{}
+			b := Browser{launcher: tt.launcher, stdout: stdout, stderr: stderr}
+			err := b.browse(tt.url, []string{"GH_WANT_HELPER_PROCESS=1", fmt.Sprintf("GOCOVERDIR=%s", os.Getenv("GOCOVERDIR"))})
+
+			if tt.wantErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				assert.Equal(t, tt.expected, stdout.String())
+				assert.Equal(t, "", stderr.String())
+			}
+		})
+	}
+}

pkg/browser/browser_test.go

@@ -59,75 +59,60 @@ func TestBrowse(t *testing.T) {
 			launcher: fmt.Sprintf("%q -test.run=TestHelperProcess -- implicit https", os.Args[0]),
 			expected: "[implicit https github.com]",
 		},
-		{
-			name:    "Explicit absolute `file://` URL errors",
-			url:     "file:///System/Applications/Calculator.app",
-			wantErr: true,
-		},
-		{
-			name:    "Implicit absolute file URL errors",
-			url:     "/bin/sh",
-			wantErr: true,
-		},
-		{
-			name:    "Implicit absolute directory URL errors",
-			url:     "/System/Applications/Calculator.app",
-			wantErr: true,
-		},
-		{
-			name:    "Explicit absolute Windows file URL errors",
-			url:     `C:\Windows\System32\calc.exe`,
-			wantErr: true,
-		},
 		{
 			name: "Implicit relative file URL errors",
 			url:  "poc.command",
 			setup: func(t *testing.T) error {
-				// Capture current working directory for test cleanup
+				// Setup a temporary directory to stage content and execute the test within,
+				// ensure the test's original working directory is restored after.
 				cwd, err := os.Getwd()
 				if err != nil {
 					return err
 				}
 
-				// Create temporary directory containing relative executable for testing
 				tempDir := t.TempDir()
 				err = os.Chdir(tempDir)
 				if err != nil {
 					return err
 				}
 
+				t.Cleanup(func() {
+					_ = os.Chdir(cwd)
+				})
+
+				// Create content for local file URL testing
 				path := filepath.Join(tempDir, "poc.command")
 				err = os.WriteFile(path, []byte("#!/bin/bash\necho hello"), 0755)
 				if err != nil {
 					return err
 				}
 
-				// Restore original working directory after test
-				t.Cleanup(func() {
-					_ = os.Chdir(cwd)
-				})
-
 				return nil
 			},
 			wantErr: true,
 		},
 		{
 			name: "Implicit relative directory URL errors",
-			url:  "poc.command",
+			url:  "Fake.app",
 			setup: func(t *testing.T) error {
-				// Capture current working directory for test cleanup
+				// Setup a temporary directory to stage content and execute the test within,
+				// ensure the test's original working directory is restored after.
 				cwd, err := os.Getwd()
 				if err != nil {
 					return err
 				}
 
-				// Create temporary directory containing relative executable for testing
 				tempDir := t.TempDir()
 				err = os.Chdir(tempDir)
 				if err != nil {
 					return err
 				}
 
+				t.Cleanup(func() {
+					_ = os.Chdir(cwd)
+				})
+
+				// Create content for local directory URL testing
 				path := filepath.Join(tempDir, "Fake.app")
 				err = os.Mkdir(path, 0755)
 				if err != nil {
@@ -140,11 +125,6 @@ func TestBrowse(t *testing.T) {
 					return err
 				}
 
-				// Restore original working directory after test
-				t.Cleanup(func() {
-					_ = os.Chdir(cwd)
-				})
-
 				return nil
 			},
 			wantErr: true,
@@ -161,7 +141,7 @@ func TestBrowse(t *testing.T) {
 			stdout := &bytes.Buffer{}
 			stderr := &bytes.Buffer{}
 			b := Browser{launcher: tt.launcher, stdout: stdout, stderr: stderr}
-			err := b.browse(tt.url, []string{"GH_WANT_HELPER_PROCESS=1"})
+			err := b.browse(tt.url, []string{"GH_WANT_HELPER_PROCESS=1", fmt.Sprintf("GOCOVERDIR=%s", os.Getenv("GOCOVERDIR"))})
 
 			if tt.wantErr {
 				require.Error(t, err)

pkg/browser/browser_windows_test.go

@@ -0,0 +1,58 @@
+//go:build windows
+
+package browser
+
+import (
+	"bytes"
+	"fmt"
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestBrowseWindows covers unique Windows-specific test cases that go beyond TestBrowse.
+func TestBrowseWindows(t *testing.T) {
+	tests := []struct {
+		name     string
+		url      string
+		launcher string
+		expected string
+		setup    func(*testing.T) error
+		wantErr  bool
+	}{
+		{
+			name:    "Explicit absolute Windows file URL errors",
+			url:     `C:\Windows\System32\calc.exe`,
+			wantErr: true,
+		},
+		{
+			name:    "Explicit absolute Windows directory URL errors",
+			url:     `C:\Windows\System32`,
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.setup != nil {
+				err := tt.setup(t)
+				require.NoError(t, err)
+			}
+
+			stdout := &bytes.Buffer{}
+			stderr := &bytes.Buffer{}
+			b := Browser{launcher: tt.launcher, stdout: stdout, stderr: stderr}
+			err := b.browse(tt.url, []string{"GH_WANT_HELPER_PROCESS=1", fmt.Sprintf("GOCOVERDIR=%s", os.Getenv("GOCOVERDIR"))})
+
+			if tt.wantErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				assert.Equal(t, tt.expected, stdout.String())
+				assert.Equal(t, "", stderr.String())
+			}
+		})
+	}
+}