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

Cleaned up test set PR #18738

Merged
merged 7 commits into from
Sep 30, 2016
Merged

Cleaned up test set PR #18738

merged 7 commits into from
Sep 30, 2016

Conversation

kshyatt
Copy link
Contributor

@kshyatt kshyatt commented Sep 30, 2016

@tkelman what are your thoughts?

@tkelman
Copy link
Contributor

tkelman commented Sep 30, 2016

This looks very good. I'm confused exactly what's going on with test/test.jl now though (looking at 8982605?w=1#diff-5222cfa9e61d516842beb8a60f07e58e). Did @testset always return the testset object itself, or is that a new behavior? Does it still throw? Do we need to be doing the try-catch in the "accounting" testset? Do the docs for Base.Test need updating? If so it can be a separate follow-up, but it would need doing.

@kshyatt
Copy link
Contributor Author

kshyatt commented Sep 30, 2016

@testset always returns itself, and it does still throw if it is the top-level testset (inner ones do not throw so that you can see if a file had multiple failures).

Docs shouldn't need much of an update, I can take a thorough look tomorrow and open a PR for it?

I think try-catch should stay for now.

@tkelman tkelman merged commit c40090f into JuliaLang:master Sep 30, 2016
@tkelman
Copy link
Contributor

tkelman commented Sep 30, 2016

dates/accessors allocates way more here than before merging, wonder why

@kshyatt kshyatt added the testsystem The unit testing framework and Test stdlib label Sep 30, 2016
@kshyatt
Copy link
Contributor Author

kshyatt commented Sep 30, 2016

@quinnj and @Sacha0, any thoughts?

@kshyatt kshyatt deleted the ksh/testset2 branch September 30, 2016 16:11
@ihnorton
Copy link
Member

@kshyatt was it intentional to remove summary printing? I only see one call to print_test_results in test/runtests.jl, whereas the summaries used to be printed by finish.

Ref this discourse question: https://discourse.julialang.org/t/test-summary-in-julia-0-6/1461

@KristofferC
Copy link
Member

KristofferC commented Jan 14, 2017

IIRC the argument was that the testsets printing was to verbose for use in Base and they therefore got removed. Imo this should instead probably just be explicitly disabled by the Base test runner somehow and the default left like it used to be.

@Arkoniak
Copy link
Contributor

Real problem was in this log: https://travis-ci.org/Arkoniak/MXNet.jl/jobs/191010907. Error message is on the row 2726 and "ERROR: LoadError: Some tests did not pass: 347 passed, 0 failed, 3 errored, 0 broken." is almost useless. I do not think, that +20 rows would be too verbose, but they will make search much more easier in cases like this.

@TotalVerb
Copy link
Contributor

Unfortunately Base.Test.print_test_results is not available on 0.5. If this is the way forward, then it should be added to Compat.

@StefanKarpinski
Copy link
Member

OMG, thank goodness this finally got merged!

@tkelman
Copy link
Contributor

tkelman commented Jan 26, 2017

in september...

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.

7 participants