From 83e89a9c9a8ecb41b44109f700f74379fb5c3b0d Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Thu, 6 Feb 2025 16:43:56 -0800 Subject: [PATCH 1/4] Move duplicated stubConfig function to a new internal/testutils dir --- internal/testutils/config_stub.go | 21 +++++++++++++++++++++ pkg/api/client_options_test.go | 3 ++- pkg/api/graphql_client_test.go | 9 +++++---- pkg/api/http_client_test.go | 15 ++------------- pkg/api/rest_client_test.go | 3 ++- pkg/repository/repository_test.go | 17 +++-------------- 6 files changed, 35 insertions(+), 33 deletions(-) create mode 100644 internal/testutils/config_stub.go diff --git a/internal/testutils/config_stub.go b/internal/testutils/config_stub.go new file mode 100644 index 0000000..ad5eeb2 --- /dev/null +++ b/internal/testutils/config_stub.go @@ -0,0 +1,21 @@ +package testutils + +import ( + "testing" + + "github.com/cli/go-gh/v2/pkg/config" +) + +// StubConfig replaces the config.Read function with a function that returns a config object +// created from the given config string. It also sets up a cleanup function that restores the +// original config.Read function. +func StubConfig(t *testing.T, cfgStr string) { + t.Helper() + old := config.Read + config.Read = func(_ *config.Config) (*config.Config, error) { + return config.ReadFromString(cfgStr), nil + } + t.Cleanup(func() { + config.Read = old + }) +} diff --git a/pkg/api/client_options_test.go b/pkg/api/client_options_test.go index 237b9d5..35cfb34 100644 --- a/pkg/api/client_options_test.go +++ b/pkg/api/client_options_test.go @@ -4,11 +4,12 @@ import ( "net/http" "testing" + "github.com/cli/go-gh/v2/internal/testutils" "github.com/stretchr/testify/assert" ) func TestResolveOptions(t *testing.T) { - stubConfig(t, testConfigWithSocket()) + testutils.StubConfig(t, testConfigWithSocket()) tests := []struct { name string diff --git a/pkg/api/graphql_client_test.go b/pkg/api/graphql_client_test.go index b27d38f..15cdc7a 100644 --- a/pkg/api/graphql_client_test.go +++ b/pkg/api/graphql_client_test.go @@ -7,12 +7,13 @@ import ( "testing" "time" + "github.com/cli/go-gh/v2/internal/testutils" "github.com/stretchr/testify/assert" "gopkg.in/h2non/gock.v1" ) func TestGraphQLClient(t *testing.T) { - stubConfig(t, testConfig()) + testutils.StubConfig(t, testConfig()) t.Cleanup(gock.Off) gock.New("https://api.github.com"). @@ -34,7 +35,7 @@ func TestGraphQLClient(t *testing.T) { } func TestGraphQLClientDoError(t *testing.T) { - stubConfig(t, testConfig()) + testutils.StubConfig(t, testConfig()) t.Cleanup(gock.Off) gock.New("https://api.github.com"). @@ -56,7 +57,7 @@ func TestGraphQLClientDoError(t *testing.T) { } func TestGraphQLClientQueryError(t *testing.T) { - stubConfig(t, testConfig()) + testutils.StubConfig(t, testConfig()) t.Cleanup(gock.Off) gock.New("https://api.github.com"). @@ -78,7 +79,7 @@ func TestGraphQLClientQueryError(t *testing.T) { } func TestGraphQLClientMutateError(t *testing.T) { - stubConfig(t, testConfig()) + testutils.StubConfig(t, testConfig()) t.Cleanup(gock.Off) gock.New("https://api.github.com"). diff --git a/pkg/api/http_client_test.go b/pkg/api/http_client_test.go index 9fe0934..19c911d 100644 --- a/pkg/api/http_client_test.go +++ b/pkg/api/http_client_test.go @@ -8,13 +8,13 @@ import ( "strings" "testing" - "github.com/cli/go-gh/v2/pkg/config" + "github.com/cli/go-gh/v2/internal/testutils" "github.com/stretchr/testify/assert" "gopkg.in/h2non/gock.v1" ) func TestHTTPClient(t *testing.T) { - stubConfig(t, testConfig()) + testutils.StubConfig(t, testConfig()) t.Cleanup(gock.Off) gock.New("https://api.github.com"). @@ -177,17 +177,6 @@ func defaultHeaders() http.Header { return h } -func stubConfig(t *testing.T, cfgStr string) { - t.Helper() - old := config.Read - config.Read = func(_ *config.Config) (*config.Config, error) { - return config.ReadFromString(cfgStr), nil - } - t.Cleanup(func() { - config.Read = old - }) -} - func printPendingMocks(mocks []gock.Mock) string { paths := []string{} for _, mock := range mocks { diff --git a/pkg/api/rest_client_test.go b/pkg/api/rest_client_test.go index bc2917e..51ac16a 100644 --- a/pkg/api/rest_client_test.go +++ b/pkg/api/rest_client_test.go @@ -8,12 +8,13 @@ import ( "testing" "time" + "github.com/cli/go-gh/v2/internal/testutils" "github.com/stretchr/testify/assert" "gopkg.in/h2non/gock.v1" ) func TestRESTClient(t *testing.T) { - stubConfig(t, testConfig()) + testutils.StubConfig(t, testConfig()) t.Cleanup(gock.Off) gock.New("https://api.github.com"). diff --git a/pkg/repository/repository_test.go b/pkg/repository/repository_test.go index dfcdca7..0430eab 100644 --- a/pkg/repository/repository_test.go +++ b/pkg/repository/repository_test.go @@ -3,12 +3,12 @@ package repository import ( "testing" - "github.com/cli/go-gh/v2/pkg/config" + "github.com/cli/go-gh/v2/internal/testutils" "github.com/stretchr/testify/assert" ) func TestParse(t *testing.T) { - stubConfig(t, "") + testutils.StubConfig(t, "") tests := []struct { name string @@ -106,7 +106,7 @@ hosts: oauth_token: yyyyyyyyyyyyyyyyyyyy git_protocol: https ` - stubConfig(t, cfgStr) + testutils.StubConfig(t, cfgStr) r, err := Parse("OWNER/REPO") assert.NoError(t, err) assert.Equal(t, "enterprise.com", r.Host) @@ -189,14 +189,3 @@ func TestParseWithHost(t *testing.T) { }) } } - -func stubConfig(t *testing.T, cfgStr string) { - t.Helper() - old := config.Read - config.Read = func(_ *config.Config) (*config.Config, error) { - return config.ReadFromString(cfgStr), nil - } - t.Cleanup(func() { - config.Read = old - }) -} From 65cf7dbe3fb2e79cf74a889702737c12eae9b263 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Thu, 6 Feb 2025 16:44:33 -0800 Subject: [PATCH 2/4] Add new accessibility package and IsEnabled() function --- pkg/accessibility/accessibility.go | 29 ++++++++ pkg/accessibility/accessibility_test.go | 91 +++++++++++++++++++++++++ 2 files changed, 120 insertions(+) create mode 100644 pkg/accessibility/accessibility.go create mode 100644 pkg/accessibility/accessibility_test.go diff --git a/pkg/accessibility/accessibility.go b/pkg/accessibility/accessibility.go new file mode 100644 index 0000000..f97bd53 --- /dev/null +++ b/pkg/accessibility/accessibility.go @@ -0,0 +1,29 @@ +package accessibility + +import ( + "os" + + "github.com/cli/go-gh/v2/pkg/config" +) + +const ACCESSIBILITY_ENV = "ACCESSIBILITY" + +// IsEnabled returns true if accessibility features are enabled via the ACCESSIBILITY +// environment variable or the configuration file. +func IsEnabled() bool { + envVar := os.Getenv(ACCESSIBILITY_ENV) + if envVar != "" { + return isEnvVarEnabled(envVar) + } + + // We are not handling errors because we don't want to fail if the config is not + // read. Instead, we assume an empty configuration is equivalent to "disabled". + cfg, _ := config.Read(nil) + accessibleConfigValue, _ := cfg.Get([]string{"accessible"}) + + return accessibleConfigValue == "enabled" +} + +func isEnvVarEnabled(envVar string) bool { + return envVar != "" && envVar != "0" && envVar != "false" +} diff --git a/pkg/accessibility/accessibility_test.go b/pkg/accessibility/accessibility_test.go new file mode 100644 index 0000000..55470e6 --- /dev/null +++ b/pkg/accessibility/accessibility_test.go @@ -0,0 +1,91 @@ +package accessibility + +import ( + "testing" + + "github.com/cli/go-gh/v2/internal/testutils" + "github.com/stretchr/testify/assert" +) + +func TestIsEnabled(t *testing.T) { + tests := []struct { + name string + envVarValue string + cfgStr string + wantOut bool + }{ + { + name: "When the accessibility configuration and env var are both unset, it should return false", + envVarValue: "", + cfgStr: "", + wantOut: false, + }, + { + name: "When the accessibility configuration is unset but the ACCESSIBLE env var is set to something truthy (not '0' or 'false'), it should return true", + envVarValue: "1", + cfgStr: "", + wantOut: true, + }, + { + name: "When the accessibility configuration is unset and the ACCESSIBLE env var returns '0', it should return false", + envVarValue: "0", + cfgStr: "", + wantOut: false, + }, + { + name: "When the accessibility configuration is unset and the ACCESSIBLE env var returns 'false', it should return false", + envVarValue: "0", + cfgStr: "", + wantOut: false, + }, + { + name: "When the accessibility configuration is set to enabled and the env var is unset, it should return true", + envVarValue: "", + cfgStr: accessibilityEnabledConfig(), + wantOut: true, + }, + { + name: "When the accessibility configuration is set to disabled and the env var is unset, it should return false", + envVarValue: "", + cfgStr: accessibilityDisabledConfig(), + wantOut: false, + }, + { + name: "When the accessibility configuration is set to disabled and the env var is set to something truthy (not '0' or 'false'), it should return true", + envVarValue: "true", + cfgStr: accessibilityDisabledConfig(), + wantOut: true, + }, + { + name: "When the accessibility configuration is set to enabled and the env var is set to '0', it should return false", + envVarValue: "false", + cfgStr: accessibilityEnabledConfig(), + wantOut: false, + }, + { + name: "When the accessibility configuration is set to enabled and the env var is set to 'false', it should return false", + envVarValue: "false", + cfgStr: accessibilityEnabledConfig(), + wantOut: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("ACCESSIBILITY", tt.envVarValue) + testutils.StubConfig(t, tt.cfgStr) + assert.Equal(t, tt.wantOut, IsEnabled()) + }) + } +} + +func accessibilityEnabledConfig() string { + return ` +accessible: enabled +` +} + +func accessibilityDisabledConfig() string { + return ` +accessible: disabled +` +} From 794568b68f54ef0023ea4f9cf9ced63cdc8cdf3d Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Fri, 7 Feb 2025 08:15:56 -0800 Subject: [PATCH 3/4] Address PR comments --- pkg/accessibility/accessibility.go | 6 +++++- pkg/accessibility/accessibility_test.go | 6 +++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/pkg/accessibility/accessibility.go b/pkg/accessibility/accessibility.go index f97bd53..e50a478 100644 --- a/pkg/accessibility/accessibility.go +++ b/pkg/accessibility/accessibility.go @@ -6,7 +6,11 @@ import ( "github.com/cli/go-gh/v2/pkg/config" ) -const ACCESSIBILITY_ENV = "ACCESSIBILITY" +// ACCESSIBILITY_ENV is the name of the environment variable that can be used to enable +// accessibility features. If the value is empty, "0", or "false", the accessibility +// features are disabled. Any other value enables the accessibility features. Note that +// this environment variable supercedes the configuration file's accessible setting. +const ACCESSIBILITY_ENV = "GH_ACCESSIBILE" // IsEnabled returns true if accessibility features are enabled via the ACCESSIBILITY // environment variable or the configuration file. diff --git a/pkg/accessibility/accessibility_test.go b/pkg/accessibility/accessibility_test.go index 55470e6..cc3234a 100644 --- a/pkg/accessibility/accessibility_test.go +++ b/pkg/accessibility/accessibility_test.go @@ -34,7 +34,7 @@ func TestIsEnabled(t *testing.T) { }, { name: "When the accessibility configuration is unset and the ACCESSIBLE env var returns 'false', it should return false", - envVarValue: "0", + envVarValue: "false", cfgStr: "", wantOut: false, }, @@ -58,7 +58,7 @@ func TestIsEnabled(t *testing.T) { }, { name: "When the accessibility configuration is set to enabled and the env var is set to '0', it should return false", - envVarValue: "false", + envVarValue: "0", cfgStr: accessibilityEnabledConfig(), wantOut: false, }, @@ -71,7 +71,7 @@ func TestIsEnabled(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - t.Setenv("ACCESSIBILITY", tt.envVarValue) + t.Setenv("GH_ACCESSIBILE", tt.envVarValue) testutils.StubConfig(t, tt.cfgStr) assert.Equal(t, tt.wantOut, IsEnabled()) }) From 50d7ff401103c2a8cf4c3cd143cd92ff92a4b261 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Fri, 7 Feb 2025 09:54:42 -0800 Subject: [PATCH 4/4] Change GH_ACCESSIBLE env var to ACCESSIBLE --- pkg/accessibility/accessibility.go | 2 +- pkg/accessibility/accessibility_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/accessibility/accessibility.go b/pkg/accessibility/accessibility.go index e50a478..b99e7e3 100644 --- a/pkg/accessibility/accessibility.go +++ b/pkg/accessibility/accessibility.go @@ -10,7 +10,7 @@ import ( // accessibility features. If the value is empty, "0", or "false", the accessibility // features are disabled. Any other value enables the accessibility features. Note that // this environment variable supercedes the configuration file's accessible setting. -const ACCESSIBILITY_ENV = "GH_ACCESSIBILE" +const ACCESSIBILITY_ENV = "ACCESSIBILE" // IsEnabled returns true if accessibility features are enabled via the ACCESSIBILITY // environment variable or the configuration file. diff --git a/pkg/accessibility/accessibility_test.go b/pkg/accessibility/accessibility_test.go index cc3234a..1bd4f80 100644 --- a/pkg/accessibility/accessibility_test.go +++ b/pkg/accessibility/accessibility_test.go @@ -71,7 +71,7 @@ func TestIsEnabled(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - t.Setenv("GH_ACCESSIBILE", tt.envVarValue) + t.Setenv("ACCESSIBILE", tt.envVarValue) testutils.StubConfig(t, tt.cfgStr) assert.Equal(t, tt.wantOut, IsEnabled()) })