From 17a87d0ecf9a181f515fdd0f7f63cbc0b0a4a459 Mon Sep 17 00:00:00 2001 From: Fabien Tregan Date: Sun, 10 Jun 2018 23:07:54 +0200 Subject: [PATCH 01/16] Breaks out TestConfigure into subtests. This allow the test tools to show individuals reports. Other tests in the CLI uses the pattern of having a slice of input+expectedOutput+name, they may also benefit form this solution (I can make the change if you like this commit) --- cmd/configure_test.go | 62 +++++++++++++++++++++++++------------------ 1 file changed, 36 insertions(+), 26 deletions(-) diff --git a/cmd/configure_test.go b/cmd/configure_test.go index 46bb326e7..053e85925 100644 --- a/cmd/configure_test.go +++ b/cmd/configure_test.go @@ -7,16 +7,18 @@ import ( "github.com/stretchr/testify/assert" ) +type testDefinition struct { + desc string + args []string + existingUsrCfg *config.UserConfig + expectedUsrCfg *config.UserConfig + existingAPICfg *config.APIConfig + expectedAPICfg *config.APIConfig +} + func TestConfigure(t *testing.T) { - tests := []struct { - desc string - args []string - existingUsrCfg *config.UserConfig - expectedUsrCfg *config.UserConfig - existingAPICfg *config.APIConfig - expectedAPICfg *config.APIConfig - }{ - { + tests := []testDefinition{ + testDefinition{ desc: "It writes the flags when there is no config file.", args: []string{"fakeapp", "configure", "--token", "a", "--workspace", "/a", "--api", "http://example.com"}, existingUsrCfg: nil, @@ -24,7 +26,7 @@ func TestConfigure(t *testing.T) { existingAPICfg: nil, expectedAPICfg: &config.APIConfig{BaseURL: "http://example.com"}, }, - { + testDefinition{ desc: "It overwrites the flags in the config file.", args: []string{"fakeapp", "configure", "--token", "b", "--workspace", "/b", "--api", "http://example.com/v2"}, existingUsrCfg: &config.UserConfig{Token: "token-b", Workspace: "/workspace-b"}, @@ -32,13 +34,13 @@ func TestConfigure(t *testing.T) { existingAPICfg: &config.APIConfig{BaseURL: "http://example.com/v1"}, expectedAPICfg: &config.APIConfig{BaseURL: "http://example.com/v2"}, }, - { + testDefinition{ desc: "It overwrites the flags that are passed, without losing the ones that are not.", args: []string{"fakeapp", "configure", "--token", "c"}, existingUsrCfg: &config.UserConfig{Token: "token-c", Workspace: "/workspace-c"}, expectedUsrCfg: &config.UserConfig{Token: "c", Workspace: "/workspace-c"}, }, - { + testDefinition{ desc: "It gets the default API base URL.", args: []string{"fakeapp", "configure"}, existingAPICfg: &config.APIConfig{}, @@ -46,37 +48,45 @@ func TestConfigure(t *testing.T) { }, } - for _, test := range tests { - cmdTest := &CommandTest{ + for _, definition := range tests { + t.Run(definition.desc, makeTest(definition)) + } +} + +func makeTest(definition testDefinition) func(*testing.T) { + + return func(t *testing.T) { + var cmdTest *CommandTest + cmdTest = &CommandTest{ Cmd: configureCmd, InitFn: initConfigureCmd, - Args: test.args, + Args: definition.args, } cmdTest.Setup(t) defer cmdTest.Teardown(t) - if test.existingUsrCfg != nil { + if definition.existingUsrCfg != nil { // Write a fake config. cfg := config.NewEmptyUserConfig() - cfg.Token = test.existingUsrCfg.Token - cfg.Workspace = test.existingUsrCfg.Workspace + cfg.Token = definition.existingUsrCfg.Token + cfg.Workspace = definition.existingUsrCfg.Workspace err := cfg.Write() - assert.NoError(t, err, test.desc) + assert.NoError(t, err, definition.desc) } cmdTest.App.Execute() - if test.expectedUsrCfg != nil { + if definition.expectedUsrCfg != nil { usrCfg, err := config.NewUserConfig() - assert.NoError(t, err, test.desc) - assert.Equal(t, test.expectedUsrCfg.Token, usrCfg.Token, test.desc) - assert.Equal(t, test.expectedUsrCfg.Workspace, usrCfg.Workspace, test.desc) + assert.NoError(t, err, definition.desc) + assert.Equal(t, definition.expectedUsrCfg.Token, usrCfg.Token, definition.desc) + assert.Equal(t, definition.expectedUsrCfg.Workspace, usrCfg.Workspace, definition.desc) } - if test.expectedAPICfg != nil { + if definition.expectedAPICfg != nil { apiCfg, err := config.NewAPIConfig() - assert.NoError(t, err, test.desc) - assert.Equal(t, test.expectedAPICfg.BaseURL, apiCfg.BaseURL, test.desc) + assert.NoError(t, err, definition.desc) + assert.Equal(t, definition.expectedAPICfg.BaseURL, apiCfg.BaseURL, definition.desc) } } } From c7278c8d522a78f673e7aa30b2cbb07d51ba6a33 Mon Sep 17 00:00:00 2001 From: Fabien Tregan Date: Wed, 13 Jun 2018 12:47:12 +0200 Subject: [PATCH 02/16] Takes into account @nywilken 's review. see https://github.com/exercism/cli/pull/540 --- cmd/configure_test.go | 45 +++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/cmd/configure_test.go b/cmd/configure_test.go index 053e85925..d1d81ed99 100644 --- a/cmd/configure_test.go +++ b/cmd/configure_test.go @@ -7,7 +7,7 @@ import ( "github.com/stretchr/testify/assert" ) -type testDefinition struct { +type testCase struct { desc string args []string existingUsrCfg *config.UserConfig @@ -17,8 +17,8 @@ type testDefinition struct { } func TestConfigure(t *testing.T) { - tests := []testDefinition{ - testDefinition{ + testCases := []testCase{ + testCase{ desc: "It writes the flags when there is no config file.", args: []string{"fakeapp", "configure", "--token", "a", "--workspace", "/a", "--api", "http://example.com"}, existingUsrCfg: nil, @@ -26,7 +26,7 @@ func TestConfigure(t *testing.T) { existingAPICfg: nil, expectedAPICfg: &config.APIConfig{BaseURL: "http://example.com"}, }, - testDefinition{ + testCase{ desc: "It overwrites the flags in the config file.", args: []string{"fakeapp", "configure", "--token", "b", "--workspace", "/b", "--api", "http://example.com/v2"}, existingUsrCfg: &config.UserConfig{Token: "token-b", Workspace: "/workspace-b"}, @@ -34,13 +34,13 @@ func TestConfigure(t *testing.T) { existingAPICfg: &config.APIConfig{BaseURL: "http://example.com/v1"}, expectedAPICfg: &config.APIConfig{BaseURL: "http://example.com/v2"}, }, - testDefinition{ + testCase{ desc: "It overwrites the flags that are passed, without losing the ones that are not.", args: []string{"fakeapp", "configure", "--token", "c"}, existingUsrCfg: &config.UserConfig{Token: "token-c", Workspace: "/workspace-c"}, expectedUsrCfg: &config.UserConfig{Token: "c", Workspace: "/workspace-c"}, }, - testDefinition{ + testCase{ desc: "It gets the default API base URL.", args: []string{"fakeapp", "configure"}, existingAPICfg: &config.APIConfig{}, @@ -48,45 +48,44 @@ func TestConfigure(t *testing.T) { }, } - for _, definition := range tests { - t.Run(definition.desc, makeTest(definition)) + for _, tc := range testCases { + t.Run(tc.desc, makeTest(tc)) } } -func makeTest(definition testDefinition) func(*testing.T) { +func makeTest(tc testCase) func(*testing.T) { return func(t *testing.T) { - var cmdTest *CommandTest - cmdTest = &CommandTest{ + cmdTest := &CommandTest{ Cmd: configureCmd, InitFn: initConfigureCmd, - Args: definition.args, + Args: tc.args, } cmdTest.Setup(t) defer cmdTest.Teardown(t) - if definition.existingUsrCfg != nil { + if tc.existingUsrCfg != nil { // Write a fake config. cfg := config.NewEmptyUserConfig() - cfg.Token = definition.existingUsrCfg.Token - cfg.Workspace = definition.existingUsrCfg.Workspace + cfg.Token = tc.existingUsrCfg.Token + cfg.Workspace = tc.existingUsrCfg.Workspace err := cfg.Write() - assert.NoError(t, err, definition.desc) + assert.NoError(t, err, tc.desc) } cmdTest.App.Execute() - if definition.expectedUsrCfg != nil { + if tc.expectedUsrCfg != nil { usrCfg, err := config.NewUserConfig() - assert.NoError(t, err, definition.desc) - assert.Equal(t, definition.expectedUsrCfg.Token, usrCfg.Token, definition.desc) - assert.Equal(t, definition.expectedUsrCfg.Workspace, usrCfg.Workspace, definition.desc) + assert.NoError(t, err, tc.desc) + assert.Equal(t, tc.expectedUsrCfg.Token, usrCfg.Token, tc.desc) + assert.Equal(t, tc.expectedUsrCfg.Workspace, usrCfg.Workspace, tc.desc) } - if definition.expectedAPICfg != nil { + if tc.expectedAPICfg != nil { apiCfg, err := config.NewAPIConfig() - assert.NoError(t, err, definition.desc) - assert.Equal(t, definition.expectedAPICfg.BaseURL, apiCfg.BaseURL, definition.desc) + assert.NoError(t, err, tc.desc) + assert.Equal(t, tc.expectedAPICfg.BaseURL, apiCfg.BaseURL, tc.desc) } } } From 1ce53b9069608b56872598127afd42130c206c2d Mon Sep 17 00:00:00 2001 From: Fabien Tregan Date: Sun, 17 Jun 2018 00:36:32 +0200 Subject: [PATCH 03/16] Breaks out TestPathType into subtests. --- workspace/path_type_test.go | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/workspace/path_type_test.go b/workspace/path_type_test.go index 232634ca4..c233bcfe3 100644 --- a/workspace/path_type_test.go +++ b/workspace/path_type_test.go @@ -8,16 +8,18 @@ import ( "github.com/stretchr/testify/assert" ) +type testCase struct { + desc string + path string + pt PathType +} + func TestDetectPathType(t *testing.T) { _, cwd, _, _ := runtime.Caller(0) root := filepath.Join(cwd, "..", "..", "fixtures", "detect-path-type") - tests := []struct { - desc string - path string - pt PathType - }{ - { + testCases := []testCase{ + testCase{ desc: "absolute dir", path: filepath.Join(root, "a-dir"), pt: TypeDir, @@ -54,9 +56,16 @@ func TestDetectPathType(t *testing.T) { }, } - for _, test := range tests { - pt, err := DetectPathType(test.path) - assert.NoError(t, err, test.desc) - assert.Equal(t, test.pt, pt, test.desc) + for _, tc := range testCases { + t.Run(tc.desc, makeTest(tc)) + + } +} + +func makeTest(tc testCase) func(*testing.T) { + return func(t *testing.T) { + pt, err := DetectPathType(tc.path) + assert.NoError(t, err, tc.desc) + assert.Equal(t, tc.pt, pt, tc.desc) } } From 9acf0d22d97229c0d02f8b0483d720daffa36e5a Mon Sep 17 00:00:00 2001 From: Fabien Tregan Date: Sun, 17 Jun 2018 11:32:33 +0200 Subject: [PATCH 04/16] Breaks out TestNewRequestSetsDefaultHeaders into subtests. --- api/client_test.go | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/api/client_test.go b/api/client_test.go index 4f3c6f9e7..ba069b738 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -18,19 +18,20 @@ func TestNewRequestSetsDefaultHeaders(t *testing.T) { UserAgent = "BogusAgent" - tests := []struct { + testCases := []struct { + desc string client *Client auth string contentType string }{ { - // Use defaults. + desc: "User defaults", client: &Client{}, auth: "", contentType: "application/json", }, { - // Override defaults. + desc: "Override defaults", client: &Client{ UserConfig: &config.UserConfig{Token: "abc123"}, ContentType: "bogus", @@ -40,12 +41,14 @@ func TestNewRequestSetsDefaultHeaders(t *testing.T) { }, } - for _, test := range tests { - req, err := test.client.NewRequest("GET", ts.URL, nil) - assert.NoError(t, err) - assert.Equal(t, "BogusAgent", req.Header.Get("User-Agent")) - assert.Equal(t, test.contentType, req.Header.Get("Content-Type")) - assert.Equal(t, test.auth, req.Header.Get("Authorization")) + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + req, err := tc.client.NewRequest("GET", ts.URL, nil) + assert.NoError(t, err) + assert.Equal(t, "BogusAgent", req.Header.Get("User-Agent")) + assert.Equal(t, tc.contentType, req.Header.Get("Content-Type")) + assert.Equal(t, tc.auth, req.Header.Get("Authorization")) + }) } } From 5d9ae04d536035fafde8637d40d36205fd1ce4d5 Mon Sep 17 00:00:00 2001 From: Fabien Tregan Date: Sun, 17 Jun 2018 11:42:15 +0200 Subject: [PATCH 05/16] Breaks out TestIsUpToDate into subtests. --- cli/cli_test.go | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/cli/cli_test.go b/cli/cli_test.go index b7968a08c..c9e71e37c 100644 --- a/cli/cli_test.go +++ b/cli/cli_test.go @@ -10,46 +10,49 @@ import ( ) func TestIsUpToDate(t *testing.T) { - tests := []struct { + testCases := []struct { + desc string cliVersion string releaseTag string ok bool }{ { - // It returns false for versions less than release. + desc: "It returns false for versions less than release.", cliVersion: "1.0.0", releaseTag: "v1.0.1", ok: false, }, { - // It returns false for pre-release versions of release. + desc: "It returns false for pre-release versions of release.", cliVersion: "1.0.1-alpha.1", releaseTag: "v1.0.1", ok: false, }, { - // It returns true for versions equal to release. + desc: "It returns true for versions equal to release.", cliVersion: "2.0.1", releaseTag: "v2.0.1", ok: true, }, { - // It returns true for versions greater than release. + desc: "It returns true for versions greater than release.", cliVersion: "2.0.2", releaseTag: "v2.0.1", ok: true, }, } - for _, test := range tests { - c := &CLI{ - Version: test.cliVersion, - LatestRelease: &Release{TagName: test.releaseTag}, - } + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + c := &CLI{ + Version: tc.cliVersion, + LatestRelease: &Release{TagName: tc.releaseTag}, + } - ok, err := c.IsUpToDate() - assert.NoError(t, err) - assert.Equal(t, test.ok, ok, test.cliVersion) + ok, err := c.IsUpToDate() + assert.NoError(t, err) + assert.Equal(t, tc.ok, ok, tc.cliVersion) + }) } } From c3f550667e8ebf8d4cc65bd028076bb97b36090c Mon Sep 17 00:00:00 2001 From: Fabien Tregan Date: Sun, 17 Jun 2018 12:00:10 +0200 Subject: [PATCH 06/16] Breaks out TestUpgrade into subtests. --- cmd/upgrade_test.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/cmd/upgrade_test.go b/cmd/upgrade_test.go index 6c3459650..e2fd09496 100644 --- a/cmd/upgrade_test.go +++ b/cmd/upgrade_test.go @@ -26,7 +26,7 @@ func TestUpgrade(t *testing.T) { Out = ioutil.Discard defer func() { Out = oldOut }() - tests := []struct { + testCases := []struct { desc string upToDate bool expected bool @@ -43,11 +43,13 @@ func TestUpgrade(t *testing.T) { }, } - for _, test := range tests { - fc := &fakeCLI{UpToDate: test.upToDate} + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + fc := &fakeCLI{UpToDate: tc.upToDate} - err := updateCLI(fc) - assert.NoError(t, err) - assert.Equal(t, test.expected, fc.UpgradeCalled, test.desc) + err := updateCLI(fc) + assert.NoError(t, err) + assert.Equal(t, tc.expected, fc.UpgradeCalled) + }) } } From 6aacbad7da2d124d663594d7d1caad5ad74af428 Mon Sep 17 00:00:00 2001 From: Fabien Tregan Date: Sun, 17 Jun 2018 12:04:38 +0200 Subject: [PATCH 07/16] Breaks out TestVersionUpdateCheck into subtests. --- cmd/version_test.go | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/cmd/version_test.go b/cmd/version_test.go index e2a55a141..6c8916282 100644 --- a/cmd/version_test.go +++ b/cmd/version_test.go @@ -25,36 +25,39 @@ func TestVersionUpdateCheck(t *testing.T) { defer ts.Close() cli.ReleaseURL = ts.URL - tests := []struct { + testCases := []struct { + desc string version string expected string }{ { - // It returns new version available for versions older than latest. + desc: "It returns new version available for versions older than latest.", version: "1.0.0", expected: "A new CLI version is available. Run `exercism upgrade` to update to 2.0.0", }, { - // It returns up to date for versions matching latest. + desc: "It returns up to date for versions matching latest.", version: "2.0.0", expected: "Your CLI version is up to date.", }, { - // It returns up to date for versions newer than latest. + desc: "It returns up to date for versions newer than latest.", version: "2.0.1", expected: "Your CLI version is up to date.", }, } - for _, test := range tests { - c := &cli.CLI{ - Version: test.version, - } + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + c := &cli.CLI{ + Version: tc.version, + } - actual, err := checkForUpdate(c) + actual, err := checkForUpdate(c) - assert.NoError(t, err) - assert.NotEmpty(t, actual) - assert.Equal(t, test.expected, actual) + assert.NoError(t, err) + assert.NotEmpty(t, actual) + assert.Equal(t, tc.expected, actual) + }) } } From 9eed5a381ce156c7e8e85339cdf970c3c81fa5db Mon Sep 17 00:00:00 2001 From: Fabien Tregan Date: Sun, 17 Jun 2018 12:50:37 +0200 Subject: [PATCH 08/16] Breaks out TestQuestion into subtests. --- comms/question_test.go | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/comms/question_test.go b/comms/question_test.go index f7721ae0d..20bd778ac 100644 --- a/comms/question_test.go +++ b/comms/question_test.go @@ -9,7 +9,7 @@ import ( ) func TestQuestion(t *testing.T) { - tests := []struct { + testCases := []struct { desc string given string fallback string @@ -18,16 +18,18 @@ func TestQuestion(t *testing.T) { {"records interactive response", "hello\n", "", "hello"}, {"responds with default if response is empty", "\n", "Fine.", "Fine."}, } - for _, test := range tests { - q := &Question{ - Reader: strings.NewReader(test.given), - Writer: ioutil.Discard, - Prompt: "Say something: ", - DefaultValue: test.fallback, - } + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + q := &Question{ + Reader: strings.NewReader(tc.given), + Writer: ioutil.Discard, + Prompt: "Say something: ", + DefaultValue: tc.fallback, + } - answer, err := q.Ask() - assert.NoError(t, err) - assert.Equal(t, answer, test.expected, test.desc) + answer, err := q.Ask() + assert.NoError(t, err) + assert.Equal(t, answer, tc.expected) + }) } } From c96688946686b1d42bb084b04ad951a055ca3411 Mon Sep 17 00:00:00 2001 From: Fabien Tregan Date: Sun, 17 Jun 2018 12:57:19 +0200 Subject: [PATCH 09/16] Breaks out TestSelectionPick into subtests. --- comms/selection_test.go | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/comms/selection_test.go b/comms/selection_test.go index ad6bc552a..bbf2ce16e 100644 --- a/comms/selection_test.go +++ b/comms/selection_test.go @@ -78,7 +78,7 @@ func TestSelectionRead(t *testing.T) { } func TestSelectionPick(t *testing.T) { - tests := []struct { + testCases := []struct { desc string selection Selection things []thing @@ -111,16 +111,18 @@ func TestSelectionPick(t *testing.T) { }, } - for _, test := range tests { - test.selection.Writer = ioutil.Discard - for _, th := range test.things { - test.selection.Items = append(test.selection.Items, th) - } - - item, err := test.selection.Pick("which one? %s") - assert.NoError(t, err) - th, ok := item.(thing) - assert.True(t, ok) - assert.Equal(t, test.expected, th.name) + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + tc.selection.Writer = ioutil.Discard + for _, th := range tc.things { + tc.selection.Items = append(tc.selection.Items, th) + } + + item, err := tc.selection.Pick("which one? %s") + assert.NoError(t, err) + th, ok := item.(thing) + assert.True(t, ok) + assert.Equal(t, tc.expected, th.name) + }) } } From 09a8fe7e9eceb1aec60a5fe20a6cc73ee1aa3b30 Mon Sep 17 00:00:00 2001 From: Fabien Tregan Date: Sun, 17 Jun 2018 13:16:26 +0200 Subject: [PATCH 10/16] Breaks out TestTrackIgnoreString into subtests. --- config/track_test.go | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/config/track_test.go b/config/track_test.go index 8440e4866..55f79ecbb 100644 --- a/config/track_test.go +++ b/config/track_test.go @@ -14,16 +14,26 @@ func TestTrackIgnoreString(t *testing.T) { }, } - tests := map[string]bool{ + testCases := map[string]bool{ "falcon.txt": false, "beacon|txt": true, "beacon.ext": true, "proof": false, } - for name, acceptable := range tests { - ok, err := track.AcceptFilename(name) - assert.NoError(t, err, name) - assert.Equal(t, acceptable, ok, name) + for name, acceptable := range testCases { + testName := name + " should " + notIfNeeded(acceptable) + "be an acceptable name." + t.Run(testName, func(t *testing.T) { + ok, err := track.AcceptFilename(name) + assert.NoError(t, err, name) + assert.Equal(t, acceptable, ok, testName) + }) } } + +func notIfNeeded(b bool) string { + if !b { + return "not " + } + return "" +} From 443cd0f591407945dcf14e4612f97d12d836c05a Mon Sep 17 00:00:00 2001 From: Fabien Tregan Date: Sun, 17 Jun 2018 13:27:27 +0200 Subject: [PATCH 11/16] Breaks out TestNormalizeWorkspace into subtests. --- config/user_config_test.go | 13 ++++++++----- config/user_config_windows_test.go | 14 +++++++++----- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/config/user_config_test.go b/config/user_config_test.go index 9d65aa532..b3ce81c77 100644 --- a/config/user_config_test.go +++ b/config/user_config_test.go @@ -42,7 +42,7 @@ func TestNormalizeWorkspace(t *testing.T) { assert.NoError(t, err) cfg := &UserConfig{Home: "/home/alice"} - tests := []struct { + testCases := []struct { in, out string }{ {"", ""}, // don't make wild guesses @@ -54,9 +54,12 @@ func TestNormalizeWorkspace(t *testing.T) { {"relative///path", filepath.Join(cwd, "relative", "path")}, } - for _, test := range tests { - cfg.Workspace = test.in - cfg.Normalize() - assert.Equal(t, test.out, cfg.Workspace) + for _, tc := range testCases { + testName := "'" + tc.in + "' should be normalized as '" + tc.out + "'" + t.Run(testName, func(t *testing.T) { + cfg.Workspace = tc.in + cfg.Normalize() + assert.Equal(t, tc.out, cfg.Workspace, testName) + }) } } diff --git a/config/user_config_windows_test.go b/config/user_config_windows_test.go index 62c8255e9..5ea9a3e45 100644 --- a/config/user_config_windows_test.go +++ b/config/user_config_windows_test.go @@ -40,7 +40,7 @@ func TestNormalizeWorkspace(t *testing.T) { assert.NoError(t, err) cfg := &UserConfig{Home: "C:\\Users\\alice"} - tests := []struct { + testCases := []struct { in, out string }{ {"", ""}, // don't make wild guesses @@ -54,9 +54,13 @@ func TestNormalizeWorkspace(t *testing.T) { {"relative///path", filepath.Join(cwd, "relative", "path")}, } - for _, test := range tests { - cfg.Workspace = test.in - cfg.Normalize() - assert.Equal(t, test.out, cfg.Workspace) + for _, tc := range testCases { + testName := "'" + tc.in + "' should be normalized as '" + tc.out + "'" + + t.Run(testName, func(t *testing.T) { + cfg.Workspace = tc.in + cfg.Normalize() + assert.Equal(t, tc.out, cfg.Workspace, testName) + }) } } From d7f1c329e3a99f2fb379ef2ac25b2fa3b24dc194 Mon Sep 17 00:00:00 2001 From: Fabien Tregan Date: Sun, 17 Jun 2018 13:47:25 +0200 Subject: [PATCH 12/16] Breaks out TestSolutionString into subtests There is currently no description for the tests: only in and expected out values, --- workspace/solution_test.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/workspace/solution_test.go b/workspace/solution_test.go index fb621099f..7c1d1e064 100644 --- a/workspace/solution_test.go +++ b/workspace/solution_test.go @@ -43,7 +43,7 @@ func TestSolution(t *testing.T) { } func TestSuffix(t *testing.T) { - tests := []struct { + testCases := []struct { solution Solution suffix string }{ @@ -77,13 +77,16 @@ func TestSuffix(t *testing.T) { }, } - for _, test := range tests { - assert.Equal(t, test.suffix, test.solution.Suffix()) + for _, tc := range testCases { + testName := "Suffix of '" + tc.solution.Dir + "' should be " + tc.suffix + t.Run(testName, func(t *testing.T) { + assert.Equal(t, tc.suffix, tc.solution.Suffix(), testName) + }) } } func TestSolutionString(t *testing.T) { - tests := []struct { + testCases := []struct { solution Solution desc string }{ @@ -136,7 +139,10 @@ func TestSolutionString(t *testing.T) { }, } - for _, test := range tests { - assert.Equal(t, test.desc, test.solution.String()) + for _, tc := range testCases { + testName := "should stringify to '" + tc.desc + "'" + t.Run(testName, func(t *testing.T) { + assert.Equal(t, tc.desc, tc.solution.String()) + }) } } From c82d2dad0c02fb439ddcd7c962aed088e2fa7acc Mon Sep 17 00:00:00 2001 From: Fabien Tregan Date: Sun, 17 Jun 2018 13:52:32 +0200 Subject: [PATCH 13/16] Breaks out TestNewTransmission into subtests. --- workspace/transmission_test.go | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/workspace/transmission_test.go b/workspace/transmission_test.go index 828d40a66..9faf7d69a 100644 --- a/workspace/transmission_test.go +++ b/workspace/transmission_test.go @@ -18,7 +18,7 @@ func TestNewTransmission(t *testing.T) { fileBird := filepath.Join(dirBird, "hummingbird.txt") fileSugar := filepath.Join(dirFeeder, "sugar.txt") - tests := []struct { + testCases := []struct { desc string args []string ok bool @@ -65,18 +65,20 @@ func TestNewTransmission(t *testing.T) { }, } - for _, test := range tests { - tx, err := NewTransmission(root, test.args) - if test.ok { - assert.NoError(t, err, test.desc) - } else { - assert.Error(t, err, test.desc) - } + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + tx, err := NewTransmission(root, tc.args) + if tc.ok { + assert.NoError(t, err, tc.desc) + } else { + assert.Error(t, err, tc.desc) + } - if test.tx != nil { - assert.Equal(t, test.tx.Files, tx.Files, test.desc) - assert.Equal(t, test.tx.Dir, tx.Dir, test.desc) - } + if tc.tx != nil { + assert.Equal(t, tc.tx.Files, tx.Files) + assert.Equal(t, tc.tx.Dir, tx.Dir) + } + }) } } From 2aae273cd2ec213705c9aa3ad7785e6a8f37f6a1 Mon Sep 17 00:00:00 2001 From: Fabien Tregan Date: Sun, 17 Jun 2018 14:00:57 +0200 Subject: [PATCH 14/16] Breaks out TestLocateErrors into subtests. --- workspace/workspace_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/workspace/workspace_test.go b/workspace/workspace_test.go index 608ba7173..e183dc0e5 100644 --- a/workspace/workspace_test.go +++ b/workspace/workspace_test.go @@ -16,7 +16,7 @@ func TestLocateErrors(t *testing.T) { ws := New(filepath.Join(root, "workspace")) - tests := []struct { + testCases := []struct { desc, arg string errFn func(error) bool }{ @@ -47,9 +47,11 @@ func TestLocateErrors(t *testing.T) { }, } - for _, test := range tests { - _, err := ws.Locate(test.arg) - assert.True(t, test.errFn(err), fmt.Sprintf("test: %s (arg: %s), %#v", test.desc, test.arg, err)) + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + _, err := ws.Locate(tc.arg) + assert.True(t, tc.errFn(err), fmt.Sprintf("test: %s (arg: %s), %#v", tc.desc, tc.arg, err)) + }) } } From a3fa5138930faca5a2ca4cb985eda0a7455a1204 Mon Sep 17 00:00:00 2001 From: Fabien Tregan Date: Sun, 17 Jun 2018 14:21:59 +0200 Subject: [PATCH 15/16] Breaks out TestDownload into subtests. --- cmd/download_test.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/cmd/download_test.go b/cmd/download_test.go index 5129de6ea..c91b38667 100644 --- a/cmd/download_test.go +++ b/cmd/download_test.go @@ -91,19 +91,23 @@ func TestDownload(t *testing.T) { err = apiCfg.Write() assert.NoError(t, err) - tests := []struct { + testCases := []struct { + desc string path string contents string }{ { + desc: "path with no subdir", path: filepath.Join(cmdTest.TmpDir, "bogus-track", "bogus-exercise", "file-1.txt"), contents: "this is file 1", }, { + desc: "path with subdir", path: filepath.Join(cmdTest.TmpDir, "bogus-track", "bogus-exercise", "subdir", "file-2.txt"), contents: "this is file 2", }, { + desc: "solution file", path: filepath.Join(cmdTest.TmpDir, "bogus-track", "bogus-exercise", ".solution.json"), contents: `{"track":"bogus-track","exercise":"bogus-exercise","id":"bogus-id","url":"","handle":"alice","is_requester":true,"auto_approve":false}`, }, @@ -111,14 +115,15 @@ func TestDownload(t *testing.T) { cmdTest.App.Execute() - for _, test := range tests { - b, err := ioutil.ReadFile(test.path) - assert.NoError(t, err) - assert.Equal(t, test.contents, string(b)) + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + b, err := ioutil.ReadFile(tc.path) + assert.NoError(t, err) + assert.Equal(t, tc.contents, string(b), "content of "+tc.path) + }) } - // It doesn't write the empty file. path := filepath.Join(cmdTest.TmpDir, "bogus-track", "bogus-exercise", path3) _, err = os.Lstat(path) - assert.True(t, os.IsNotExist(err)) + assert.True(t, os.IsNotExist(err), "It doesn't write the empty file.") } From 949921964f96146b22573ef95c9c010c57b3c68c Mon Sep 17 00:00:00 2001 From: Fabien Tregan Date: Thu, 21 Jun 2018 09:29:05 +0200 Subject: [PATCH 16/16] Refactors TestDownload. --- cmd/download_test.go | 152 +++++++++++++++++++++++-------------------- 1 file changed, 81 insertions(+), 71 deletions(-) diff --git a/cmd/download_test.go b/cmd/download_test.go index c91b38667..406fad628 100644 --- a/cmd/download_test.go +++ b/cmd/download_test.go @@ -13,9 +13,37 @@ import ( "github.com/stretchr/testify/assert" ) +const payloadTemplate = ` +{ + "solution": { + "id": "bogus-id", + "user": { + "handle": "alice", + "is_requester": true + }, + "exercise": { + "id": "bogus-exercise", + "instructions_url": "http://example.com/bogus-exercise", + "auto_approve": false, + "track": { + "id": "bogus-track", + "language": "Bogus Language" + } + }, + "file_download_base_url": "%s", + "files": [ + "%s", + "%s", + "%s" + ], + "iteration": { + "submitted_at": "2017-08-21t10:11:12.130z" + } + } +} +` + func TestDownload(t *testing.T) { - // Let's not actually print to standard out while testing. - Out = ioutil.Discard cmdTest := &CommandTest{ Cmd: downloadCmd, @@ -25,70 +53,12 @@ func TestDownload(t *testing.T) { cmdTest.Setup(t) defer cmdTest.Teardown(t) - // Write a fake user config setting the workspace to the temp dir. - userCfg := config.NewEmptyUserConfig() - userCfg.Workspace = cmdTest.TmpDir - err := userCfg.Write() - assert.NoError(t, err) - - payloadBody := ` - { - "solution": { - "id": "bogus-id", - "user": { - "handle": "alice", - "is_requester": true - }, - "exercise": { - "id": "bogus-exercise", - "instructions_url": "http://example.com/bogus-exercise", - "auto_approve": false, - "track": { - "id": "bogus-track", - "language": "Bogus Language" - } - }, - "file_download_base_url": "%s", - "files": [ - "%s", - "%s", - "%s" - ], - "iteration": { - "submitted_at": "2017-08-21t10:11:12.130z" - } - } - } - ` - - mux := http.NewServeMux() - server := httptest.NewServer(mux) - defer server.Close() - - path1 := "file-1.txt" - mux.HandleFunc("/"+path1, func(w http.ResponseWriter, r *http.Request) { - fmt.Fprint(w, "this is file 1") - }) - - path2 := "subdir/file-2.txt" - mux.HandleFunc("/"+path2, func(w http.ResponseWriter, r *http.Request) { - fmt.Fprint(w, "this is file 2") - }) - - path3 := "file-3.txt" - mux.HandleFunc("/"+path3, func(w http.ResponseWriter, r *http.Request) { - fmt.Fprint(w, "") - }) - - payloadBody = fmt.Sprintf(payloadBody, server.URL+"/", path1, path2, path3) - mux.HandleFunc("/solutions/latest", func(w http.ResponseWriter, r *http.Request) { - fmt.Fprint(w, payloadBody) - }) + mockServer := makeMockServer() + defer mockServer.Close() - // Write a fake api config setting the base url to the test server. - apiCfg := config.NewEmptyAPIConfig() - apiCfg.BaseURL = server.URL - err = apiCfg.Write() + err := writeFakeUserConfigSetting(cmdTest.TmpDir) + assert.NoError(t, err) + err = writeFakeAPIConfigSetting(mockServer.URL) assert.NoError(t, err) testCases := []struct { @@ -97,17 +67,17 @@ func TestDownload(t *testing.T) { contents string }{ { - desc: "path with no subdir", + desc: "It should download a file.", path: filepath.Join(cmdTest.TmpDir, "bogus-track", "bogus-exercise", "file-1.txt"), contents: "this is file 1", }, { - desc: "path with subdir", + desc: "It should download a file in a subdir.", path: filepath.Join(cmdTest.TmpDir, "bogus-track", "bogus-exercise", "subdir", "file-2.txt"), contents: "this is file 2", }, { - desc: "solution file", + desc: "It creates the .solution.json file.", path: filepath.Join(cmdTest.TmpDir, "bogus-track", "bogus-exercise", ".solution.json"), contents: `{"track":"bogus-track","exercise":"bogus-exercise","id":"bogus-id","url":"","handle":"alice","is_requester":true,"auto_approve":false}`, }, @@ -119,11 +89,51 @@ func TestDownload(t *testing.T) { t.Run(tc.desc, func(t *testing.T) { b, err := ioutil.ReadFile(tc.path) assert.NoError(t, err) - assert.Equal(t, tc.contents, string(b), "content of "+tc.path) + assert.Equal(t, tc.contents, string(b)) }) } - path := filepath.Join(cmdTest.TmpDir, "bogus-track", "bogus-exercise", path3) + path := filepath.Join(cmdTest.TmpDir, "bogus-track", "bogus-exercise", "file-3.txt") _, err = os.Lstat(path) - assert.True(t, os.IsNotExist(err), "It doesn't write the empty file.") + assert.True(t, os.IsNotExist(err), "It should not write the file if empty.") +} + +func writeFakeUserConfigSetting(tmpDirPath string) error { + userCfg := config.NewEmptyUserConfig() + userCfg.Workspace = tmpDirPath + return userCfg.Write() +} + +func writeFakeAPIConfigSetting(serverURL string) error { + apiCfg := config.NewEmptyAPIConfig() + apiCfg.BaseURL = serverURL + return apiCfg.Write() +} + +func makeMockServer() *httptest.Server { + mux := http.NewServeMux() + server := httptest.NewServer(mux) + + path1 := "file-1.txt" + mux.HandleFunc("/"+path1, func(w http.ResponseWriter, r *http.Request) { + fmt.Fprint(w, "this is file 1") + }) + + path2 := "subdir/file-2.txt" + mux.HandleFunc("/"+path2, func(w http.ResponseWriter, r *http.Request) { + fmt.Fprint(w, "this is file 2") + }) + + path3 := "file-3.txt" + mux.HandleFunc("/"+path3, func(w http.ResponseWriter, r *http.Request) { + fmt.Fprint(w, "") + }) + + payloadBody := fmt.Sprintf(payloadTemplate, server.URL+"/", path1, path2, path3) + mux.HandleFunc("/solutions/latest", func(w http.ResponseWriter, r *http.Request) { + fmt.Fprint(w, payloadBody) + }) + + return server + }