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

Generalize doctest() API #1054

Merged
merged 2 commits into from
Jul 7, 2019
Merged

Generalize doctest() API #1054

merged 2 commits into from
Jul 7, 2019

Conversation

mortenpi
Copy link
Member

@mortenpi mortenpi commented Jul 2, 2019

Updates the API. You can now simply call doctest(::Module), and it will doctest both docstrings and manual pages, e.g.:

using Test, Documenter, MyPackage
@testset "MyPackage" begin
    doctest(MyPackage)
end

For more control, one can also use the doctest(source, modules) method, which does the heavy lifting (e.g. to test a set of modules, but no pages, you can call doctest(nothing, [Module1, Module2, ...])).

Finally, it also adds the fix keyword to the methods, to run the doctest fixing machinery. This actually means you can fix doctests pretty easily in the REPL now:

julia> using Documenter, MyPkg
julia> doctest(MyPkg, fix=true)

cc @davidanthoff You don't happen to have any suggestions for packages where we could test this in the wild?

Should also fix the dev/ docs for Documenter, since we're now using the doctest function in runtests.jl.

Close #1051, close #1052

@davidanthoff
Copy link
Contributor

I'll create a branch in Query.jl to test this!

@davidanthoff
Copy link
Contributor

I tried it with queryverse/Query.jl#269, and it all seems to work. The thing won't work for the CI builds because they don't pick up this branch here of Documenter, but locally it all looked great.

One thing I particularly like is that this does not pick up anything from the docs/Project.toml or docs/Manifest.toml, but instead it all uses the env that is specified for the tests in the main package Project.toml. I think that is exactly right!

@fredrikekre
Copy link
Member

I think that is exactly right!

Why?

@davidanthoff
Copy link
Contributor

I'm not checking in a docs/Manifest.toml, so I generally feel that shouldn't play a role.

I think one important aspect is that if my active env is say shared foo, and I have deved my package in that env (and potentially other packages as well), then I would want the doc test run to pick up any other packages that I've deved in foo for that test run, in the same way that a test run picks up specific versions of other packages that I have in foo. I'm actually not sure now whether that is happening with the current PR here for the doctests, but generally that is what I'd expect, and that seems somewhat easier if the doctests run in the same env as the general tests? Not sure.

@fredrikekre
Copy link
Member

So you would have to add all doc dependendencies as test dependencies then?

@davidanthoff
Copy link
Contributor

Yes, that is what I did for the Query.jl branch. I could see this all being more elegant with some of the sub-project stuff that is coming? But for now this seems fine.

@davidanthoff
Copy link
Contributor

One weird thing I ran across: when I ran the doctests as part of runtests.jl locally, I had to change all the doctests so that the output looked like 1×2 DataFrame instead of the 1×2 DataFrames.DataFrame. I then opened a PR against Query.jl to change that in all doctests (https://github.com/queryverse/Query.jl/pull/268/files), but in the CI run these now all failed.

Not sure what is going on, and I never understood when I have to add the module name in the output, but it looks a little as if one needs to do this differently when the doctests are run from runtests.jl?

@fredrikekre
Copy link
Member

fredrikekre commented Jul 2, 2019

Thats because DataFrames was sometimes loaded in Main, see e.g. JuliaLang/julia#29466, you should be able to work around it by loading that into Main unconditionally.

@davidanthoff
Copy link
Contributor

Thats because DataFrames was sometimes loaded in Main, see e.g. JuliaLang/julia#29466, you should be able to work around it by loading that into Main unconditionally.

Cool, thanks for pointing me to that, I always wanted to know what is going on there :)

So the story then is that in runtests.jl I have a using DataFrames, but not in docs/make.jl, and that leads to different output of the doctests depending on how you run them.

The short term fix is simple: I'll just add using DataFrames to docs/make.jl. As a general solution not great, but I guess this really needs to be fixed in base...

@mortenpi mortenpi mentioned this pull request Jul 3, 2019
mortenpi added a commit that referenced this pull request Jul 3, 2019
(cherry picked from commit 1e9c615)
@tkf
Copy link
Contributor

tkf commented Jul 4, 2019

using Test, Documenter, MyPackage
@testset "Doctesting" begin
    @test doctest(MyPackage)
end

Can it be just doctest(MyPackage) instead of @testset "Doctesting"; @test doctest(MyPackage); end? It would be nice to get N failures for N failing doctests, rather than just a binary output.

(PS: It's great to see that doctests are decoupled from documentation!)

@mortenpi
Copy link
Member Author

mortenpi commented Jul 4, 2019

Can it be just doctest(MyPackage) instead of @testset "Doctesting"; @test doctest(MyPackage); end? It would be nice to get N failures for N failing doctests, rather than just a binary output.

That's a good idea, but I'll leave the implementation for another time, since it's a bit of work to make it happen (#1053).

@tkf
Copy link
Contributor

tkf commented Jul 4, 2019

How about defining it as doctest(m::Module) = @test doctest_impl(m) for now? This way, users don't have to change anything at the call sites if you make test results more detailed at some point.

@mortenpi
Copy link
Member Author

mortenpi commented Jul 5, 2019

This way, users don't have to change anything at the call sites if you make test results more detailed at some point.

That's a fair point. I am inclined to keep the doctest function as is though, so that there would a simple function you can call that just gives you true/false (and allows fixing the doctests).

What about @doctestset MyPackage? Would then currently expand into:

@testset "Doctests: MyPackage" begin
    @test doctest(MyPackage)
end

@tkf
Copy link
Contributor

tkf commented Jul 5, 2019

a simple function you can call that just gives you true/false

My honest impression is that it's not clear what the target user of doctest(::Module) :: Bool is. It's not handy for end-users because they need to use @testset and @test. It's not really an API suitable for testing utility packages because the output is too coarse-grained. But maybe this is not going to be a problem in practice. Error reporting from makedocs has been already quite useful.

FYI, I created a test helper tool https://github.com/tkf/Aqua.jl#quick-usage (though not related to doctest at all). I use the API like doctest(m::Module) = @test doctest_impl(m) I suggested above. I've never needed true/false output and I'm enjoying the simplicity of the test setup. Of course, this could be just due to my workflow.

@tkf
Copy link
Contributor

tkf commented Jul 5, 2019

What about @doctestset MyPackage?

I think it makes sense if you decide to go with doctest(::Module) :: Bool.

@mortenpi
Copy link
Member Author

mortenpi commented Jul 5, 2019

@tkf You won me over. The -> Bool API would not actually be particularly useful indeed. The only other use I can see for the doctest function, outside of runtests.jl, is in the REPL and there is no problem if it also returns a testset there (in fact, it might be preferred).

@tkf
Copy link
Contributor

tkf commented Jul 5, 2019

Great. Thank you for being open minded!

doctest() can now also doctest manual pages, which makes it easier to
include doctesting as part of the test suite, and behaves like a
testset.
@mortenpi mortenpi merged commit 1d0a383 into master Jul 7, 2019
@mortenpi mortenpi deleted the mp/fix-docs branch July 7, 2019 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix doctesting docs Doctest manual pages too with doctest?
4 participants