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.

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 tests for undocumented symbols #52723

Merged
merged 31 commits into from Jan 14, 2024
Merged

add tests for undocumented symbols #52723

merged 31 commits into from Jan 14, 2024

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Jan 3, 2024

Following #52413 by @jariji and the discussion in #51174, this adds tests to ensure that there are no undocumented (= lacking docstrings) public symbols in any documented module.

Many of the tests are broken — we have a lot of undocumented exports. In such cases I added both a @test_broken and a @test to ensure that there are no additional regressions added in the future.

(This PR may have to be updated when #52413 (comment) is fixed, i.e. when #52727 #52743 is merged.) Has been updated.

@stevengj stevengj added the domain:docs This change adds or pertains to documentation label Jan 3, 2024
@jariji
Copy link
Contributor

jariji commented Jan 3, 2024

May I suggest a more gradual approach:

@test length(undocumented_names(M)) <= 10

which can be incrementally decremented as people add docstrings. That way we can still test modules that aren't fully documented yet.

@DilumAluthge
Copy link
Member

In the context of this PR, what does "undocumented symbol" mean? Does it mean:

  1. The symbol does not have a docstring.
  2. The symbol does not appear in the Julia manual.

@jariji
Copy link
Contributor

jariji commented Jan 3, 2024

@DilumAluthge see #52723 (comment)

@stevengj
Copy link
Member Author

stevengj commented Jan 3, 2024

That way we can still test modules that aren't fully documented yet.

The current PR does support such modules. The way it works is:

undoc = Docs.undocumented_names(SomeModule)
@test_broken undoc == []
@test undoc = [:bar, :baz]

where the second @test ensures that there are no further regressions.

(And we prefer @test undoc == [...] rather than test length(undoc) tests here, because then test failures will print the list of undocumented symbols.)

@stevengj
Copy link
Member Author

stevengj commented Jan 3, 2024

@DilumAluthge, an undocumented symbol is a public/exported symbol lacking a docstring, ala #52413, regardless of whether it appears in the manual (which we cannot easily test at the moment).

@jariji
Copy link
Contributor

jariji commented Jan 3, 2024

The current PR does support such modules.

Oops, you're right.

@stevengj stevengj added the test This change adds or pertains to unit tests label Jan 3, 2024
@stevengj stevengj mentioned this pull request Jan 4, 2024
@LilithHafner
Copy link
Member

Do we want a submodules function to make sure we don't miss anything?

"""
    submodules(mod::Module; private::Bool=false) -> Vector{Module}

Return a sorted vector of public submodules of `mod`, including recursive submodules.

If `private` is `true`, also return non-public submodules.
"""
submodules(mod::Module; private::Bool=false) = 
    sort!(collect(_submodules!(Set((mod,)), mod, private)), by=string)

function _submodules!(mods, mod, private)
    for sym in names(mod, all=true)
        if (private || Base.ispublic(mod, sym)) && isdefined(mod, sym)
            val = getproperty(mod, sym)
            if val isa Module && val  mods
                push!(mods, val)
                _submodules!(mods, val, private)
            end
        end
    end
    mods
end

@jariji
Copy link
Contributor

jariji commented Jan 4, 2024

submodules could be useful but probably belongs in its own issue because it's really only tangential here.

test/docs.jl Outdated Show resolved Hide resolved
test/ordering.jl Outdated Show resolved Hide resolved
@stevengj
Copy link
Member Author

stevengj commented Jan 4, 2024

@LilithHafner, I ran your submodules function manually on Base and added tests for a few more submodules that were marked public (Broadcast, Checked, Order, Sort).

It wouldn't be as easy to run automatically because of the large number of missing docstrings, however. Once #52725 is closed we might reconsider.

test/misc.jl Show resolved Hide resolved
@stevengj
Copy link
Member Author

CI failure on x86_64-linux-gnuassertrr seems to be an unrelated glitch in `testset Distributed:

Distributed                                       (1) |   767.79 |   0.00 |  0.0 |      31.94 |   519.91
gc                                                (1) |        started at 2024-01-10T16:01:10.812 on pid 538
julia: /cache/build/tester-amdci4-12/julialang/julia-master/src/gf.c:1458: jl_mt_assoc_by_type: Assertion `tt->isdispatchtuple || tt->hasfreetypevars' failed.

Should be okay to merge?

@stevengj stevengj added the status:awaiting review PR is complete and seems ready to merge. Has tests and news/compat if needed. CI failures unrelated. label Jan 10, 2024
@vtjnash
Copy link
Sponsor Member

vtjnash commented Jan 11, 2024

I haven't seen that one before. Could you open an issue for it and add the rr-trace-included tag. It should be pretty quick for someone to solve with that info

@stevengj
Copy link
Member Author

Filed an issue.

@stevengj
Copy link
Member Author

Good to merge?

@jariji
Copy link
Contributor

jariji commented Jan 11, 2024

LGTM

@LilithHafner LilithHafner removed the status:awaiting review PR is complete and seems ready to merge. Has tests and news/compat if needed. CI failures unrelated. label Jan 14, 2024
@LilithHafner LilithHafner added the status:merge me PR is reviewed. Merge when all tests are passing label Jan 14, 2024
@LilithHafner
Copy link
Member

Merging base in before merging to catch semantic merge conflicts with folks adding docstrings to public symbols

@stevengj
Copy link
Member Author

Unrelated CI error on test x86_64-w64-mingw32:

Error in testset Artifacts:
Test Failed at C:\buildkite-agent\builds\win2k22-amdci6-4\julialang\julia-master\julia-f286d44476\share\julia\stdlib\v1.11\Artifacts\test\runtests.jl:73
  Expression: load_overrides() == empty_output
   Evaluated: Dict{Symbol, Any}(:UUID => Dict{Base.UUID, Dict{String, Union{Base.SHA1, String}}}(Base.UUID("7b879065-7f74-5fa4-bdd5-9b7a15df8941") => Dict("arty" => SHA1("64d0b4f8d9c004b862b38c4acfbd74988226995c"), "barty" => "C:\\Users\\julia\\AppData\\Local\\Temp\\jl_m2pH8c\\a_wild_barty_appears")), :hash => Dict{Base.SHA1, Union{Base.SHA1, String}}(SHA1("087d8c93bff2f2b05f016bcd6ec653c8def76568") => SHA1("64d0b4f8d9c004b862b38c4acfbd74988226995c"))) == Dict{Symbol, Any}(:UUID => Dict{Base.UUID, Dict{String, Union{Base.SHA1, String}}}(), :hash => Dict{Base.SHA1, Union{Base.SHA1, String}}())
ERROR: LoadError: Test run finished with errors
in expression starting at C:\buildkite-agent\builds\win2k22-amdci6-4\julialang\julia-master\julia-f286d44476\share\julia\test\runtests.jl:97
┌ Warning: Failed to clean up temporary path "C:\\Users\\julia\\AppData\\Local\\Temp\\jl_D6gQU0"

Should be good to merge.

@stevengj stevengj merged commit 25c4166 into master Jan 14, 2024
6 of 8 checks passed
@stevengj stevengj deleted the test_undoc branch January 14, 2024 23:00
@LilithHafner LilithHafner removed the status:merge me PR is reviewed. Merge when all tests are passing label Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:docs This change adds or pertains to documentation test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants