Add Aqua tests #662
Add Aqua tests #662
Conversation
charleskawczynski
force-pushed
the
ck/aqua
branch
from
bbfb06c to
1a99035
Compare
charleskawczynski
force-pushed
the
ck/aqua
branch
from
1a99035 to
4fae6eb
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #662 +/- ##
==========================================
- Coverage 89.57% 86.75% -2.83%
==========================================
Files 11 10 -1
Lines 969 921 -48
==========================================
- Hits 868 799 -69
- Misses 101 122 +21 ☔ View full report in Codecov by Sentry. |
|
I'd appreciate feedback on what to do with the tests, here. From what I can see, it seems like CI is broken on master |
| @testset "Aqua tests - unbound_args" begin | ||
| # This tests that we don't accidentally run into | ||
| # https://github.com/JuliaLang/julia/issues/29393 | ||
| ua = Aqua.detect_unbound_args_recursively(ForwardDiff) | ||
| @test length(ua) == 6 | ||
| end |
There was a problem hiding this comment.
Ideally the unbound arguments should be fixed, which would also allow us to just run the default Aqua.test_all test with unbound_args = true below instead of this custom test.
There was a problem hiding this comment.
These ambiguities seem to be upstream issues. See JuliaTesting/Aqua.jl#86 and JuliaLang/julia#28086.
We can "fix" this (I'm not 100% sure if the issue is here or upstream, even based on discussions) by changing e.g.,
@generated function one_tuple(::Type{NTuple{N,V}}) where {N, V}to
@generated function one_tuple(::Type{Tuple{V,Vararg{R1,N}}) where {N, V}Should I move forward with those changes?
test/AquaTest.jl
Outdated
| pkg_match(pkgname, pkdir::AbstractString) = occursin(pkgname, pkdir) | ||
| filter!(x -> pkg_match("ForwardDiff", pkgdir(last(x).module)), ambs) | ||
|
|
||
| @test length(ambs) == 0 |
There was a problem hiding this comment.
There was a problem hiding this comment.
I wasn't able to reproduce this locally for 1.10.
One of the ambiguities that I'm seeing on 1.6, @inline Base.convert(::Type{Dual{T,V,N}}, x) where {T,V,N} = Dual{T}(V(x), zero(Partials{N,V})), can be eliminated, but we would need to depend on ChainRulesCore, and define @inline Base.convert(::Type{Dual{T,V,N}}, x::ChainRulesCore.AbstractZero) where {T,V,N} = Dual{T}(V(x), zero(Partials{N,V})) in order to do so.
I'm not sure that adding the dependency is worth it? I suppose we could add it as an extension, but I'd rather address that in a followup PR.
There was a problem hiding this comment.
Just run the Aqua tests first, before StaticArrays etc are loaded (I think StaticArrays or some other test dependency might pull in ChainRulesCore?). If only ForwardDiff, Aqua, and Test are loaded such indirect issues should not show up.
There was a problem hiding this comment.
Done, let's see what happens. Hmm, I'm getting the same thing locally (6 ambiguities).
|
bump |
1 similar comment
|
bump |
This PR adds Aqua tests.