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

use bisect to test coverage of the tcpip tests #160

Closed
avsm opened this issue Jul 6, 2015 · 4 comments · Fixed by #436
Closed

use bisect to test coverage of the tcpip tests #160

avsm opened this issue Jul 6, 2015 · 4 comments · Fixed by #436

Comments

@avsm
Copy link
Member

avsm commented Jul 6, 2015

instructions from @djs55 on the mailing list below:


I've been having a lot of fun with bisect[1] recently-- I've been using it to see which parts of the mirage-block-volume[2] and shared-block-ring[3] libraries are completely untouched by their unit tests. I found the following game to be quite addictive:

  1. "make coverage", and load result in web-browser
  2. spot a big chunk of obvious red (danger, danger, danger)
  3. (thinking carefully about what could go wrong) devise an interesting test to stress the red bits (obviously you could cover it with a 'noddy' test but there is probably no point)
  4. "make coverage", reload in browser and see the red go green!

I've hooked it up with coveralls.io (admittedly in a bit of a hacky way[4]) such that the master branch is firmly in "development" mode, linking against bisect, and without checking in the oasis autogen rubbish. The .travis.yml runs both the travisci-skeleton script and then invokes ocveralls[5] to upload the results.

I've added a separate "make release" step which removes bisect and checks in the autogen (presumably into a release branch). Perhaps eventually this could make a github pull request (with the "hub" tool?) and make an opam package?

I think the game is made even more addictive when the coveralls badge changes colour, see:

https://coveralls.io/r/mirage/mirage-block-volume?branch=master

Here's an example bisect report (the code is a work-in-progress):

http://dave.recoil.org/tmp/report/file0000.html

If you haven't given bisect a go -- I recommend playing with it.

Also, if you can think of a nicer way to integrate this -- I admit using 'sed' on the _oasis file is a bit of a hack -- please let me know!

Cheers,
Dave

[1] http://bisect.x9c.fr
[2] https://github.com/mirage/mirage-block-volume
[3] https://github.com/mirage/shared-block-ring
[4] mirage/shared-block-ring@67b9f31
[5] https://github.com/sagotch/ocveralls

@yomimono
Copy link
Contributor

yomimono commented Jul 6, 2015

I have a branch that generates coverage reports with bisect, although it's unfortunately branched off of #155 rather than master. It's also using bisect-ppx which requires 4.02.

@yomimono
Copy link
Contributor

Now that mirage-tcpip and friends use ppx rather than camlp4, this can probably be revisited and merged properly.

@yomimono
Copy link
Contributor

yomimono commented Apr 8, 2019

I took a look at this again, inspired by #401 and my general feeling that into_cstruct is probably not well-tested compared to make_cstruct, which is bad now that into_cstruct is likely to be the usual code path in tcpip >= "3.7.0". Unfortunately, while the invocation of bisect_ppx can be set conditionally (see bisect_ppx's instructions for details), the dependency on bisect_ppx is unconditional - even if the environment variable isn't set, dune build will fail if bisect_ppx is not installed.

It seems that most projects using bisect_ppx use a solution that involves some pre-release massaging of dune files to remove bisect_ppx from preprocess (pps stanzas, and then release an opam file that doesn't mention bisect_ppx, but keep bisect_ppx in their repository-local opam files.

I think this kind of workflow is OK for repositories that have one or two very involved maintainers, but it seems error-prone for MirageOS repositories, where there's a team of maintainers that have varying amounts of involvement. I can very easily imagine myself going to make a release of a repository with this strategy and accidentally releasing the bisected version.

I'm interested if anyone has a solution in mind for this that's a bit more automatic.

@emillon
Copy link

emillon commented Apr 9, 2019

There's a consensus on the dune side about how to support that so that bisect_ppx can be a simple test dependency, but it's not implemented yet unfortunately: ocaml/dune#57

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants