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

better handling of runtest errors? #37809

Merged
merged 1 commit into from
Oct 6, 2020
Merged

better handling of runtest errors? #37809

merged 1 commit into from
Oct 6, 2020

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Sep 29, 2020

Looking at #37789, trying to interpret how to avoid these problems in the future

test/testdefs.jl Outdated Show resolved Hide resolved
@@ -214,6 +227,25 @@ function Base.show(io::IO, t::Broken)
end
end

# Types that appear in TestSetException.errors_and_fails we convert eagerly into strings
# other types we convert lazily
function Serialization.serialize(s::Serialization.AbstractSerializer, t::Pass)
Copy link
Member

Choose a reason for hiding this comment

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

Is there any downside to stringifying everything eagerly so we don't need to add serialize methods?

Copy link
Member Author

Choose a reason for hiding this comment

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

(I was sure I replied already—twice—but somehow my comments are gone?)

There's a possible performance concern (we almost never need these strings, but almost always need the other ones). More importantly though, there's a number of consumers that introspect the result of Pass to do further tests on the result.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, it's also possible that everything should be preserved as lazy until serialize (refs #24847)

@vtjnash vtjnash force-pushed the jn/runtest-errors branch 2 times, most recently from cf775ac to 2889b88 Compare September 30, 2020 23:27
By serializing both tests and backtraces (as strings), and trying to
ensure other parts are not going to mess them up along the way.

Hopefully this should improve error message reporting reliability in our
Base runtests.
@vtjnash vtjnash force-pushed the jn/runtest-errors branch from 2889b88 to ee43966 Compare October 1, 2020 14:51
@vtjnash vtjnash marked this pull request as ready for review October 1, 2020 14:53
@vtjnash vtjnash requested a review from JeffBezanson October 1, 2020 21:03
@vtjnash vtjnash merged commit 7d3dac4 into master Oct 6, 2020
@vtjnash vtjnash deleted the jn/runtest-errors branch October 6, 2020 21:26
@oxinabox
Copy link
Contributor

oxinabox commented Oct 19, 2020

Stringifying all fields in test results breaks some of my code.
#37809
I suspect it might break other code that defines custom testsets, that specially handle various kinds of results.
Though I am not sure, it doesn’t break TestReports.jl as all TestReports.jl does is stringify the information itself.
Should I open an issue?

rai-release pushed a commit to RelationalAI-oss/XUnit.jl that referenced this pull request Feb 4, 2021
XUnit calls Distributed.myid through Test (i.e. it calls Test.myid).
Distributed.myid is not re-exported from Test, and ergo is not part
of Test's public API and cannot be relied upon.

Case in point, Test's dependency on Distributed was removed in
JuliaLang/julia#37809, and so Test.myid
fails under Julia 1.6.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants