search

exercism / go

Exercism exercises in Go.
https://exercism.org/tracks/go
MIT License
948 stars 644 forks source link

Migrate data-driven tests to subtests #2098

Open artamonovkirill opened 2 years ago

artamonovkirill commented 2 years ago

Following this Slack thread and this PR discussion

In the track, there are still some data-driven tests like:

func TestSomething(t *testing.T) {
    for _, test := range tests {
        ... some test code ...
        t.Logf("PASS: %s", test.description)
    }
}

Such tests could benefit from rewriting them with subtests:

func TestSomething(t *testing.T) {
    for _, test := range tests {
        t.Run(test.description, func(t *testing.T) {
            ... some test code ...
        }
    }
}

Pros:

Cons:

A (possibly incomplete) list of exercises without subtests:

artamonovkirill commented 2 years ago

Slack thread screenshot, if it gets deleted:

Screenshot 2022-02-11 at 09 33 21
andrerfcsantos commented 2 years ago

If anyone wants to work on this issue, it is OK (and even recommended) the make PR's for individual exercises - working on this issue doesn't mean necessarily fixing all exercises yourself all at once. On the contrary, several people are more than welcome to make PR's for individual exercises where this is a problem.

We'll use this issue as a tracking issue to track all the PR's related to this.

junedev commented 2 years ago

@andrerfcsantos Did you already check whether this issue might also be related to code that produced via the generator? I have a hunch this is not just a matter of making individual PRs but this might be a generator issues that needs to be addressed.

andrerfcsantos commented 2 years ago

@junedev Some exercises suffering from this problem do have generators, but others don't. So I don't think it's a generator specific problem, although for the exercises that do have the generator, we must adjust it. Your suggestion would be to add this capability to the generator first and then work on individual exercises? (Although exercises without a generator currently don't have this dependency)

eklatzer commented 1 year ago

Hey, to get an overview I used the following script:

#! /bin/bash

for test in ./exercises/*/*/*_test.go; do
    if [[ $test == *cases_test* ]] # ignore cases_test.go
    then
        continue
    fi
    testFileWithoutSubtest=$(grep -Li "t.Run" "${test}")
    if [ ! -z "$testFileWithoutSubtest" ]
    then
        echo "- [ ] ${testFileWithoutSubtest}"
    fi
done

Seems like the following exercises still need subtests (some might not need subtests):

Might not be needed:

eklatzer commented 1 year ago

@junedev or @andrerfcsantos Could one of you check the exercises without a :heavy_check_mark: and check if one of them needs usage of subtests, I am not sure about all of them, so double checking would be nice :smile: Thanks in advance https://github.com/exercism/go/issues/2098#issuecomment-1213004176

andrerfcsantos commented 1 year ago

Left my analysis below. This was a quick analysis of each exercise. If there's an exercise for which I wrote that we maybe could add subtests, but when tackling you conclude that is not viable, feel free to say so.

I also noticed some exercises are missing generators. Adding generators is of course optional for this task, but some refactoring into subtests could be made in a way that makes writing a future generator easier. But since you are our expert at writing generators, feel free to also add generators and we'll size the PRs accordingly ;) But it's totally ok to do some of the refactoring into subtests now and leave the generator for later, especially because I feel the generators for some exercises will require quite some work.

Seems robot_simulator_test.go might not need it but seems robot_simulator_step2_test.go and robot_simulator_step3_test.go need it, as each iteration of the loop seems to be a test?

grade_school_test.go might not need right now, but with a little refactoring maybe. For instance, it seems that TestAddMoreSameGrade and TestAddDifferentGrades are doing the same things, they take a list of students and add them. Looking at canonical-data.json, it seems these tests could be grouped by "property" and each one could be a subtest.

I think it's safe to skip this one - it only has one test and I don't think it'll ever change, so it's fine to be a regular test.

The tests here were tailor-made for the track and there's only a single test for each function, so I think we can also skip this one.

A similar situation to Grade School - there's some duplicated code between functions which could be grouped into single functions and maybe we could add a generator. Although I expect the creation of the generator and the refactoring to be significantly harder and require more thought than Grade School.

These could also be 2 separate tasks - 1 to refactor into subtests and another one to add the generator.

There seems to also be some duplicated code in this one between functions, so adding subtests at least for some tests could be a good idea. The tests seem to be a list of instructions, maybe the subtest definition itself can contain a list of string containing the operation to do at each stage.

Only has one test currently and canonical data also only specify a single test, so it's fine to leave as it is. We could add a subtest, but it seems overkill for this case.

Similar to Simple Linked List - the tests can be refactored into subtests in a similar way.

Looks like some functions are looping over tests, so maybe they are good candidates for subtests.

Also some duplication in the code that maybe can be refactored into subtests.

Some duplication in code, the function testWrite also seems to iterate over a list of tests, but not using subtests, maybe a good candidate for them?

Safe to skip this one, tests are custom for this exercise and there isn't really code duplication.

I also don't see an obvious way to refactor this into subtests.