NNPDF / pineappl

PineAPPL is not an extension of APPLgrid
https://nnpdf.github.io/pineappl/
GNU General Public License v3.0
12 stars 3 forks source link

Include doctests in code coverage #201

Closed cschwan closed 1 year ago

cschwan commented 1 year ago

After merging https://github.com/NNPDF/pineappl/pull/187 doctests are no longer included in the code coverage. They should added back.

cschwan commented 1 year ago

See https://doc.rust-lang.org/rustc/instrument-coverage.html#including-doc-tests, which however is probably outdated.

cschwan commented 1 year ago

Contrary to what I said, adding the line

export RUSTDOCFLAGS='-C instrument-coverage -Z unstable-options --persist-doctests ../target/debug/doctestbins'

successfully generates the doctest binaries (the ../ is needed because cargo test seems to enter each subdirectory) and replacing the call to sed with

( sed -nE 's/[[:space:]]+Running( unittests|) [^[:space:]]+ \(([^)]+)\)/\2/p' stderr && echo target/debug/doctestbins/*/rust_out | tr ' ' "\n" )

adds the new binaries containing the doctests.

However, when generating coverage it generates the following output:

error: /scratch/cschwan/pineappl/pineappl/pineappl/src/lumi.rs: No such file or directory
warning: The file '/scratch/cschwan/pineappl/pineappl/pineappl/src/lumi.rs' isn't covered.

(there should be only two pineappl directories instead of three).

cschwan commented 1 year ago

Additionally passing -Z doctest-in-workspace to cargo test and removing ../ in ../target/debug/doctestbins fixes the problem.

To use this, we need an updated container with nightly Rust: the options -Z fail unless run with an nightly toolchain.

alecandido commented 1 year ago

Since the edition is the same, and you are determining anyhow the MSRV automatically, it should not be a problem to move the container to just use nightly.

cschwan commented 1 year ago

The MSRV isn't determined automatically, I have to explicitly write it into Cargo.toml and then check it whether I wasn't 'lying'. I' going to add an explicit check for the MSRV in a new workflow, and we're going to use a relatively new nightly version everywhere else in an updated container.

alecandido commented 1 year ago

Isn't this an automated check?

https://github.com/NNPDF/pineappl/blob/2d1f97bd73c0554a538dee80fb528a310d8bf937/maintainer/make-release.sh#L52

cschwan commented 1 year ago

Right, but we want to check it for every commit ideally.

cschwan commented 1 year ago

Commit fa4efb1c59ab73dd32037c59ad6f3a0164d647aa implements also code coverage for doctests, and commits c04887ab74297a272b22ea80a3965b7be491d4e7 explicitly checks the MSRV.