Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include parent testset descriptions in immediate test report #32938

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tkf
Copy link
Member

@tkf tkf commented Aug 17, 2019

With this PR, the failure messages printed immediately after the test failure (i.e., not Test Summary) include testset descriptions of all parent test sets. This clarifies the context in which the failure occurs. This is important especially in Julia CI (Base.runtests) which does not print Test Summary. Without a change like this, heavily parameterized test like stdlib/LinearAlgebra/test/addmul.jl cannot produce any meaningful failure message (JuliaLang/LinearAlgebra.jl#655).

Some examples. The contents between Test Set: and Test Failed at are added in this PR:

julia> @testset "first level test set" begin @test false end
Test Set: first level test set
Test Failed at REPL[3]:1
  Expression: false
Stacktrace:
 [1] top-level scope at REPL[3]:1
 [2] top-level scope at /home/takafumi/repos/watch/_wt/julia/parentset/usr/share/julia/stdlib/v1.3/Test/src/Test.jl:1137
 [3] top-level scope at REPL[3]:1
Test Summary:        | Fail  Total
first level test set |    1      1
ERROR: Some tests did not pass: 0 passed, 1 failed, 0 errored, 0 broken.

julia> @testset "first level test set" begin
           @testset "second level test set" begin
               @testset "third level test set" begin
                   @test false
               end
           end
       end
Test Set:
first level test set
  second level test set
    third level test set
Test Failed at REPL[4]:4
  Expression: false
Stacktrace:
 [1] top-level scope at REPL[4]:4
 [2] top-level scope at /home/takafumi/repos/watch/_wt/julia/parentset/usr/share/julia/stdlib/v1.3/Test/src/Test.jl:1137
 [3] top-level scope at REPL[4]:4
 [4] top-level scope at /home/takafumi/repos/watch/_wt/julia/parentset/usr/share/julia/stdlib/v1.3/Test/src/Test.jl:1137
 [5] top-level scope at REPL[4]:3
 [6] top-level scope at /home/takafumi/repos/watch/_wt/julia/parentset/usr/share/julia/stdlib/v1.3/Test/src/Test.jl:1137
 [7] top-level scope at REPL[4]:2
Test Summary:            | Fail  Total
first level test set     |    1      1
  second level test set  |    1      1
    third level test set |    1      1
ERROR: Some tests did not pass: 0 passed, 1 failed, 0 errored, 0 broken.

@andreasnoack andreasnoack added the testsystem The unit testing framework and Test stdlib label Aug 19, 2019
@andreasnoack
Copy link
Member

I think this is a good change. Please speak out if you have any objections.

@vtjnash
Copy link
Member

vtjnash commented Nov 13, 2019

WeakRef seems like the wrong choice, why not just a normal reference? Otherwise, seems like @andreasnoack already approved this and so it's good to merge.

@tkf
Copy link
Member Author

tkf commented Nov 13, 2019

I removed WeakRef

@tkf
Copy link
Member Author

tkf commented Nov 19, 2021

@vtjnash Do you want to review the post-merge diff? Or can I merge this?

@DilumAluthge
Copy link
Member

You'll need to fix the doctest failures.

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember ever looking at this before

@tkf tkf added the merge me PR is reviewed. Merge when all tests are passing label Nov 19, 2021
@rfourquet
Copy link
Member

rfourquet commented Nov 19, 2021

Unless I'm missing something, given that the parent testsets are stored in the task local storage, it doesn't seem necessary to add parent as a field of DefaultTestSet nor to add the subtestset function, strictly for the purpose of including parent testsets in test report (it might be useful for other reasons though?)

@vtjnash vtjnash removed the merge me PR is reviewed. Merge when all tests are passing label Nov 19, 2021
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rfourquet for review. I agree: since we got to this record function by calling get_testset, and that function had the full list of testset parents already, it looks like we don't need to add a field to do this printing.

@tkf
Copy link
Member Author

tkf commented Nov 19, 2021

Thanks, @rfourquet. Yes, that's a good point. I don't remember why I didn't take that strategy. It's probably easy to fix.

@tkf tkf marked this pull request as draft November 19, 2021 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testsystem The unit testing framework and Test stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants