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.

Sign up for GitHub

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

Add Aqua tests #662

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Add Aqua tests #662

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5639465
Add Aqua tests
charleskawczynski Sep 5, 2023
4fae6eb
Update
charleskawczynski Jan 4, 2024
7a4fe94
Wrap aqua tests in module
charleskawczynski Jan 4, 2024
75416f2
Add aqua badge to readme
charleskawczynski Jan 4, 2024
7d7dc43
Fix tests
charleskawczynski Jan 5, 2024
9d24e2e
Reorder tests
charleskawczynski Jan 5, 2024
15bdd61
Skip amb test for all other julia versions
charleskawczynski Jan 5, 2024
be2f879
Merge branch 'master' into ck/aqua
charleskawczynski Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,27 @@ StaticArrays = "90137ffa-7385-5640-81b9-e52037218182"
ForwardDiffStaticArraysExt = "StaticArrays"

[compat]
Aqua = "0.8"
Calculus = "0.5"
CommonSubexpressions = "0.3"
DiffResults = "1.1"
DiffRules = "1.4"
DiffTests = "0.1"
InteractiveUtils = "1"
LinearAlgebra = "1"
LogExpFunctions = "0.3"
NaNMath = "1"
Preferences = "1"
Printf = "1"
Random = "1"
SparseArrays = "1"
SpecialFunctions = "1, 2"
StaticArrays = "1.5"
Test = "1"
julia = "1.6"

[extras]
Aqua = "4c88cf16-eb10-579e-8560-4a9242c79595"
Calculus = "49dc2e85-a5d0-5ad3-a950-438e2897f1b9"
DiffTests = "de460e47-3fe3-5279-bb4a-814414816d5d"
InteractiveUtils = "b77e0a4c-d291-57a0-90e8-8db25a27a240"
Expand All @@ -43,4 +51,4 @@ StaticArrays = "90137ffa-7385-5640-81b9-e52037218182"
Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"

[targets]
test = ["Calculus", "DiffTests", "SparseArrays", "StaticArrays", "Test", "InteractiveUtils"]
test = ["Aqua", "Calculus", "DiffTests", "SparseArrays", "StaticArrays", "Test", "InteractiveUtils"]
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

[![](https://img.shields.io/badge/docs-stable-blue.svg)](https://juliadiff.org/ForwardDiff.jl/stable)
[![](https://img.shields.io/badge/docs-dev-blue.svg)](https://juliadiff.org/ForwardDiff.jl/dev)
[![Aqua QA](https://raw.githubusercontent.com/JuliaTesting/Aqua.jl/master/badge.svg)](https://github.com/JuliaTesting/Aqua.jl)

# ForwardDiff.jl

Expand Down
29 changes: 29 additions & 0 deletions test/AquaTest.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
module AquaTest

using Test
using ForwardDiff
using Aqua

@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
Comment on lines +7 to +12
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?


@testset "Aqua tests - ambiguities" begin
# See: https://github.com/SciML/OrdinaryDiffEq.jl/issues/1750
# Test that we're not introducing method ambiguities across deps
ambs = Aqua.detect_ambiguities(ForwardDiff; recursive = true)
pkg_match(pkgname, pkdir::Nothing) = false
pkg_match(pkgname, pkdir::AbstractString) = occursin(pkgname, pkdir)
filter!(x -> pkg_match("ForwardDiff", pkgdir(last(x).module)), ambs)

@test length(ambs) == 0
Copy link
Member

Choose a reason for hiding this comment

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

Seems this test fails: https://github.com/JuliaDiff/ForwardDiff.jl/actions/runs/7412737730/job/20170158384?pr=662#step:6:1288

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, let's see what happens. Hmm, I'm getting the same thing locally (6 ambiguities).

end

@testset "Aqua tests - remaining" begin
Aqua.test_all(ForwardDiff; ambiguities = false, unbound_args = false)
end

end
5 changes: 5 additions & 0 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,5 +51,10 @@ Random.seed!(SEED)
t = @elapsed include("AllocationsTest.jl")
println("##### done (took $t seconds).")
end
@testset "Aqua" begin
println("##### Testing aqua...")
t = @elapsed include("AquaTest.jl")
println("##### done (took $t seconds).")
end
println("##### Running all ForwardDiff tests took $(time() - t0) seconds.")
end
Loading