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

Jump to bottom

Add more aqua tests #892

Closed
wants to merge 1 commit into from
Closed

Add more aqua tests #892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ StatsAPI = "1.2"
julia = "1"

[extras]
Aqua = "4c88cf16-eb10-579e-8560-4a9242c79595"
BenchmarkTools = "6e4b80f9-dd63-53aa-95a3-0cdb28fa8baf"
Dates = "ade2ca70-3891-5945-98fb-dc099432e06a"
DelimitedFiles = "8bb1440f-4735-579b-a4ab-409b98df4dab"
Expand All @@ -34,4 +35,4 @@ StableRNGs = "860ef19b-820b-49d6-a774-d7a799459cd3"
Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"

[targets]
test = ["BenchmarkTools", "Dates", "DelimitedFiles", "OffsetArrays", "StableRNGs", "Test"]
test = ["Aqua", "BenchmarkTools", "Dates", "DelimitedFiles", "OffsetArrays", "StableRNGs", "Test"]
2 changes: 1 addition & 1 deletion docs/Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[deps]
Documenter = "e30172f5-a6a5-5a46-863b-614d45cd2de4"
StatsBase = "2913bbd2-ae8a-5f71-8c99-4fb6c76f3a91"
StatsAPI = "82ae8749-77ed-4fe6-ae5f-f523153014b0"
StatsBase = "2913bbd2-ae8a-5f71-8c99-4fb6c76f3a91"

[compat]
Documenter = "0.27"
8 changes: 0 additions & 8 deletions test/ambiguous.jl
View file
Open in desktop

This file was deleted.

36 changes: 36 additions & 0 deletions test/aqua.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
using Test
using StatsBase
using Aqua

@testset "Aqua tests (performance)" begin
# 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
Comment on lines +6 to +10
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I guess we can change it to the test function. I have my own gripe with that interface, tbh. JuliaTesting/Aqua.jl#180


# 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)
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Author

Choose a reason for hiding this comment

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

I figured that it was more thorough.. I'm happy to change this to ambs = Aqua.detect_ambiguities(StatsBase)

# Uncomment for debugging:
# for method_ambiguity in ambs
# @show method_ambiguity
# end
@test length(ambs) ≤ 5
Copy link
Member

Choose a reason for hiding this comment

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

Is there a simple way to check which functions are ambiguous?

Copy link
Author

@charleskawczynski charleskawczynski Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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) < 5

so 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..

Copy link
Author

Choose a reason for hiding this comment

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

I'm happy to change it if it means getting the PR merged.

potentially_pirated_methods = Aqua.Piracy.hunt(StatsBase)
# Uncomment for debugging:
# for pirate in potentially_pirated_methods
# @show pirate
# end
@test length(potentially_pirated_methods) ≤ 27
end

@testset "Aqua tests (additional)" begin
Aqua.test_undefined_exports(StatsBase)
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 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.

Copy link
Author

Choose a reason for hiding this comment

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

There's a bug currently and hence this should not be tested generally (see JuliaTesting/Aqua.jl#105). Hence I tend to use

Suggested change
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; )

Copy link
Author

Choose a reason for hiding this comment

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

Alternatively, I'm fine with commenting this out.

end

nothing
2 changes: 1 addition & 1 deletion test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ using Dates
using LinearAlgebra
using Random
charleskawczynski marked this conversation as resolved.
Show resolved Hide resolved

tests = ["ambiguous",
tests = ["aqua",
"weights",
"moments",
"scalarstats",
Expand Down
Loading