JuliaTesting / Aqua.jl

Auto QUality Assurance for Julia packages
MIT License
339 stars 25 forks source link

Bug: Discrepancies Between `test_all()` and `test_ambiguities()` #258

Closed kmariuszk closed 9 months ago

kmariuszk commented 9 months ago

I have been trying to add the Aqua package to CounterfactualExplanations.jl repository. I have run into the following problem:

Initially, I started with the regular Aqua.test_all(CounterfactualExplanations but it threw almost 90 ambiguities, and basically none of them was from the package itself. For example:

Ambiguity #1
<(a::Integer, b::SentinelArrays.ChainedVectorIndex) @ SentinelArrays ~/.julia/packages/SentinelArrays/1kRo4/src/chainedvector.jl:208
<(x::BigInt, i::Integer) @ Base.GMP gmp.jl:711

is from the SentinelArrays package.

I noticed there's no such problem when running Aqua.test_ambiguities([CounterfactualExplanations]; recursive=false, broken=false). Then, there are only two relevant ambiguities detected.

After quick digging in the codebase, I noticed the test set for ambiguity in the test_all() method includes some additional modules in the function call, i.e.:

@testset "Method ambiguity" begin
        if ambiguities !== false
            test_ambiguities([testtarget, Base, Core]; askwargs(ambiguities)...)
        end
    end

I suppose that's what causes the discrepancies.

I would be very thankful if someone who knows the package better could pivot me in the right direction.

lgoettgens commented 9 months ago

Duplicate of https://github.com/JuliaTesting/Aqua.jl/issues/77

The issue with Aqua.test_ambiguities([CounterfactualExplanations]; recursive=false, broken=false) is that it only finds ambiguities between functions of your package. Ambiguities that you introduce to a Base function may be skipped. Furthermore, there are some problems with ambiguities in constructors.

Once I find some time for https://github.com/JuliaTesting/Aqua.jl/issues/180, you would get better possibilities to use the ambiguity test. For the time being, I recommend to either use your workaround (and miss some ambiguities) or skip the test altogether.