Conversation
|
I still think we should go with #866. But it seems not everyone agrees. |
Yeah, that’s exactly why I thought it would be good to add tests in this way. My hope is that this is not controversial, and the benefit is that things don’t get worse |
|
I have no idea what’s going on with the tests though |
| # for method_ambiguity in ambs | ||
| # @show method_ambiguity | ||
| # end | ||
| @test length(ambs) ≤ 5 |
There was a problem hiding this comment.
Is there a simple way to check which functions are ambiguous?
There was a problem hiding this comment.
We can use Aqua.test_ambiguities(StatsBase; recursive = true, exclude = [StatsBase.TestStat, Base.:(==)]). However, this doesn't test the number of ambiguities, only the functions.. Testing the number of ambiguities is nice, IMO, because we can even write this as:
@test length(ambs) ≤ 5
@test_broken length(ambs) < 5so that we're alerted when it's fixed. It's also just easier to spread across the ecosystem than to collect all the functions after printing them all out..
There was a problem hiding this comment.
I'm happy to change it if it means getting the PR merged.
| end | ||
|
|
||
| @testset "Aqua tests (additional)" begin | ||
| Aqua.test_undefined_exports(StatsBase) |
There was a problem hiding this comment.
Just run Aqua.test_all and disable the ones you're handling separately? Then we won't miss out on new tests potentially added in the future.
There was a problem hiding this comment.
While I do like the idea of not missing out on new tests in the future, my issue with test_all is that, if something does fail, then the simplest fix is often to call the individual test functions.
| Aqua.test_stale_deps(StatsBase) | ||
| Aqua.test_deps_compat(StatsBase) | ||
| Aqua.test_project_extras(StatsBase) | ||
| Aqua.test_project_toml_formatting(StatsBase) |
There was a problem hiding this comment.
There's a bug currently and hence this should not be tested generally (see JuliaTesting/Aqua.jl#105). Hence I tend to use
| Aqua.test_project_toml_formatting(StatsBase) | |
| Aqua.test_all( | |
| StatsBase; | |
| project_toml_formatting=VERSION >= v"1.7" || !haskey(ENV, "GITHUB_ACTIONS"), | |
| ) | |
| Aqua.test_project_toml_formatting(StatsBase; ) |
There was a problem hiding this comment.
Alternatively, I'm fine with commenting this out.
|
|
||
| # See: https://github.com/SciML/OrdinaryDiffEq.jl/issues/1750 | ||
| # Test that we're not introducing method ambiguities across deps | ||
| ambs = Aqua.detect_ambiguities(StatsBase; recursive = true) |
There was a problem hiding this comment.
Why do we want to test ambiguities recursively? It's generally argued against that in JuliaTesting/Aqua.jl#77 (and against the default setting of testing Base and Core as well in the Aqua test suite).
There was a problem hiding this comment.
I figured that it was more thorough.. I'm happy to change this to ambs = Aqua.detect_ambiguities(StatsBase)
| # This tests that we don't accidentally run into | ||
| # https://github.com/JuliaLang/julia/issues/29393 | ||
| # Aqua.test_unbound_args(StatsBase) | ||
| ua = Aqua.detect_unbound_args_recursively(StatsBase) | ||
| @test length(ua) == 0 |
There was a problem hiding this comment.
Can we just run the full Aqua test suite (see also my other comment below)? It's already included there by default: https://github.com/JuliaTesting/Aqua.jl/blob/3d5ed9fa2c915bbb06b2ef9037d57029d18bc8b5/src/unbound_args.jl#L36
There was a problem hiding this comment.
Yeah, I guess we can change it to the test function. I have my own gripe with that interface, tbh. JuliaTesting/Aqua.jl#180
|
This doesn't seem to have any traction. Closing. |
3 participants
This PR adds and fixes more Aqua tests. I've put in some limits for the aqua tests, to prevent issues from worsening.