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

added helpful failure massages for tests #288

Merged
merged 5 commits into from
Dec 15, 2023

Conversation

nomadbl
Copy link
Contributor

@nomadbl nomadbl commented Nov 24, 2023

A small PR to help debugging packages once tests fail.
test_rrule now indicates which cotangent did not match the finite difference result, which helps to narrow down bugs.

@oxinabox
Copy link
Member

Need to update the doctests to agree with this.
Also can you post an example here of what the output looks like for 1 or 2 different failures?

@nomadbl
Copy link
Contributor Author

nomadbl commented Nov 27, 2023

Here is an example of how failing tests look like, taken from some tests of a private package of mine:

 Expression: isapprox(actual, expected; kwargs...)
  Problem: cotangent for input 3, 1×1×1×1×2 Array{Float32, 5}
   Evaluated: isapprox([12.567303;;;;; 7.8655686], [10.600296;;;;; 8.684607]; rtol = 1.0e-9, atol = 0.0001

@nomadbl
Copy link
Contributor Author

nomadbl commented Nov 27, 2023

I've updated the doctests by running make.jl with doctest=:fix, and added a filter for test timings so it doesn't fail on that.

A few of this packages unit tests are failing, but it seems to me that this is not to do with this PR. If this is incorrect I will investigate further.

@nomadbl
Copy link
Contributor Author

nomadbl commented Nov 27, 2023

I've expanded the docs a bit by messing up the rrule in the docs and demonstrating how the tests indicate where to find the issue. This also includes the improved error message which I've included here which helps with this endeavor.

Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

This seems good, can you fix the errors reported by CI?

Copy link

codecov bot commented Dec 14, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (49a0324) 93.50% compared to head (f7388ec) 93.71%.

Files Patch % Lines
src/testers.jl 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #288      +/-   ##
==========================================
+ Coverage   93.50%   93.71%   +0.21%     
==========================================
  Files          12       12              
  Lines         354      350       -4     
==========================================
- Hits          331      328       -3     
+ Misses         23       22       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nomadbl
Copy link
Contributor Author

nomadbl commented Dec 14, 2023

That should do it for the main CI tests.
The doctests is failing because I introduced a failing case in the docs on purpose. How should I mark it so that the test suite recognizes this as okay? Would @test_broken be appropriate if I want to preserve the overall error message in the docs?

@nomadbl
Copy link
Contributor Author

nomadbl commented Dec 14, 2023

Regarding the failing CI tests for ChainRules - I think this is a separate issue but I don't know why it happens. These are type inference related tests failing for SVector being expected as Any. For example:

julia> test_rrule(sum, abs, @SVector[1.0, -3.0])
test_rrule: sum on typeof(abs),SVector{2, Float64}: Error During Test at /home/lior/.julia/dev/ChainRulesTestUtils/src/testers.jl:202
  Got exception outside of a @test
  return type Tuple{ChainRulesCore.NoTangent, ChainRulesCore.NoTangent, SVector{2, Float64}} does not match inferred return type Tuple{ChainRulesCore.NoTangent, ChainRulesCore.NoTangent, Any}
  Stacktrace:
    [1] error(s::String)
      @ Base ./error.jl:35
    [2] _test_inferred(f::Any, args::Any; kwargs::Base.Pairs{Symbol, V, Tuple{Vararg{Symbol, N}}, NamedTuple{names, T}} where {V, N, names, T<:Tuple{Vararg{Any, N}}})
      @ ChainRulesTestUtils ~/.julia/dev/ChainRulesTestUtils/src/testers.jl:265
    [3] _test_inferred
      @ ~/.julia/dev/ChainRulesTestUtils/src/testers.jl:263 [inlined]
    [4] macro expansion
      @ ~/.julia/dev/ChainRulesTestUtils/src/testers.jl:220 [inlined]
    [5] macro expansion
      @ /opt/julia-1.9.0/share/julia/stdlib/v1.9/Test/src/Test.jl:1498 [inlined]
    [6] test_rrule(::ChainRulesCore.RuleConfig, ::Any, ::Any, ::Vararg{Any}; output_tangent::Any, check_thunked_output_tangent::Any, fdm::Any, rrule_f::Any, check_inferred::Bool, fkwargs::NamedTuple, 
rtol::Real, atol::Real, testset_name::Any, kwargs::Base.Pairs{Symbol, V, Tuple{Vararg{Symbol, N}}, NamedTuple{names, T}} where {V, N, names, T<:Tuple{Vararg{Any, N}}})                                
      @ ChainRulesTestUtils ~/.julia/dev/ChainRulesTestUtils/src/testers.jl:205
    [7] test_rrule(::ChainRulesCore.RuleConfig, ::Any, ::Any, ::Vararg{Any})
      @ ChainRulesTestUtils ~/.julia/dev/ChainRulesTestUtils/src/testers.jl:181
    [8] test_rrule(::Any, ::Vararg{Any}; kwargs::Base.Pairs{Symbol, V, Tuple{Vararg{Symbol, N}}, NamedTuple{names, T}} where {V, N, names, T<:Tuple{Vararg{Any, N}}})
      @ ChainRulesTestUtils ~/.julia/dev/ChainRulesTestUtils/src/testers.jl:178
    [9] test_rrule(::Any, ::Any, ::Any)
      @ ChainRulesTestUtils ~/.julia/dev/ChainRulesTestUtils/src/testers.jl:177
   [10] top-level scope
      @ REPL[5]:1

@oxinabox
Copy link
Member

yes, that is unrelated

@oxinabox oxinabox merged commit 64971e7 into JuliaDiff:main Dec 15, 2023
15 of 18 checks passed
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.

2 participants