Skip to content

Commit 83025f0

Browse files
jai-deepsourcejai-deepsource
committed
Refactor code quality and improve test patterns
- Use underscore prefix for unused cobra command parameters - Refactor logout to use struct fields instead of package-level vars - Fix mutex double-unlock issues in status_test.go - Replace deprecated ioutil.ReadFile with os.ReadFile - Convert value receivers to pointer receivers where appropriate - Extract setupGitRepo helper and use table-driven tests - Improve error handling with io.ReadAll and json.NewEncoder - Add configFilePermissions constant and clean up config.go - Use DialTimeout for server readiness checks - Fix test loop variable capture issues
1 parent e53a145 commit 83025f0

28 files changed

Lines changed: 213 additions & 316 deletions

File tree

‎command/auth/login/login.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ func NewCmdLogin() *cobra.Command {
5656
Short: "Log in to DeepSource using Command Line Interface",
5757
Long: doc,
5858
Args: utils.NoArgs,
59-
RunE: func(cmd *cobra.Command, args []string) error {
59+
RunE: func(_ *cobra.Command, _ []string) error {
6060
return opts.Run()
6161
},
6262
}

‎command/auth/login/pat_login_flow.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import (
77
)
88

99
// Starts the login flow for the CLI (using PAT)
10-
func (opts *LoginOptions) startPATLoginFlow(cfg *config.CLIConfig, token string) error {
10+
func (*LoginOptions) startPATLoginFlow(cfg *config.CLIConfig, token string) error {
1111
// set personal access token (PAT)
1212
cfg.Token = token
1313

‎command/auth/logout/logout.go‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,22 +10,22 @@ import (
1010
"github.com/spf13/cobra"
1111
)
1212

13-
// Function variables for testing
14-
var (
15-
getConfigFn = config.GetConfig
16-
confirmFromUserFn = utils.ConfirmFromUser
17-
)
18-
19-
type LogoutOptions struct{}
13+
type LogoutOptions struct {
14+
GetConfigFn func() (*config.CLIConfig, error)
15+
ConfirmFromUserFn func(string, string) (bool, error)
16+
}
2017

2118
// NewCmdLogout handles the logout functionality for the CLI
2219
func NewCmdLogout() *cobra.Command {
2320
cmd := &cobra.Command{
2421
Use: "logout",
2522
Short: "Logout of your active DeepSource account",
2623
Args: utils.NoArgs,
27-
RunE: func(cmd *cobra.Command, args []string) error {
28-
opts := LogoutOptions{}
24+
RunE: func(_ *cobra.Command, _ []string) error {
25+
opts := &LogoutOptions{
26+
GetConfigFn: config.GetConfig,
27+
ConfirmFromUserFn: utils.ConfirmFromUser,
28+
}
2929
return opts.Run()
3030
},
3131
}
@@ -34,7 +34,7 @@ func NewCmdLogout() *cobra.Command {
3434

3535
func (opts *LogoutOptions) Run() error {
3636
// Fetch config
37-
cfg, err := getConfigFn()
37+
cfg, err := opts.GetConfigFn()
3838
if err != nil {
3939
return fmt.Errorf("Error while reading DeepSource CLI config : %v", err)
4040
}
@@ -45,7 +45,7 @@ func (opts *LogoutOptions) Run() error {
4545

4646
// Confirm from the user if they want to logout
4747
logoutConfirmationMsg := "Are you sure you want to log out of DeepSource account?"
48-
response, err := confirmFromUserFn(logoutConfirmationMsg, "")
48+
response, err := opts.ConfirmFromUserFn(logoutConfirmationMsg, "")
4949
if err != nil {
5050
return err
5151
}

‎command/auth/logout/logout_test.go‎

Lines changed: 28 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,12 @@ package logout
22

33
import (
44
"errors"
5-
"sync"
65
"testing"
76

87
"github.com/deepsourcelabs/cli/config"
98
"github.com/stretchr/testify/assert"
109
)
1110

12-
var mockMu sync.Mutex
13-
1411
func TestNewCmdLogout(t *testing.T) {
1512
t.Run("creates command with correct properties", func(t *testing.T) {
1613
cmd := NewCmdLogout()
@@ -21,91 +18,55 @@ func TestNewCmdLogout(t *testing.T) {
2118

2219
func TestLogoutOptions_Run(t *testing.T) {
2320
t.Run("error reading config", func(t *testing.T) {
24-
mockMu.Lock()
25-
originalGetConfig := getConfigFn
26-
getConfigFn = func() (*config.CLIConfig, error) {
27-
return nil, errors.New("config read error")
21+
opts := &LogoutOptions{
22+
GetConfigFn: func() (*config.CLIConfig, error) {
23+
return nil, errors.New("config read error")
24+
},
25+
ConfirmFromUserFn: func(msg, helpText string) (bool, error) {
26+
return false, nil
27+
},
2828
}
29-
mockMu.Unlock()
30-
31-
defer func() {
32-
mockMu.Lock()
33-
getConfigFn = originalGetConfig
34-
mockMu.Unlock()
35-
}()
36-
37-
opts := LogoutOptions{}
3829
err := opts.Run()
3930
assert.Error(t, err)
4031
assert.Contains(t, err.Error(), "Error while reading DeepSource CLI config")
4132
})
4233

4334
t.Run("not logged in - empty token", func(t *testing.T) {
44-
mockMu.Lock()
45-
originalGetConfig := getConfigFn
46-
getConfigFn = func() (*config.CLIConfig, error) {
47-
return &config.CLIConfig{Token: ""}, nil
35+
opts := &LogoutOptions{
36+
GetConfigFn: func() (*config.CLIConfig, error) {
37+
return &config.CLIConfig{Token: ""}, nil
38+
},
39+
ConfirmFromUserFn: func(msg, helpText string) (bool, error) {
40+
return false, nil
41+
},
4842
}
49-
mockMu.Unlock()
50-
51-
defer func() {
52-
mockMu.Lock()
53-
getConfigFn = originalGetConfig
54-
mockMu.Unlock()
55-
}()
56-
57-
opts := LogoutOptions{}
5843
err := opts.Run()
5944
assert.Error(t, err)
6045
assert.Contains(t, err.Error(), "not logged into DeepSource")
6146
})
6247

6348
t.Run("user cancels logout", func(t *testing.T) {
64-
mockMu.Lock()
65-
originalGetConfig := getConfigFn
66-
originalConfirm := confirmFromUserFn
67-
68-
getConfigFn = func() (*config.CLIConfig, error) {
69-
return &config.CLIConfig{Token: "valid_token"}, nil
70-
}
71-
confirmFromUserFn = func(msg, helpText string) (bool, error) {
72-
return false, nil // User cancels
49+
opts := &LogoutOptions{
50+
GetConfigFn: func() (*config.CLIConfig, error) {
51+
return &config.CLIConfig{Token: "valid_token"}, nil
52+
},
53+
ConfirmFromUserFn: func(msg, helpText string) (bool, error) {
54+
return false, nil // User cancels
55+
},
7356
}
74-
mockMu.Unlock()
75-
76-
defer func() {
77-
mockMu.Lock()
78-
getConfigFn = originalGetConfig
79-
confirmFromUserFn = originalConfirm
80-
mockMu.Unlock()
81-
}()
82-
83-
opts := LogoutOptions{}
8457
err := opts.Run()
8558
assert.NoError(t, err)
8659
})
8760

8861
t.Run("confirmation prompt returns error", func(t *testing.T) {
89-
mockMu.Lock()
90-
originalGetConfig := getConfigFn
91-
originalConfirm := confirmFromUserFn
92-
93-
getConfigFn = func() (*config.CLIConfig, error) {
94-
return &config.CLIConfig{Token: "valid_token"}, nil
62+
opts := &LogoutOptions{
63+
GetConfigFn: func() (*config.CLIConfig, error) {
64+
return &config.CLIConfig{Token: "valid_token"}, nil
65+
},
66+
ConfirmFromUserFn: func(msg, helpText string) (bool, error) {
67+
return false, errors.New("prompt error")
68+
},
9569
}
96-
confirmFromUserFn = func(msg, helpText string) (bool, error) {
97-
return false, errors.New("prompt error")
98-
}
99-
mockMu.Unlock()
100-
101-
defer func() {
102-
mockMu.Lock()
103-
getConfigFn = originalGetConfig
104-
confirmFromUserFn = originalConfirm
105-
mockMu.Unlock()
106-
}()
107-
108-
opts := LogoutOptions{}
10970
err := opts.Run()
11071
assert.Error(t, err)
11172
assert.Contains(t, err.Error(), "prompt error")

‎command/auth/refresh/refresh.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,14 +38,14 @@ func NewCmdRefresh() *cobra.Command {
3838
Short: "Refresh stored authentication credentials",
3939
Long: doc,
4040
Args: utils.NoArgs,
41-
RunE: func(cmd *cobra.Command, args []string) error {
41+
RunE: func(_ *cobra.Command, _ []string) error {
4242
return opts.Run()
4343
},
4444
}
4545
return cmd
4646
}
4747

48-
func (opts *RefreshOptions) Run() error {
48+
func (*RefreshOptions) Run() error {
4949
// Fetch config
5050
cfg, err := getConfigFn()
5151
if err != nil {
@@ -58,8 +58,8 @@ func (opts *RefreshOptions) Run() error {
5858

5959
// Fetching DS Client
6060
dsClient, err := newDeepsourceFn(deepsource.ClientOpts{
61-
Token: config.Cfg.Token,
62-
HostName: config.Cfg.Host,
61+
Token: cfg.Token,
62+
HostName: cfg.Host,
6363
})
6464
if err != nil {
6565
return err

‎command/auth/status/status.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,15 +29,15 @@ func NewCmdStatus() *cobra.Command {
2929
Short: "View the authentication status",
3030
Long: doc,
3131
Args: utils.NoArgs,
32-
RunE: func(cmd *cobra.Command, args []string) error {
32+
RunE: func(_ *cobra.Command, _ []string) error {
3333
opts := AuthStatusOptions{}
3434
return opts.Run()
3535
},
3636
}
3737
return cmd
3838
}
3939

40-
func (opts *AuthStatusOptions) Run() error {
40+
func (*AuthStatusOptions) Run() error {
4141
// Fetch config
4242
cfg, err := getConfigFn()
4343
if err != nil {

‎command/auth/status/status_test.go‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,8 @@ func TestAuthStatusOptions_Run(t *testing.T) {
2828
getConfigFn = func() (*config.CLIConfig, error) {
2929
return nil, errors.New("config read error")
3030
}
31-
mockMu.Unlock()
3231

3332
defer func() {
34-
mockMu.Lock()
3533
getConfigFn = originalGetConfig
3634
mockMu.Unlock()
3735
}()
@@ -48,10 +46,8 @@ func TestAuthStatusOptions_Run(t *testing.T) {
4846
getConfigFn = func() (*config.CLIConfig, error) {
4947
return &config.CLIConfig{Token: ""}, nil
5048
}
51-
mockMu.Unlock()
5249

5350
defer func() {
54-
mockMu.Lock()
5551
getConfigFn = originalGetConfig
5652
mockMu.Unlock()
5753
}()
@@ -72,10 +68,8 @@ func TestAuthStatusOptions_Run(t *testing.T) {
7268
TokenExpiresIn: time.Now().Add(24 * time.Hour),
7369
}, nil
7470
}
75-
mockMu.Unlock()
7671

7772
defer func() {
78-
mockMu.Lock()
7973
getConfigFn = originalGetConfig
8074
mockMu.Unlock()
8175
}()
@@ -95,10 +89,8 @@ func TestAuthStatusOptions_Run(t *testing.T) {
9589
TokenExpiresIn: time.Now().Add(-24 * time.Hour),
9690
}, nil
9791
}
98-
mockMu.Unlock()
9992

10093
defer func() {
101-
mockMu.Lock()
10294
getConfigFn = originalGetConfig
10395
mockMu.Unlock()
10496
}()

‎command/config/generate/generate.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ func NewCmdConfigGenerate() *cobra.Command {
4040
Short: "Generate config for DeepSource",
4141
Long: doc,
4242
Args: utils.NoArgs,
43-
RunE: func(cmd *cobra.Command, args []string) error {
43+
RunE: func(_ *cobra.Command, _ []string) error {
4444
return o.Run()
4545
},
4646
}

‎command/config/validate/validate.go‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"context"
55
"errors"
66
"fmt"
7-
"io/ioutil"
87
"os"
98
"os/exec"
109
"path/filepath"
@@ -30,15 +29,15 @@ func NewCmdValidate() *cobra.Command {
3029
Use: "validate",
3130
Short: "Validate DeepSource config",
3231
Args: utils.NoArgs,
33-
RunE: func(cmd *cobra.Command, args []string) error {
32+
RunE: func(_ *cobra.Command, _ []string) error {
3433
return o.Run()
3534
},
3635
}
3736
return cmd
3837
}
3938

4039
// Run executes the command.
41-
func (o *Options) Run() error {
40+
func (*Options) Run() error {
4241
// Fetch config
4342
cfg, err := config.GetConfig()
4443
if err != nil {
@@ -60,7 +59,7 @@ func (o *Options) Run() error {
6059
}
6160

6261
// Read the config in the form of string and send it
63-
content, err := ioutil.ReadFile(configPath)
62+
content, err := os.ReadFile(configPath)
6463
if err != nil {
6564
return errors.New("Error occured while reading DeepSource config file. Exiting...")
6665
}

‎command/issues/list/list.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ func NewCmdIssuesList() *cobra.Command {
7171
Use: "list",
7272
Short: "List issues reported by DeepSource",
7373
Long: doc,
74-
RunE: func(cmd *cobra.Command, args []string) error {
74+
RunE: func(_ *cobra.Command, args []string) error {
7575
opts.FileArg = args
7676
return opts.Run()
7777
},

0 commit comments

Comments
 (0)