Erroneous return code and "test passed" message

[philbit]

We've recently had trouble with TestReports >= v1.0.0. Our testing pipelines were erroneously succeeding even though some tests were failing (which was correctly documented in the generated xml). We've successfully tracked this down to the fact that the runner code from gen_runner_code in src/runner.jl only checks for failed results and determines the exit code after flattening the results.

This MWE shows the problem with that:

using Test, TestReports
ts = @testset ReportingTestSet "" begin
    @testset "mytestset" begin
        @test 1==1
        @testset "level2testset" begin
            @test 1==2
        end
    end
end

println("Before flattening: $(any_problems(ts))")
fr = TestReports.flatten_results!(ts)
println("After flattening: $(any_problems(ts))")

with the output

level2testset: Test Failed at /home/philip/Documents/DiskCellDesign/TestReports.jl/src/bug.jl:6
  Expression: 1 == 2
   Evaluated: 1 == 2

Stacktrace:
# omitted for brevity

Test Summary:   | Pass  Fail  Total  Time
mytestset       |    1     1      2  0.0s
  level2testset |          1      1  0.0s
Before flattening: true
After flattening: false

I think I know exactly what's happening: _flatten_results! is using the original testset "mytestset" to wrap any bare results, replacing the original vector of results, so all results which were already themselves testsets, like "level2testset" in the example, simply disappear from the testset. Instead, they are returned via the resulting flattened vector, fr in the above case. This is why the xml written by the runner code is correct, but the test with any_problems fails. This problem doesn't occur if the testset doesn't contain any bare results that need to be wrapped, because the original testset is not modified in this case.

This seems to be easy to fix by either testing with any_problems before flattening, or using any(any_problems.(fr)) on the flattened vector instead.

But before I make a pull request to implement this, I would like to confirm that this is not just an unintentional side effect of flatten_results!. I.e., is flatten_results! indeed meant to make the original testset unusable, so the problem should be fixed in the runner code (and not in _flatten_results! itself)?

[oxinabox]

Cc @omus

[oxinabox]

But before I make a pull request to implement this, I would like to confirm that this is not just an unintentional side effect of flatten_results!. I.e., is flatten_results! indeed meant to make the original testset unusable, so the problem should be fixed in the runner code (and not in _flatten_results! itself)?

It is not intended that flatten_results! should make any testset unusuable.

So go ahead and make the PR

[philbit]

It is not intended that flatten_results! should make any testset unusuable.

Ok, in this case the problem would be deeper than I thought, i.e. it would need to be fixed in _flatten_results! itself, rather than just fixing the test in the runner code where flatten_results! is used. May I ask why flatten_results! has a "!" in its name if it is not actually supposed to alter the original testset?

The way _flatten_results! currently written, it explicitly reuses existing testsets to wrap bare results (there is even a comment in the code acknowledging this). I was almost sure this is the way it's supposed to be and the fact that it alters the testsets was just "forgotten" in the runner code. Changing _flatten_results! would require deep copies or something similar (and the "!" could also be removed).

In contrast, my original solution would have been to simply alter the runner code and test with any_problems before applying flatten_results!. That would have left the _flatten_results! implementation as lightweight as it is (without copies).

Are you sure _flatten_results! should be made non-mutating, instead of just using it correctly at the caller site?

[oxinabox]

Oh sorry I misunderstood what you means by unusuable.
Yes, it is correct that it mutates, but the result is intended to still be useful

Its been 6 years since i wrote this code.

[omus]

This seems to be easy to fix by either testing with any_problems before flattening, or using any(any_problems.(fr)) on the flattened vector instead.

I believe the solution is to define any_problems(::Vector{<:AbstractTestSet}). This was and oversight in the runner code when we changed how flattening worked. The easy fix is to check the results before we flatten which avoids having to make an extra copy.

[philbit]

This was and oversight in the runner code when we changed how flattening worked. The easy fix is to check the results before we flatten which avoids having to make an extra copy.

Exactly. This was also my original idea and I think this is the much better solution compared to rewriting how flattening works (it's impossible to preserve the original testset structure without creating lots of copies or making certain assumptions).