Break out test into subtests. #540

FabienTregan · 2018-06-10T22:09:01Z

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)

kytrinyx · 2018-06-11T00:42:08Z

I'm going to let @nywilken review and merge this, but had a small unrelated comment. It looks like GitHub doesn't recognize the email that you've used to commit with. You can tweak this in https://github.com/settings/emails without making your email visible on your profile.

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)

FabienTregan · 2018-06-11T09:35:19Z

I think it's ok now :)

kytrinyx · 2018-06-11T17:39:14Z

I think it's ok now

Indeed! Nicely done 🌷

nywilken

I think breaking these up into subtest makes sense. As you said in the previous PR it helps to see which is failing and run individual tests.

Nice work on the split. I left some comments pertaining to variable naming. For future commits or in this one would it make sense to change the descriptions so that they are a bit consistent? Making it easy to use go test -run=

nywilken · 2018-06-12T23:59:57Z

cmd/configure_test.go

+
+	return func(t *testing.T) {
+		var cmdTest *CommandTest
+		cmdTest = &CommandTest{
 			Cmd:    configureCmd,


We can drop the var cmdTest ... above and use the short variable declaration here.

nywilken · 2018-06-13T00:01:50Z

cmd/configure_test.go

@@ -7,76 +7,86 @@ import (
 	"github.com/stretchr/testify/assert"
 )

+type testDefinition struct {


What do you think about calling this type testCase ? testDefinition is a bit vague

nywilken · 2018-06-13T00:04:56Z

cmd/configure_test.go

-		expectedAPICfg *config.APIConfig
-	}{
-		{
+	tests := []testDefinition{


testCases as a slice of testCase

nywilken · 2018-06-13T00:31:05Z

cmd/configure_test.go

 			desc:           "It gets the default API base URL.",
 			args:           []string{"fakeapp", "configure"},
 			existingAPICfg: &config.APIConfig{},
 			expectedAPICfg: &config.APIConfig{BaseURL: "https://v2.exercism.io/api/v1"},
 		},
 	}

-	for _, test := range tests {
-		cmdTest := &CommandTest{
+	for _, definition := range tests {


It’s hard to make the distinction on the context of definition as we enter the makeTest func. Maybe it’s because I’m used to seeing it but testcase seems easier to wrap my head around.

I agree with all you comment and made the change locally. Will commit soon, thank you.

Also in the meantime I needed to do the same modification on another test (TestDetectPathType). Would you rather avec separate branch/PR for each test that can benefit from this pattern, or separate commit all in the same branch ? (there are 21 tests using testCases)

also, what do you favor in Go for naming a variable when the abvious name is the variable type ? I've seen func xx(testCase testCase) and func xx(tc TestCase), and others (pre/postfixing type or variable). What do you favor in this project ?

I wouldn't say favor. We generally lean towards readability and consistency. xx(testCase testCase) stomps out the type so I would not recommend that. Seeing as makeFunc is essentially a method of testCase, which could also be written as func (tc testCase) makeTest() func(*t.Testing) I would think that tc as the name is sufficient, an alternative is testcase as a mixed case is not an option here.

Also in the meantime I needed to do the same modification on another test (TestDetectPathType). Would you rather avec separate branch/PR for each test that can benefit from this pattern, or separate commit all in the same branch ? (there are 21 tests using testCases)

If they are the same type of change than one PR can work. I ask that you commits be broken down per test file (no mixing between configure_test, workspace_test, etc) so that we can review each commit without having to revisit already reviewed items. Please let me know what you think or my ask is not clear.

Thanks again for all your effort with the CLI.

see exercism#540

nywilken · 2018-06-16T22:14:22Z

@FabienTregan is there anything else I can help with on this PR? There’s no rush; I want to make sure that you are getting the feedback you need to get this merged. Cheers!

FabienTregan · 2018-06-16T22:59:47Z

@nywilken No it's ok, I think I'll be done tomorrow morning for this one (paris time).

I only refactored the tests I needed to reach a first milestone of PR #539, but now it's done I'll get back to it.

Thanks to you and @kytrinyx , I really leant a lot this week and coding was fun :)

FabienTregan · 2018-06-16T23:19:36Z

There is currently no description for the tests: only in and expected out values,

FabienTregan · 2018-06-17T12:26:57Z

Done for now \o/

It makes sens to wait for #539 to be merged before working on workspace/workspace_test.go

nywilken · 2018-06-18T04:45:07Z

This is looking good. Having the comments actually turned into test case descriptions is a bonus within this refactor. I’ll take another pass once behind a machine to get this merged.

nywilken

@FabienTregan overall this looks good thank you. I found a few inconsistencies with testNames using desc vs string concatenation. Nothing major and I would be willing to merge and address later as you already have a PR for solving the really problem of fixing tests on Windows. Let me know if you are okay with defering or if you want to make changes after reading my comments.

nywilken · 2018-06-19T11:57:33Z

config/track_test.go

-		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."


As a fast follows to this we should replace testName generation with a desc field in the testCases slice. This way we can specify test cases with -run= easier.

nywilken · 2018-06-19T11:58:31Z

config/track_test.go

 	}
 }
+
+func notIfNeeded(b bool) string {


I found this a little confusing at first, but I get it. I think for future state we should add a desc field to the testCases

Yes. It's a quick hack because the original code, instead of using a []struct input {input, expected_output} uses a map with a (string) key as the input value, and the value as the expected output.
I think this can just be replaced with the pattern that is used elsewhere, then it would be easy to add a description field to the struct, but I was not.

nywilken · 2018-06-21T11:02:23Z

@FabienTregan I am ready to merge this PR. I see you made a change since my last review yesterday evening. Do you still want to make more changes? I took your last comment to mean that you will not refactor other tests at this time.

Everything here looks good. Like I said we can refactor further once we have Windows tests rebases. Let me know.

FabienTregan · 2018-06-21T11:18:41Z

No, it's ok, I am done :) ----- Mail original ----- De: "Wilken Rivera" <[email protected]> À: "exercism/cli" <[email protected]> Cc: "Fabien Tregan" <[email protected]>, "Mention" <[email protected]> Envoyé: Jeudi 21 Juin 2018 13:02:26 Objet: Re: [exercism/cli] Break out test into subtests. (#540) @FabienTregan I am ready to merge this PR. I see you made a change since my last review yesterday evening. Do you still want to make more changes? I took your last comment to mean that you will not refactor other tests at this time. Everything here looks good. Like I said we can refactor further once we have Windows tests rebases. Let me know. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub , or mute the thread .

FabienTregan force-pushed the breakout_test_configure branch 2 times, most recently from 7d9cf66 to dd49a1e Compare June 11, 2018 09:06

Breaks out TestConfigure into subtests.

17a87d0

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)

FabienTregan force-pushed the breakout_test_configure branch from dd49a1e to 17a87d0 Compare June 11, 2018 09:20

nywilken reviewed Jun 13, 2018

View reviewed changes

Takes into account @nywilken 's review.

c7278c8

see exercism#540

Breaks out TestPathType into subtests.

1ce53b9

FabienTregan force-pushed the breakout_test_configure branch from 723add0 to 1ce53b9 Compare June 16, 2018 22:37

FabienTregan added 12 commits June 17, 2018 11:41

Breaks out TestNewRequestSetsDefaultHeaders into subtests.

9acf0d2

Breaks out TestIsUpToDate into subtests.

5d9ae04

Breaks out TestUpgrade into subtests.

c3f5506

Breaks out TestVersionUpdateCheck into subtests.

6aacbad

Breaks out TestQuestion into subtests.

9eed5a3

Breaks out TestSelectionPick into subtests.

c966889

Breaks out TestTrackIgnoreString into subtests.

09a8fe7

Breaks out TestNormalizeWorkspace into subtests.

443cd0f

Breaks out TestSolutionString into subtests

d7f1c32

There is currently no description for the tests: only in and expected out values,

Breaks out TestNewTransmission into subtests.

c82d2da

Breaks out TestLocateErrors into subtests.

2aae273

Breaks out TestDownload into subtests.

a3fa513

nywilken reviewed Jun 19, 2018

View reviewed changes

nywilken changed the title ~~Breaks out TestConfigure into subtests.~~ Break out test into subtests. Jun 21, 2018

Refactors TestDownload.

9499219

nywilken merged commit 818bc53 into exercism:nextercism Jun 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Break out test into subtests. #540

Break out test into subtests. #540

FabienTregan commented Jun 10, 2018

kytrinyx commented Jun 11, 2018

FabienTregan commented Jun 11, 2018

kytrinyx commented Jun 11, 2018

nywilken left a comment

nywilken Jun 12, 2018

nywilken Jun 13, 2018

nywilken Jun 13, 2018

nywilken Jun 13, 2018

FabienTregan Jun 13, 2018

FabienTregan Jun 13, 2018

nywilken Jun 13, 2018

nywilken Jun 13, 2018

nywilken commented Jun 16, 2018 •

edited

Loading

FabienTregan commented Jun 16, 2018 •

edited

Loading

FabienTregan commented Jun 16, 2018 •

edited

Loading

FabienTregan commented Jun 17, 2018

nywilken commented Jun 18, 2018

nywilken left a comment

nywilken Jun 19, 2018

nywilken Jun 19, 2018

FabienTregan Jun 19, 2018

nywilken commented Jun 21, 2018

FabienTregan commented Jun 21, 2018 via email

Break out test into subtests. #540

Break out test into subtests. #540

Conversation

FabienTregan commented Jun 10, 2018

kytrinyx commented Jun 11, 2018

FabienTregan commented Jun 11, 2018

kytrinyx commented Jun 11, 2018

nywilken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nywilken commented Jun 16, 2018 • edited Loading

FabienTregan commented Jun 16, 2018 • edited Loading

FabienTregan commented Jun 16, 2018 • edited Loading

FabienTregan commented Jun 17, 2018

nywilken commented Jun 18, 2018

nywilken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nywilken commented Jun 21, 2018

FabienTregan commented Jun 21, 2018 via email

nywilken commented Jun 16, 2018 •

edited

Loading

FabienTregan commented Jun 16, 2018 •

edited

Loading

FabienTregan commented Jun 16, 2018 •

edited

Loading