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

Show evaluated expr when @test isequal(...) fails #22296

Merged
merged 2 commits into from
Jun 24, 2017

Conversation

omus
Copy link
Member

@omus omus commented Jun 8, 2017

Support showing the evaluated version of the expression when using @test with isequal:

julia> using Base.Test

julia> x, y = 1, 2
(1, 2)

julia> @test isequal(x, y)
Test Failed
  Expression: isequal(x, y)
   Evaluated: isequal(1, 2)
ERROR: There was an error during testing

@omus omus added the testsystem The unit testing framework and Test stdlib label Jun 8, 2017
base/test.jl Outdated
@@ -305,6 +305,12 @@ function get_test_result(ex)
terms[i] = esc(terms[i])
end
testret = :(eval_comparison(Expr(:comparison, $(terms...))))
elseif isa(ex, Expr) && ex.head == :call && ex.args[1] == :isequal
Copy link
Contributor

Choose a reason for hiding this comment

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

ex.args[1] in (:isequal, :isapprox, :≈) would also be useful if that comes out looking right

Copy link
Member Author

Choose a reason for hiding this comment

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

The operator is evaluated already. Unfortunately it isn't quite right:

julia> @test x  y
Test Failed
  Expression: x  y
   Evaluated: 1 isapprox 2

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like this wasn't tested very thoroughly:

julia> @test x  y  z
Test Failed
  Expression: $(Expr(:escape, :x)) $(Expr(:escape, :)) $(Expr(:escape, :y)) $(Expr(:escape, :)) $(Expr(:escape, :z))
   Evaluated: 1 isapprox 2 isapprox 3

I'll try to address this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed all of the above mentioned issues.

@omus
Copy link
Member Author

omus commented Jun 9, 2017

Changes now include:

  • Corrected @test x == y == z shown expression
  • Corrected @test x ≈ y shown evaluated expression
  • Added showing evaluated expression of @test isequal(...) and @test isapprox(...)
  • Lots of tests

@KristofferC
Copy link
Member

Any commits that can be backported?

@omus
Copy link
Member Author

omus commented Jun 9, 2017

Any commits that can be backported?

I'll break this into two PRs which separates the new features from the bugfixes

@omus
Copy link
Member Author

omus commented Jun 9, 2017

I couldn't make a clean separation between the fixes and the features. PR #22315 contains just the fixes and this PR extends that work with the features.

@omus
Copy link
Member Author

omus commented Jun 14, 2017

Sibling PR #22315 has been merged. I've rebased this PR which now makes the diff understandable. Reviews are welcome.

@omus
Copy link
Member Author

omus commented Jun 21, 2017

I think this is good to go. I'll merge at the end of JuliaCon unless there are objections.

@kshyatt kshyatt added the display and printing Aesthetics and correctness of printed representations of objects. label Jun 21, 2017
@tkelman tkelman merged commit bef5811 into master Jun 24, 2017
@tkelman tkelman deleted the cv/test-results-isequal branch June 24, 2017 16:29
@nalimilan
Copy link
Member

This seems to have broken StatsBase tests. MWE:

julia> @test isapprox(1, 1, atol=0)
Error During Test
  Test threw an exception of type MethodError
  Expression: isapprox(1, 1, atol=0)
  MethodError: no method matching Expr(::Symbol, ::Base.#isapprox, ::Int64, ::Int64; atol=0)
  Closest candidates are:
    Expr(::ANY...) at boot.jl:193 got unsupported keyword argument "atol"
  Stacktrace:
   [1] eval(::Module, ::Any) at ./boot.jl:236
   [2] eval_user_input(::Any, ::Base.REPL.REPLBackend) at ./repl/REPL.jl:67
   [3] macro expansion at ./repl/REPL.jl:98 [inlined]
   [4] (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:73
ERROR: There was an error during testing

@omus
Copy link
Member Author

omus commented Jun 25, 2017

I'll look into it.

omus added a commit that referenced this pull request Jun 26, 2017
The introduction of showing evaluated expressions for calls in #22296
only dealt with parsing function arguments without considering keyword
arguments. Mostly this oversight was due to the functionality being
added just for `isequal` call and `isapprox` was afterthought.
@omus
Copy link
Member Author

omus commented Jun 27, 2017

I just noticed that I need to some additional work to get splatting to work with the call syntax:

julia> @test isequal((1,1)...)
Error During Test
  Test threw an exception of type BoundsError
  Expression: isequal((1, 1)...)
  BoundsError: attempt to access 3-element Array{Any,1} at index [3:4]

Additionally, there is a corner case that was introduced in Julia 0.5 with ==:

julia> @test ==(1, (1, 2)...)
Error During Test
  Test threw an exception of type BoundsError
  Expression: ==(1,(1,2)...)
  BoundsError: attempt to access 4-element Array{Any,1} at index [5]

julia> ==(1, (1, 2)...)
ERROR: MethodError: no method matching ==(::Int64, ::Int64, ::Int64)

The @test is failing here as we should see the MethodError and not the BoundsError.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display and printing Aesthetics and correctness of printed representations of objects. testsystem The unit testing framework and Test stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants