[wip] many exercises: use subtests for table driven tests

[martint17r]

This is a work in progress to use subtests for all table driven tests for all exercises. I use exercism for teaching/coaching and a lot of the students have problems identifying the failing test case. Subtests make this much easier.

There is a downside though: using t.Run raises the minimum go version to 1.7. Go 1.7 was released back in August '16. In my opinion it is prudent to require a version that old.

If the maintainers agree all table driven tests will be converted in all exercises

[leenipper]

Thanks for contributing here @martint17r.
Sorry about the delay in responding on this.

I have experimented a bit with a couple of exercises using t.Run to get the feel for what changes.
Using the t.Run approach for table driven tests that don't currently use Logf("PASS: ... ", ...) does provide more information in the verbose case "go test -v". But for tests that already use Logf for passing test cases, the verbose output is a bit noisy with the extra t.Run subtest output; so seems only one should be used. I also have a slight preference for the test .description (or .name) shown "as-is" with the Logf verbose output, not with spaces changed to underscores with t.Run subtest labeling.

See my one review comment about testing the t.Run return value as a means to break out of the test case loop; without t.Run, use of Fatalf would abort the testing function, stopping on the first failed case; use of Fatalf with t.Run lets the case loop continue unless the return value is tested and used to break. Some exercises use Errorf and continue on a failure(about 24), while most(about 104) use Fatalf and stop on first failure at present.

I'd like to hear any comments from @bitfield, @ferhatelmas, @hilary, @sebito91, and @kytrinyx. I'm undecided as to whether it is worth it to move exercises to use t.Run. I welcome some discussion.

[leenipper]

Just something to consider. The bool return value from t.Run could be used to break out of the loop. Previously (as t.Run wasn't used), when t.Fatalf is called, it does stop the looping over the test cases.

[martint17r]

I think there's value in continuing to try to run all tests. I will change that if it's needed to get merged.

[martint17r]

No worries on the delay.

I agree that Logf output looks nicer, but it sets the focus on passed tests. I observed that the students had problems to identify the failed table driven tests.

The blog post introducing the subtests lists many reasons why using them is beneficial. In addition subtests are understood by IDEs such as Goland and Visual Studio Code so they display helpful context.

The single most useful feature with subtests for me is that I am able to rerun exactly one subtest using the -run flag while developing, so that the output is not cluttered with any other test.

Another solution would be to ensure that the test name or description is included in every t.Error or t.Fatal call and the students can identify it as such.

[bitfield]

I'm a big fan of t.Run(). It's not like you couldn't manually print out the name of the test case along with the failure message, but this just makes it easier. It saves you having to remember to include the test case name along with each possible output message. It also encourages you to name your test cases, making the tests easier to read. Being able to run individual subtests, as @martint17r noted, is also handy.

The semantics of t.Error and t.Fatal are interesting. My rule of thumb is as follows:

• t.Fatal means either the test code failed, or we encountered a test failure so fundamental as to make it a waste of time to run any more test cases. For example, an error reading a test input file or golden file would be a t.Fatal.

• t.Error means the test failed, but we can still go on and run other cases. It may be that the specific pattern of failures gives us a clue as to the problem, whereas if you just stop after the first failure, you wouldn't have that information.

I never output any other information during tests (except for debugging), so that failures are very obvious and easy to identify.

I strongly encourage people to use subtests and t.Run() in 'real life', so I'm all in favour of getting Exercism students familiar with them too. Whether it's worth refactoring all the existing tests, as @leenipper asked, I'm not sure. If someone feels like doing it, more power to them (though we'll need to change the generators for tests with canonical data, too). I don't think the advantages are so overpowering as to be a no-brainer, though.

[martint17r]

I would go forward and refactor all the tests if we agree on a path forward ; that's just a way for me of giving back some of the profit excercism provides.

I am going to update this PR to include a changed generator as well later this week, so we can discuss that, too.

[martint17r]

@bitfield Regarding your semantics of t.Fatal and t.Error: As far as I understand it, t.Fatal works on the function level, so it basically returns from the TestFunc with appropriate output and function result. I usually write a test case per function and that matches the subtest semantics (as t.Fatal only terminates the subtest).

[leenipper]

Thanks for the helpful discussion. I'm now convinced that using t.Run is valuable.

Approach to consider:

• Use t.Run for the table driven tests
• One commit per exercise, typically
• Update generators where necessary to name the test cases
• Drop use of Logf for PASSED tests
• Use Errorf instead of Fatalf
• Use the testcase .name or .description in each Errorf("FAIL: %s ... ", ...)
• Allow tests to continue, so ignore return value from t.Run
• Integrate with multiple PRs for chunking the work by exercise.

[martint17r]

PTAL
I have updated the clock generator and test cases. I think there are a few things to discuss:

• the compact representation of the test cases is now gone, because I decided to put the description onto its own line. If you don't want this, the recommended max columns of 100 will get blown once or twice.
• I put the description in the first position, but it could be in the last one as well (especially if we want to keep the one line test definition).
• for the sub tests in this exercise it does not matter, whether we use t.Fatalf or t.Errorf because there only is one - so I did not change them.

Do you really want me to add the test name in front of every error? When a test fails, the test name is always printed so it looks kind of weird.

[leenipper]

Layout of test cases is fine. I don't mind loss of compactness for gaining description.
Description in first position is fine.
Yeah, when using Run, Fatalf and Errorf become equivalent, so no need for change.

Test name is not necessary for error output, since the underscored sub-test title shows enough. The most useful thing is showing the inputs given and the actual and desired output. So it's good.

For future reference, prefer this style on the commit subject line:
clock: add subtests and update generator

LG

[tehsphinx]

Stumbled over this issue by chance.

Here one more reason to use t.Run (and to implement it on all exercises now): We are in the process of defining the interface for running the tests automatically on all solutions (exercism/automated-tests#14). The output of the test runners is to be a json with a list of all the tests and if they passed/failed/skipped/etc. For that all tests need a name/description and it should be mentioned as an extra sub test in go test --json. Using t.Run does that. That makes it easy to parse the output of go test --json and generate the required output defined by the PR linked above.

[leenipper]

Thanks @tehsphinx for the comment. I think that bumps the incentive to move forward with integrating the use of t.Run.

@martint17r , do you want to continue adapting the exercise tests to use t.Run ? We can phase them in by exercise as time permits. I can help as well.

[martint17r]

Yes, I do want to help, but life got in the way ;)

There's no reason why we can't work in parallel - I think we agreed on creating a separate PR for each exercise, so there is very little conflict pontential.

[jmrunkle]

I am closing all PRs that are over a year old. Please reopen if you are still working on this.