Do not include parent test in test results when using sub-tests

[ErikSchierboom]

The recommendation for Go test cases is usually to use table-based tests. This is also how the samples defined in the tests directory are defined. However, running the test runner on these results also results in the parent (root) test being included in the test results, which is problematic when trying to display what went wrong for which input. The test runner should omit the parent/root test from the test results output.

As an example, consider the second example's test suite:

package sample

import (
    "testing"
)

var tests = []struct {
    name string
    fail bool
}{
    {
        name: "first test",
        fail: false,
    },
    {
        name: "second test",
        fail: true,
    },
    {
        name: "third test",
        fail: true,
    },
}

func TestSample(t *testing.T) {
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            if tt.fail {
                t.Fail()
            }
        })
    }
}

If we run the test runner on these tests using go test --json ./tests/2 | go run cmd/testoutput/main.go > tests/2/results.json, we get the following output:

{
    "status": "fail",
    "tests": [
        {
            "name": "TestSample",
            "status": "fail",
            "expected": "",
            "actual": "",
            "message": "\n=== RUN   TestSample\n\n--- FAIL: TestSample (0.00s)\n"
        },
        {
            "name": "TestSample/first_test",
            "status": "pass",
            "expected": "",
            "actual": "",
            "message": "\n=== RUN   TestSample/first_test\n\n    --- PASS: TestSample/first_test (0.00s)\n"
        },
        {
            "name": "TestSample/second_test",
            "status": "fail",
            "expected": "",
            "actual": "",
            "message": "\n=== RUN   TestSample/second_test\n\n    --- FAIL: TestSample/second_test (0.00s)\n"
        },
        {
            "name": "TestSample/third_test",
            "status": "fail",
            "expected": "",
            "actual": "",
            "message": "\n=== RUN   TestSample/third_test\n\n    --- FAIL: TestSample/third_test (0.00s)\n"
        }
    ]
}

We can see that besides the three sub-tests, there is also an entry for the parent test:

{
    "name": "TestSample",
    "status": "fail",
    "expected": "",
    "actual": "",
    "message": "\n=== RUN   TestSample\n\n--- FAIL: TestSample (0.00s)\n"
},

This entry should be omitted from the test results output.

[tehsphinx]

Wondering what we should do when there are 2 or more parent tests with sub-tests... still ignore the parent tests?

[ErikSchierboom]

Wondering what we should do when there are 2 or more parent tests with sub-tests... still ignore the parent tests?

So in case something like this is used:

func TestSample1(t *testing.T) {
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            if tt.fail {
                t.Fail()
            }
        })
    }
}

func TestSample2(t *testing.T) {
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            if tt.fail {
                t.Fail()
            }
        })
    }
}

If so, the parent tests should still be ignored.

[tehsphinx]

What about this:

func TestSample(t *testing.T) {
    if err := someInit(); err != nil {
        t.Error(err)
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            if tt.fail {
                t.Fail()
            }
        })
    }
}

Now the parent test can fail independently of the sub tests...

[tehsphinx]

I'm not really in favour of implementing this issue. Maybe this could be better solved with a UI design solution. Go is not the only language with parent and child tests.

[ErikSchierboom]

I'm not really in favour of implementing this issue. Maybe this could be better solved with a UI design solution. Go is not the only language with parent and child tests.

Thanks for explaining your reasoning. I'll talk to @iHiD to see what we can do!

[iHiD]

So @ErikSchierboom suggested that any tests that are not present in the exercise's config.json file are excluded from output. I think I think this is a good idea.

One question/clarification for me, could the parent test ever fail, but the child tests pass?

[tehsphinx]

In my example above, yes. Of course it is up to us to write it that way and on exercism I don't really see complex setup routines that could fail. In the real world they definitely do. Although if the setup fails, I'd most likely not even run the child tests. But then all I have is the parent.

I'm also not a fan of suppressing the parent test as that is not how the usual output of Go tests looks like. Of course we have to find a unified way to represent the tests... So I am still wondering about showing the parent and sub-tests in a hierarchical order as they usually are in many languages. Actually I can't really think of a test library I have used that didn't have some hierarchy or grouping for tests.

Maybe not in the first step... but later on.

[ErikSchierboom]

So I am still wondering about showing the parent and sub-tests in a hierarchical order as they usually are in many languages. Actually I can't really think of a test library I have used that didn't have some hierarchy or grouping for tests.

There are indeed other languages that do this kind of sub-grouping of tests.

Maybe not in the first step... but later on.

This would also be my suggestion, to start with the simplest solution first and then possibly expand on that later.

[iHiD]

I'm going to give up on trying to think about this for a while. I'll come back to it once the Experiment is launched and I have some brainspace. Is that ok?

[ErikSchierboom]

Sure. I've put this ON HOLD (in the title).

[angelikatyborska]

Is this still on hold? I am testing the Golang track from a perspective of an absolute Golang noob and I found this every surprising. I actually thought that the debugging via fmt.Println is broken because the parent test doesn't show any output from those function calls (the children do).

[iHiD]

I have no idea where this is at now. The issue is nearly 2 years old, so I guess we have lots more ideas/info now. Thoughts everyone?

[angelikatyborska]

From the perspective of a student that doesn't understand golang's table-based tests yet, if those parents test cannot be deleted altogether, it would be preferable if they were reported AFTER all of their children. The platform always highlights the first test failure, and if that's the parent test failure, I need to look for the children failures to know which problem to tackle next.

[iHiD]

@ekingery Any thoughts on this?

[ekingery]

@ekingery Any thoughts on this?

From an implementation standpoint, we could probably loop through all of the "output" failures and drop failed tests without a "/" (not a subtest), when a failed subtest of the same name (with a "/") exists. For reference, we already split test names like this in the splitTestName function.

It does seem like an odd workaround and could potentially result in some unexpected behavior depending on how the tests are working / structured. But it should be fine for most cases and it makes sense in terms of wanting to only show the most specific failure. Unfortunately I won't have time to implement this in the near future, but I'd be happy to review a PR.

[junedev]

I think this issue is also the cause for the original issue reported in #44.

If I find some time, I will try out the solution proposed by @ekingery.

[junedev]

If anyone else want to pick this up, feel free to claim it. I haven't started yet.

[junedev]

I will work on this. I looked at all the existing test files that use sub-tests. None of them contain code of the form that @tehsphinx mentioned (https://github.com/exercism/go-test-runner/issues/8#issuecomment-572259999) that the solution proposed by @ekingery cannot handle. So I think that solution (https://github.com/exercism/go-test-runner/issues/8#issuecomment-932854153) is fine for now and I will implement that one.