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.

Sign up for GitHub

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

Jump to bottom

bring back testset printing #20221

Merged
merged 1 commit into from
Jan 25, 2017
Merged

bring back testset printing #20221

merged 1 commit into from
Jan 25, 2017

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented Jan 24, 2017 •
edited
Loading

Should restore things like they were on 0.5 while keeping things like they are for Base tests (no testset printing).

Review with https://github.com/JuliaLang/julia/pull/20221/files?w=1

@KristofferC KristofferC added the testsystem The unit testing framework and Test stdlib label Jan 24, 2017
@KristofferC
Copy link
Member Author

actually this fails to print the testset when there is a failure, will update

@KristofferC KristofferC force-pushed the kc/dude_where_is_my_testset_printing branch from 1768324 to 07e028b Compare January 24, 2017 20:33
@KristofferC
Copy link
Member Author

done

@TotalVerb
Copy link
Contributor

TotalVerb commented Jan 24, 2017 •
edited
Loading

How does this handle nested testsets? In 0.5, only top-level testsets were printed.

@KristofferC
Copy link
Member Author

Like on 0.5.

julia> @testset "set" begin
             # write your own tests here
             @test 1 == 1
             @testset "sesst" begin
                 @test 1 == 1
             end
             end
Test Summary: | Pass  Total
  set         |    2      2

tkelman
tkelman reviewed Jan 24, 2017
View reviewed changes
test/runtests.jl
@@ -62,7 +62,7 @@ cd(dirname(@__FILE__)) do
test = shift!(tests)
local resp
try
resp = remotecall_fetch(runtests, p, test)
resp = remotecall_fetch((t) -> (Base.Test.DISABLE_TESTSET_PRINTING[] = true; runtests(t)), p, test)
Copy link
Contributor

Choose a reason for hiding this comment

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

do in the runtests function so it also applies to the node 1 tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea

@tkelman
Copy link
Contributor

tkelman commented Jan 24, 2017

double negatives are confusing, would be better to call it enable and switch the sense

@KristofferC
Copy link
Member Author

updated to put the disabling in runtests instead.

In this case I think that the negative is warranted since we are explicitly disabling the printing.

@tkelman
Copy link
Contributor

tkelman commented Jan 24, 2017 •
edited
Loading

Disabling printing would be done by setting enable to false. More direct than enabling printing by setting disable to false. We've preferred this same direction in command line arguments and elsewhere. Whatever the default is, don't flip the sense of boolean options.

@KristofferC
Copy link
Member Author

KristofferC commented Jan 24, 2017 •
edited
Loading

This is better?

@tkelman
Copy link
Contributor

tkelman commented Jan 24, 2017

the setter function isn't really necessary, it can be a ref, just that if it's a boolean then the sense should be enable=true, disable=false

@kshyatt kshyatt added the display and printing Aesthetics and correctness of printed representations of objects. label Jan 24, 2017
@KristofferC
Copy link
Member Author

OK but this is like gc_enable now and if we decide to export it at some point then this is better.

tkelman
tkelman reviewed Jan 24, 2017
View reviewed changes
test/testdefs.jl
@@ -1,7 +1,7 @@
# This file is a part of Julia. License is MIT: http://julialang.org/license

function runtests(name, isolate=true)
Base.Test.DISABLE_TESTSET_PRINTING[] = true
Base.Test.testset_print_enable(false)
Copy link
Contributor

@tkelman tkelman Jan 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

does this turn off testset display for everyone else after having been run once? should restore past value when done

Copy link
Member Author

Choose a reason for hiding this comment

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

This is only called from runtests.jl which spawn this in a new process?

Copy link
Contributor

Choose a reason for hiding this comment

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

That won't necessarily be the case if some form of #19567 happens. It's a global-state trap, best to avoid leaving it hiding for anyone who wants to refactor this to get surprised by later.

Copy link
Member Author

@KristofferC KristofferC Jan 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

https://github.com/JuliaLang/julia/pull/20221/files?w=1 This is not intended to be a public api where you can disable and enable at will. Right now it is like 0.5 with an escape hatch for base to disable it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then why did you make a setter function for it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Cleaner imo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary if it's called in exactly one place and not intended to be ever touched by anyone else.

Copy link
Member Author

Choose a reason for hiding this comment

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

In case some form of #19567 happens then it is already there.

Copy link
Contributor

@tkelman tkelman Jan 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

If you're anticipating it to be used in the future, then manage it as if someone may have been setting it - save its actual past value and restore it.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok done

@KristofferC KristofferC force-pushed the kc/dude_where_is_my_testset_printing branch from 9fa34bc to 48fcf2e Compare January 25, 2017 07:53
@KristofferC KristofferC force-pushed the kc/dude_where_is_my_testset_printing branch from 48fcf2e to 2ecdcf3 Compare January 25, 2017 08:32
tkelman
tkelman approved these changes Jan 25, 2017
View reviewed changes
Copy link
Contributor

@tkelman tkelman left a comment

Choose a reason for hiding this comment

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

lgtm as long as it doesn't cause any issues on CI

@KristofferC KristofferC mentioned this pull request Jan 25, 2017
Merged
@KristofferC KristofferC merged commit 9659c38 into master Jan 25, 2017
@KristofferC KristofferC deleted the kc/dude_where_is_my_testset_printing branch January 25, 2017 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkelman tkelman tkelman approved these changes

@kshyatt kshyatt kshyatt approved these changes

Assignees
No one assigned
Labels
display and printing Aesthetics and correctness of printed representations of objects. testsystem The unit testing framework and Test stdlib
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@KristofferC @TotalVerb @tkelman @kshyatt