aantron / bisect_ppx Goto Github PK

All other modes (besides HTML, which creates a directory), do.

https://github.com/rleonid/bisect_ppx/blob/530a713b4227e3b55a4d8a0c5f1a28ca25df9a6c/src/report/report.ml#L91-L104

I'm noting this for later, because it's probably best done after the "big" changes are in, but:

• Using git describe to generate version.ml probably means that our code can't be built without a git repo, e.g. if downloaded by OPAM from the GH releases link. We need another way of generating this file, but I would also suggest just hardcoding it (it's no worse than having VERSION like before), and adopting a habit of doing a grep for the old version number when bumping versions.
• Need to edit the tests section in opam to do make dev before make tests.
• I don't think the str dependency in META is necessary, since str gets linked into the binaries but is not needed for the runtime. Haven't tested.

is the file format generated by running the application under test documented somewhere? if so, I'd appreciate a pointer to that. thx!

The findlib.dynload library is used by a host program to load plugins. It tracks already loaded modules, so that we can prevent corruption. It does this by embedding special initialization code into the binary. The code is put into the temporary file, and the file is later deleted. When we run bisect-ppx-report on the output of such binary, we get an error:

 *** system error: /tmp/findlib_initl44dd7c.ml: No such file or directory

Maybe it is possible to add some command-line flag, that will allow to ignore missing files? Or just relax this error to a warning?

At present, automatically testing for both 4.02 and 4.03 is impractical, because the testing framework uses diffs on -dsource output. The formatting has changed between 4.02 and 4.03. Perhaps the comparison method should be changed to some kind of structural parse tree comparison, with -dsource output printed only in case of error.

Not yet sure if this is reasonable, but I want to look into adding this translation:

match foo with
| `P1 -> e1
| _, (`P2 | `P3 as x) as y -> e2

to

let e2' x y = e2 in
match foo with
| `P1 -> point 1; e1
| _, (`P2 as x) as y -> point 2; e2' x y
| _, (`P3 as x) as y -> point 3; e2' x y

and reporting as

match foo with
| (* point 1 *) `P1 -> e1
| _, ((* point 2 *) `P2 | (* point 3 *) `P3 as x) as y -> e2

In case of nested or-patterns, the point would be on the deepest clauses. In case of a product of or-patterns, I am not sure what to do. There is both a combinatorial blow-up and no good place to put a point. Since Bisect_ppx explicitly doesn't try to understand execution paths, just whether execution passes by a point, this whole thing might be against the spirit of the tool anyway and a bad idea. In that case, I we should briefly mention why we don't do this in some new documentation we may write later.

@rleonid thoughts?

opam-builder reports that bisect_ppx needs to be updated to work on OCaml 4.04: http://opam.ocamlpro.com/builder/html/bisect_ppx/bisect_ppx.1.1.0/7b6c8c5404fcbd94d5f63514ad51d9da

+ ocamlfind ocamlopt -c -package ppx_tools -w A-4 -warn-error A-3 -I src/syntax -I doc -I src -I src/report -I src/ocamlbuild -I src/library -o src/syntax/instrumentPpx.cmx src/syntax/instrumentPpx.ml
File "src/syntax/instrumentPpx.ml", line 152, characters 4-2061:
Warning 8: this pattern-matching is not exhaustive.
Here is an example of a case that is not matched:
Ppat_open (_, _)
File "src/syntax/instrumentPpx.ml", line 1:
Error: Some fatal warnings were triggered (1 occurrences)
Command exited with code 2.

This would be not just for comparing modes against one another, but for understanding what effect modifications are having, and testing new modes proposed in #41.

Suggested by Alain Frisch: https://sympa.inria.fr/sympa/arc/caml-list/2016-02/msg00044.html

This would be in addition to comments, not instead – there doesn't seem to be a reason to break backwards compatibility.

Currently it's a file comparison against a reference. This needs to be less brittle and, for example, ignore space issues.

Our .itarget file causes us to build the native reporter unconditionally. We may want to relax that in light of this thread https://sympa.inria.fr/sympa/arc/caml-list/2016-01/msg00038.html

In tests

$ make one NAME=report
Running tests for 'report'...
     testing 'bisect' format ...
     testing 'csv' format ...
Files result.csv and reference.csv differ
     testing 'dtd' format ...
     testing 'dump' format ...
Files result.dump and reference.dump differ
     testing 'html' format ...
Files result.html and reference.html differ
     testing 'text' format ...
Files result.text and reference.text differ
     testing 'xml' format ...
Files result.xml and reference.xml differ
     testing 'xml-emma' format ...

The only difference in the report logic was replacing String.create with Bytes.create. But somewhere this broke. Really odd that the actual binary files match but not the custom formats.

See #92. I think most of the warnings we were implicitly checking for absence were related to the match expressions generated for simulating points on or-pattern clauses.

test.ml

open OUnit2

let (===) = assert_equal ~printer: String.escaped

let test_foo _ =
  "foo" === "foo"

let test_bar _ =
  "bar" === "bar"

let test_baz _ =
  "baz" === "baz"

let test_qux _ =
  "qux" === "qux"

let test_suite =
  "example" >::: [
    "foo" >:: test_foo;
    "bar" >:: test_bar;
    "baz" >:: test_baz;
    "qux" >:: test_qux;
  ]


let suite =
  "all" >::: [
    test_suite;
  ]

let _ =
  run_test_tt_main suite

$ ocamlfind ocamlc -package bisect_ppx -package oUnit -linkpkg test.ml -o test.covered
$ ./test.covered 
....
Ran: 4 tests in: 0.10 seconds.
OK
$ bisect-report -html report_dir bisect0001.out
$ xdg-open report_dir/index.html

xml version:

<?xml version="1.0" encoding="iso-8859-1"?>
<!-- generated by Bisect 0.1 (http://bisect.x9c.fr) on 2015-05-21 19:25:29 -->
<bisect-report>
  <summary>
    <element kind="binding" count="4" total="7"/>
    <element kind="sequence" count="0" total="0"/>
    <element kind="for" count="0" total="0"/>
    <element kind="if/then" count="0" total="0"/>
    <element kind="try" count="0" total="0"/>
    <element kind="while" count="0" total="0"/>
    <element kind="match/function" count="0" total="0"/>
    <element kind="class expression" count="0" total="0"/>
    <element kind="class initializer" count="0" total="0"/>
    <element kind="class method" count="0" total="0"/>
    <element kind="class value" count="0" total="0"/>
    <element kind="toplevel expression" count="1" total="1"/>
    <element kind="lazy operator" count="0" total="0"/>
    <element kind="total" count="5" total="8"/>
  </summary>
  <file path="test.ml">
    <summary>
      <element kind="binding" count="4" total="7"/>
      <element kind="sequence" count="0" total="0"/>
      <element kind="for" count="0" total="0"/>
      <element kind="if/then" count="0" total="0"/>
      <element kind="try" count="0" total="0"/>
      <element kind="while" count="0" total="0"/>
      <element kind="match/function" count="0" total="0"/>
      <element kind="class expression" count="0" total="0"/>
      <element kind="class initializer" count="0" total="0"/>
      <element kind="class method" count="0" total="0"/>
      <element kind="class value" count="0" total="0"/>
      <element kind="toplevel expression" count="1" total="1"/>
      <element kind="lazy operator" count="0" total="0"/>
      <element kind="total" count="5" total="8"/>
    </summary>
    <point offset="25" count="1" kind="binding"/>
    <point offset="83" count="0" kind="binding"/>
    <point offset="119" count="1" kind="binding"/>
    <point offset="155" count="0" kind="binding"/>
    <point offset="191" count="0" kind="binding"/>
    <point offset="227" count="1" kind="binding"/>
    <point offset="360" count="1" kind="binding"/>
    <point offset="404" count="1" kind="toplevel expression"/>
  </file>
</bisect-report>

test_foo, test_baz and test_qux are marked as untested.

Came up during #113.

The tests don't account for context information inserted by the compiler since 4.02.3. Should take care of this problem before merging #34. Not exactly sure why this is a problem only in that build only in Travis, but will look into it.

Original compiler feature request: http://caml.inria.fr/mantis/view.php?id=6497

This caused problems in #96.

Right now, running the test requires installing the Bisect_ppx under test, then running the test specifically from its directory, and it is not included in make -C tests/all.

Since the testing setup is already capable of making a development-local install of Bisect_ppx for the unit tests, it should do this for the usage test as well, and the usage test should be easy to trigger from the Makefile.

Apart from aesthetics, there are two main issues with the HTML output:

• At least on my system, ~ and - are nearly indistinguishable in Courier New when it is late and my eyes are tired. We should prepend some more modern monospace fonts before Courier New in the stylesheet to deal with this. If the fonts are not available, Courier New or whatever system monospace font will still be a fallback.
• The red background color is too dark and does not contrast enough with the black text.

Make sure that we correctly pass along the annotations in the AST.

Hi,

Is a released on opam planned for version 1.1.0? I think this is the one that supports ocaml 4.03.0.

I also suppose that the constraint on the version of ocaml in the opam file of this repository should be changed to >= 4.03.0.

Porting to https://github.com/let-def/ocaml-migrate-parsetree will allow this library to avoid cppo and make it work against more OCaml versions.

I have a question about how to find the code point for an entry in the runtime data. Given for a source file a counter in its array runtime at index i (that is, runtime.(i)), what is the corresponding code point? Is it the one with identifier i or the one at index i in the list of code points?

I'm not sure about the purpose of the identifier field in type point_definition as every point definition could be identified by its position in the list of all code points.

type point_definition = {
    offset : int; (** Point offset, relative to file beginning. *)
    identifier : int; (** Point identifier, unique in file. *)
    kind : point_kind; (** Point kind, used for statistics. *)
}

I think this connection could be documented by also providing a type definition for the runtime data and documenting it in common.mli.

The ./configure script doesn't provide very much. All the variables are either trivial or unused, except for the paths, which interfere with OPAM in the following way: if you run opam switch, you then need to rerun ./configure again for the build system to pick up the change. I say we should just use ocamlc and ocamlfind directly, and get rid of ./configure.

Also, there are a lot of redundant files in the root directory: FEATURE and README (not .md), INSTALL, and VERSION, that are there for an old source distribution convention, which doesn't matter when installing from OPAM, or just dealing with modern software in general. We should delete them. This will also make it easier to reach README.md on the GitHub page, a small but nice benefit, especially when we update the README :)

for "or" expansion.

The current release of bisect_ppx sometimes leads to timed-out Travis tests, perhaps due to some kind of deadlock in the instrumented code. The problem disappears after #50 is merged. Could you please release a version of bisect_ppx that includes those changes?

Right now, bisect-ppx-report is installed only by OPAM in the bin/ directory of the current OPAM switch. This is problematic for users who are not using OPAM.

In addition to the current behavior, we should make the install target install bisect-ppx-report in the package directory, alongside the bisect_ppx binary. It will then be possible to run it with

ocamlfind bisect_ppx/bisect-ppx-report [options]

as an alternative that works even without OPAM. If the user wants to make the binary runnable as when installing through OPAM, they can add the package directory to their PATH, or symlink the binary from a directory in their PATH.

This option for running bisect-ppx-report should be documented somewhere, probably in advanced.md.

Reported by Anarchos on IRC.

Reported by David Mentre on the mailing list: https://sympa.inria.fr/sympa/arc/caml-list/2016-02/msg00038.html

As a more radical follow-on to #102, we could also inline source code into files during instrumentation. This would make the .out files completely self-contained. Any report could be generated from them without access to the source or build directories.

We could offer a preprocessor flag such as -no-source-code for users worried about accidentally leaking intellectual property in an instrumented binary.

I'm not sure this is a good idea at all, and I don't think it has a use case yet, but I would like to hear any opinions.

• Inline source.
• Deprecate -ignore-missing-files option.

Bisect_ppx needs to work well with other ppx rewriters in any order:
ppx_deriving -> bisect_ppx and bisect_ppx -> ppx_deriving

It would be nice to be able to #require (or compile aganist) instrumented libraries without dragging the ppx instrumentation.

Test suite should be able to test both byte and native code.

The original instrumentation logic implemented a fast mode. I kept the source and made it compile but have not tested it.

opam pin add --dev-repo ppx_tools
opam pin add -y .         # In bisect_ppx directory

Does not work on 4.03.0+beta1.

In addition to fixing the Bisect_ppx code, we may have to update previously-released opam files to prevent attempts at installing old Bisect_ppx on 4.03.

This is probably a bad idea because it will break every single use of Bisect_ppx.

However, I always found it annoying that the extension is .out.

• It conflicts with a.out when trying to run simple tests.
• It's easy to collide with output of other tools.
• It's not descriptive.

Any thoughts?

• Announce upcoming change to .coverage in 1.5.0.
• Make the change in 2.0.0.
• Show a message if the reporter receives a .out argument, or no file arguments.

There is no need to have a soft transition, because it's more effort than just making the transition.

Currently it looks like although top-level values can be excluded by regex, excluded filenames must be specified exactly in the exclusion file. It would be great to support regex for filenames as well, for example to allow excluding unit tests themselves from coverage via ^test_.*.

...when you hover a point in the HTML report. Unconfirmed.

Add them to advanced.md but link from the README. Probably something based on the following, which was worked out by icicled and reported on IRC:

# _oasis

Flag coverage
  Description: Collect coverage data
  Default:     false



# myocamlbuild.ml

let has_coverage () =
  let c = Scanf.Scanning.from_file "setup.data"
  and stop = ref false
  and enabled = ref false in
  while not !stop do
    try
      Scanf.bscanf c "%s@=\"%s@\" "
        (fun k v ->
           if k = "coverage"
           then (stop := true;
                 enabled := v = "true"))
    with
    | _ -> stop := true
  done;
  Scanf.Scanning.close_in c;
  !enabled

let () =
  let additional_rules = function
    | After_rules     ->
      if has_coverage () then
        begin
          flag ["ocaml"; "compile"] (S [A "-package"; A "bisect_ppx"]);
          flag ["ocaml"; "link"] (S [A "-package"; A "bisect_ppx"])
        end
      else
        ()
    | _ -> ()
  in
  dispatch
    (MyOCamlbuildBase.dispatch_combine
       [MyOCamlbuildBase.dispatch_default conf package_default;
        additional_rules])

If you use Bisect_ppx point kinds, please let us know. Otherwise, we may remove them.

Bisect_ppx currently has "match" points, "binding" points, etc., and breaks down coverage statistics by point kind (i.e. "your tests visited 53% of the functions, 17% of the match cases," etc).

I'm not aware of a use case for this information. I've already removed it from the HTML reports since January, and we got no complaints.

There is also some talk of removing them from other reports, because then the reports could be generated without reading the .cmp files (not to mention, IMO, the reports would become much cleaner).

This

let try_finally x f h = (* point *) ...

is equivalent to

let try_finally = fun x -> fun f -> fun h -> (* point *) ...

But we want:

let try_finally = (* point *) fun x -> fun f -> fun h -> (* point *) ...

Reported by David Mentre on the mailing list: https://sympa.inria.fr/sympa/arc/caml-list/2016-02/msg00038.html

The Bisect "library" was originally included in the installation only so that instrumented programs could be linked with the runtime. It's an internal interface. However, it has made Bisect somewhat "hackable" (ocveralls, bisect-summary). That's a good thing :)

To avoid problems like in #108, and more that are on the way, I think we should try to design a good replacement interface. "Good" means both easy to use, and abstract enough to withstand serious changes in Bisect_ppx.

Proposal

The interface I can suggest now is something quite similar to @lindig's proposal. This would replace Bisect.Common:

(* module Bisect.Coverage *)

type location
(* Abstract, see below. *)

val location_to_offset : location -> int
val location_to_line : location -> int

type point =
  {location : location;
   hits     : int}

type file =
  {points      : point array;
   line_count  : int;
   source_code : string option}

type t = (string, file) Hashtbl.t

val read : ?source_code_directory:string -> ~out_files:string list -> coverage

At a file system level, there would be only .out files. They would be self-contained. They would include everything now in .out files, .cmp files (#102), and the source code (#103). So, a tool would only need to find a collection of them, and pass their names to read. This should make both programmatic use and use of bisect-ppx-report easier – especially in contexts where there isn't access to the build directories.

Details

• Lines are included in locations, so that it's not necessary to walk over the source to convert offsets to lines. Also, the source may be missing.

• Type location is abstract, because we might need to switch to location ranges to address a potential future problem:

  There may be some challenges, when an expression has the same location as some relevant subexpression.

  I'd also like the freedom to decide between using Location.t, and thus depending on compiler-libs, or not.

• A digest can be easily computed from source_code. We can include the digest, so that ocveralls can work even if the source is missing, but I'm wondering if it's not better for ocveralls to simply fail in that case. @lindig, what was your reason for including the digest?

• line_count is there for the same reason as lines in locations, so that you can know how many lines to report as unvisited without scanning the source, especially if it is missing.

• The source can be missing if we provide the -no-source-code option:

  We could offer a preprocessor flag such as -no-source-code for users worried about accidentally leaking intellectual property in an instrumented binary.

  I'm not really sure if it's even necessary to provide it. It would simplify the interface if we knew we don't have to worry about this. However, if someone does use -no-source-code, the ?source_code_directory option to read would be a fallback to look for source files in the file system.

• If we proceed with this, Bisect_ppx's library will become incompatible with Bisect's. That will complicate ocveralls, if it wants to be compatible with both. @sagotch, what do you think?

Work

• .cmp (#102).
• Sources (#103). Abandoned. See #187 (comment).
• Provide the new interface.
• Hide (what's left of) Bisect.Common or discourage its use, e.g. by renaming it to Internal. Bisect.Common was renamed to Bisect_common as part of #206. This isn't quite hiding, but it's enough to disrupt any current users.
• Avoid use of Marshal for .out files (see #114). Done as part of #206, see 923f088.

Hello,

I'm not able to compile a file which contains strings with backslashes.
I'm using the 0.2 release from opam.

Here is an example:

$ ls                                                                             
actors.ml  test.ml
$ cat actors.ml 
type t =
  | Anthony
  | Caesar
  | Cleopatra

let message = function
  | Anthony     -> "foo\\"
  | Caesar      -> "\\bar"
  | Cleopatra   -> "Fool! Don't you see now that I could have poisoned you\
                    a hundred times had I been able to live without you."
$ cat test.ml
open Actors

let () =
  print_endline (message Cleopatra);
  print_endline (message Anthony);
$ ocamlfind ocamlc -package bisect_ppx -linkpkg actors.ml test.ml -o test.covered
lexing error at line 11: unexpected end of file
File "actors.ml", line 1:
Error: Error while running external preprocessor
Command line: /home/louis/.opam/4.02.1+PIC/lib/bisect_ppx/./bisect_ppx.byte '/tmp/camlppx5982c4' '/tmp/camlppxdb6336'

There are several issues here.

1. -fast is also apparently safe. @rleonid if -fast is faster than -safe (is it?), why is there -safe? Is -safe more correct than -fast in some case?
2. I want to write a little performance test so we can understand the real performance of the various modes.
3. I want to also write a race condition test.
4. The difference between -fast and -faster is that -faster doesn't try to take locks even if you link in BisectThread.
   • The reason for locks in either of these modes is that they increment counts. Are counts actually often used from Bisect_ppx? If we were only updating true/false flags, we could write true without locks in all cases. Maybe what we need instead of -fast and -faster is -boolean and -numeric, and if you choose -numeric, it is always correct, but you pay a price for locks.
   • If we don't want to do this, I want to look at the actual performance difference between -fast with locks and -faster. It's probably significant, but if it isn't, I would like to eliminate -faster.
   • There may also be a way to use the ocaml.ppx.context attribute, or some other method, to find out if we need to take locks. This would eliminate the brittleness of BisectThread, but also take per-module control over safety away from the user compiling the code. Does anyone actually take advantage of this control?

I'm currently hoping to make work -boolean/-numeric with automatic detection of the need for locks in -numeric. I'd probably want to make -boolean the default. It wouldn't affect the report tool. The runtime would just output counts 0 and 1.

1. Remove EMMA reports.

   This presumably tries to mimic EMMA, an old Java coverage tool. Wikipedia states that it has not been maintained since 2005 or so. The presumed replacement JaCoCo seems to have incompatible XML reports (JaCoCo DTD vs. our EMMA code).

2. Remove XML, CSV, dump reports, "bisect" reports.

   None of these are used much (no uses in GitHub). We should pick one machine-friendly output format, and provide only that. @lindig suggested JSON – that may be the best idea. Outputting coveralls.io JSON does not seem good, however, because that loses information about points and measures only lines. @sagotch?

   EDIT: Actually, I think -text is going to be quite easy to read, once we get rid of point kinds. So, maybe we can remove all of these. If someone later has a strong reason for supporting an additional machine-friendly format, we can consider it then.

3. De-emphasize -combine-expr.

   This seems like a cool feature in principle, but I don't think anybody is using it (0 uses in GitHub between Bisect_ppx and the original Bisect).

When using locally abstract types, the exclusions do not work. I have created a branch of this repo and added a (failing) testcase here:

rleonid/bisect_ppx@rleonid:master...simonjbeaumont:locally-abstract-types

The preprocessor should not add instrumentation to this function since it matches the regexp in the exclusions file. However it does:

$ make -C tests one NAME=ppx-exclude-file
make: Entering directory `/local/work/code/bisect_ppx/tests'
Running tests for 'ppx-exclude-file'...
make[1]: Entering directory `/local/work/code/bisect_ppx/tests/ppx-exclude-file'
     testing 'source.ml' ...
Files source.ml.reference and source.ml.result differ
make[1]: *** [default] Error 1
make[1]: Leaving directory `/local/work/code/bisect_ppx/tests/ppx-exclude-file'
make: Leaving directory `/local/work/code/bisect_ppx/tests'

$ (cd tests/ppx-exclude-file; diff -u source.ml.reference source.ml.result)
--- source.ml.reference 2015-06-08 15:22:37.932377656 +0100
+++ source.ml.result    2015-06-08 15:27:43.124377656 +0100
@@ -4,4 +4,10 @@
   Bisect.Runtime.mark "source.ml" 1;
   for i = 1 to 5 do (Bisect.Runtime.mark "source.ml" 0; print_endline s) done
 let f2 b x = if b then x * x else x
-let f3 : 'a . 'a -> string= fun (type a) -> (fun _  -> "Hello" : a -> string)
+let f3 : 'a . 'a -> string=
+  Bisect.Runtime.mark "source.ml" 2;
+  fun
+  (type
+  a)
+  ->
+  ((fun _  -> "Hello") : a -> string)

I generated the additional code in the .reference file using the (*BISECT-IGNORE-BEGIN/END*) bracketing.

My motivation for this is my use of this extension with https://github.com/simonjbeaumont/ocaml-pci which uses ctypes to generate bindings and ctypes uses code of the above form. I want to exclude the generated code from my results.

I might take a look around the code when I get a change see if I can come up with a pull request but any pointers would be appreciated.

Thanks in advance!

Christian Lindig (@lindig) has suggested providing a report mode that doesn't require reading the .cmp files, which would be useful in testing environments where they are not readily available (note: see lindig/bisect-summary). In fact, bisect-ppx-report -text - -summary-only doesn't actually need the .cmp (or source) files, but it reads them anyway.

I think it's a good idea to do this at least for -text -summary-only, and probably -text, in any case, but we should remove .cmp files entirely.

• A .cmp file is basically a point list: for each point, its offset into the file and kind. We could inline this information at the top level of each instrumented file, and write it into .out files. bisect-ppx-report would then deduplicate this information when reading those files.
• I never was fully comfortable with the instrumenter writing .cmp files next to the source files as a side effect of operation. That works out fine under some build systems like Ocamlbuild, because the build is done on copies of source in _build, but it seems messy otherwise (even if only as messy as having .cm[iox] files everywhere).
• It should no longer be necessary to use -I with any output mode except HTML, because HTML needs to find the source files.

EDIT: -summary-only does print point kinds.

An installation via Make or Opam doesn't install any MLI files:

$ ls $(ocamlfind query bisect_ppx)
META            bisect.cmx      bisect_ppx_plugin.cmi
bisect-ppx-report   bisect.cmxa     bisect_ppx_plugin.cmo
bisect.a        bisect.o        bisect_ppx_plugin.cmx
bisect.cma      bisect_ppx      bisect_ppx_plugin.cmxa
bisect.cmi      bisect_ppx_plugin.a bisect_ppx_plugin.o
bisect.cmo      bisect_ppx_plugin.cma   opam.config

This makes it difficult to use the installed library. I'd like to suggest to extend the definition of LIBARAY_FILES in the Makefile (line 155) and to install MLI files.