Make doctests part of Pkg.test("Oscar")

[thofma]

This allows for the downstream testing in our upstream packages GAP, Hecke, Polymake, etc., to also run the doctests by default (the downstream tests are run by default on julia version 1.6, so we are good in this regard).

A minor disadvantage is that the default julia version for https://github.com/oscar-system/OscarDevTools.jl and the Oscar
doctest julia version need to be kept in lockstep.

(In an ideal world, we would make the doctests julia version agnostic and then these remarks about versions would all be unnecessary.)

CC: @fingolfin @benlorenz

[benlorenz]

We do run the Oscar doctests already for the Singular and Polymake OscarCI tests, they failed e.g. here.

[thofma]

Ah, that would also be an option. At the moment all those config files need to be updated across all packages once we change the Oscar doctest julia version?

Does speak something against doing it directly in Pkg.test()? Should also make it faster since then the doctests are run in the same julia session.

[fingolfin]

I see no obvious downsides (we also do this for GAP.jl). Perhaps @benlorenz can think of some?

On the upside, it makes ]test Oscar more comprehensive.

[fingolfin]

Tests fail due to some actual (minor) issues. I've pushed a fix (hopefully)

[benlorenz]

I think the main reason for this should not be the OscarCI downstream tests as we already have code for that in place which is working fine (and I think also reasonably easy to add to any repository).
I prefer to have different test-steps separate in the CI to make it easier to spot where a failure occurs, and also to make it easier to spot that the tests are running correctly. With the current setup I can directly see when the doctests are running for a job and when they are skipped. With this PR I would need to open the job log for the tests block, scroll to somewhere in the middle (after the installation / precompilation but before the other tests) and look for some @info block.

At least, I think we should swap the tests and run the doctests after the proper testsuite, which (at least in theory) should contain the more specific and more reliable tests that make it easier to pinpoint any failure. Parsing failures in the doctests is in my opinion way more annoying than parsing normal test failures.

PS: Just recently I did notice that the Run tests-step was running in about 10 minutes instead of 35 minutes because of many accidentally disabled tests. Including the doctests in that step would have made it less likely to be spotted that way, I could have blamed it on some of the usual fluctuations in the github actions performance instead.

[benlorenz]

Ah, that would also be an option. At the moment all those config files need to be updated across all packages once we change the Oscar doctest julia version?

Currently yes, but we could probably put that version number in an environment variable instead which is set by OscarDevTools.jl, then we can just write it once in that repo (or we might even parse it from some file in the oscar repository).

[thofma]

I have no strong opinion. I am also happy to just amend the OscarCI.yml in the other packages and leave everything as is.

It is just a bit odd in my opinion, that for the purpose of coverage we count also the doctest(Oscar) run, but we do not consider it part of the official tests. I believe it would simplify the development, since no one would need to remeber Oscar.doc_build(;doctest = true) for running the doctests (in yet another julia session).

[benlorenz]

I would be fine with running the doctests within the normal tests (but please move them after the other tests), and using something like && !haskey(ENV,"oscar_run_doctests") (or some other variable name) to skip them in the CI where we want to run them separately.

Also, I think it would be preferable to cover all code with normal tests as this makes sure that it runs correctly with all julia versions.

[thofma]

Agreed. I will make the changes.

[thofma]

Thanks for your feedback. I have moved the doctests to the end.

[benlorenz]

Thanks for your feedback. I have moved the doctests to the end.

Thanks! Fixes for the doctests are (almost) ready to merge in #1769 (it will merge into this PR here).