-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
RFC: Add information to passing tests #36879
RFC: Add information to passing tests #36879
Conversation
858c0fb
to
f54b2a4
Compare
To retain the same output for a This PR is ready for a code review. |
f54b2a4
to
116a4f0
Compare
Please can someone review this? Cheers |
Giving a summary of the effect of this would be helpful. Who would be good to review? GitHub suggests @JeffBezanson, but that may just be because the did some sweeping change that touched this code. |
Maybe @oxinabox who filed the issue. Would also be great to see some numbers on whether/how this affects the time to run Julia's own tests. |
Thanks both for your comments. I'll run a comparison on With this PR, the information (tested expression and the value) of a succfessul test will be stored in the The storing of information in the |
Julia test times averaged across two runs: With this PR: 42.5 mins The difference could be just what else was running on my machine at the same time, it would be useful if someone else could repeat the test so the results don't just rely on one laptop. I have been struggling to actually run Julia's tests as the generating precompile statements step hangs (>1 hr) whilst building, so have had to resort to running |
Note that @mmiller-max is now working on the package that I was working on when I filed that issue. I will review this PR presently |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs tests that it actually is storing the correct thing.
stdlib/Test/src/Test.jl
Outdated
elseif t.test_type === :test && t.data !== nothing | ||
# The test was an expression, so display the term-by-term | ||
# evaluated version as well | ||
print(io, "\n Evaluated: ", t.data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we want to display this?
Given someone has created a test result object, why not show its fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see it being argued either way.
One the one hand the information is there so why not display it to give the user more information, but on the other hand if a test has passed the user may not want the details of the passing of that test to fill up the REPL. I typically only see the Test Passed
output when manually stepping through tests to find the failing ones, and wouldn't be interested in knowing the details of the ones that passed. Normally the tests are in testsets
and therefore the pass information isn't shown either way.
I kept it to the latter as that's what was originally happening before the changes here, albeit because that information wasn't stored so couldn't be displayed anyway.
I'm leaning towards not displaying it, but its definitely worth a discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I am not sure when this is displayed, except if @test
is used on its out, in the REPL.
do @testsets
call display on the tests with in them? If so, i agree we want to minimize noise and not display this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's only displayed when @test
is used on its own in the REPL, as you say. @testsets
do not call display so makes no difference for them.
I still think it is more helpful to not display this information, but actually thinking about it @test_throws
already displays the exception type, so to be consistent we should either remove that printing too, or keep them all in.
Is this a question for slack/a wider group of people?
Current situation is:
julia> @test 1==1
Test Passed
julia> @test_throws ErrorException throw(ErrorException("Msg"))
Test Passed
Thrown: ErrorException
Full printing:
julia> @test 1==1
Test Passed
Expression: 1 == 1
Evaluated: 1 == 1
julia> @test_throws ErrorException throw(ErrorException("Msg"))
Test Passed
Expression: throw(ErrorException("Msg"))
Thrown: ErrorException
Or no printing:
julia> @test 1==1
Test Passed
julia> @test_throws ErrorException throw(ErrorException("Msg"))
Test Passed
116a4f0
to
0492c37
Compare
Bump. I think the following two points need addressing to move this PR forward:
Cheers |
Bump Just wondering if this has been discussed on a triage call or if that's still pending? |
This looks fine to me. We needed to make sure not to store the information (by default), but it didn't need to be removed this early. Please ping me when you rebase for review. For the questions:
|
2380b12
to
d2a3576
Compare
d2a3576
to
a67d42e
Compare
@vtjnash thanks for following up. Rebased and ready for another review. Without the changes for printing there are quite a few doctest changes due to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This restores the behaviour of how `@test` shows a test pass in Julia v1.0 - v1.6, reverting the display changes that resulted when we began storing more info in the `Pass` type in JuliaLang#36879. Closes JuliaLang#43794.
This restores the behaviour of how `@test` shows a test pass in Julia v1.0 - v1.6, reverting the display changes that resulted when we began storing more info in the `Pass` type in JuliaLang#36879. Closes JuliaLang#43794.
This restores the behaviour of how `@test` shows a test pass in Julia v1.0 - v1.6, reverting the display changes that resulted when we began storing more info in the `Pass` type in JuliaLang#36879. Closes JuliaLang#43794.
This restores the behavior of how `at-test` shows a test pass in Julia v1.0 - v1.6, reverting the display changes introduced when storing more info in the `Pass` type in JuliaLang#36879, closes JuliaLang#43794.
This restores the behavior of how `at-test` shows a test pass in Julia v1.0 - v1.6, reverting the display changes introduced when storing more info in the `Pass` type in JuliaLang#36879, closes JuliaLang#43794.
This restores the behavior of how `at-test` shows a test pass in Julia v1.0 - v1.6, reverting the display changes introduced when storing more info in the `Pass` type in JuliaLang#36879, closes JuliaLang#43794.
Closes #25483.
Summary
With this PR, the information (tested expression and the value) of a succfessul test will be stored in the
Pass
Result
rather than these fields being set tonothing
. This will be the case for both@test
and@test_throws
. There will be no change toDefaultTestSet
behaviour, as the current behaviour ofrecord(ts:DefaultTestSet, t::Pass)
discards any information in thePass
result and just increments the number of successful tests. Storing the information allows other testsets to use this information, for example in test report generation.More info
Following the discussion in the issue, it seems like the changes made by #20150 mean that information can be stored when tests
Pass
as the whole result is discarded byDefaultTestSet
s.This allows other
CustomTestSet
s to use this information, which is important if this information is required, for example by a test case management suite.