Use bisect PPX instead of Jest coverage #261

jihchi · 2020-04-19T07:31:31Z

Relevant issue: #156

This is an attempt to try bisect ppx for code coverage and see how it goes.

jihchi · 2020-04-19T07:59:44Z

First successful coverage report by bisect ppx: https://coveralls.io/builds/30172931

aantron · 2020-04-19T08:42:50Z

__tests__/Relude_IO_test.re

@@ -2,6 +2,9 @@ open Jest;
 open Expect;
 open Relude.Globals;

+[@coverage exclude_file];
+afterAll(Bisect.Runtime.write_coverage_data);


Argh :/ I think we need some better way of doing this globally in a Jest project. The problem I ran into is that when Jest exits, it somehow doesn't trigger process.on('exit') hooks.

Also, a near-future version of Bisect will support excluding files by pattern in the ppx-flags. In the meantime, you could use an exclusions file: https://github.com/aantron/bisect_ppx/blob/master/doc/advanced.md#Excluding

In the meantime, you could use an exclusions file: https://github.com/aantron/bisect_ppx/blob/master/doc/advanced.md#Excluding

I've tried without success, could you please advise?

I added a dune file in the root of project:

(preprocess (pps bisect_ppx --exclusions bisect.exclude)) (preprocessor_deps (file bisect.exclude))

And added a bisect.exclude file next to dune:

file regexp "*_test.bs.js" file regexp "node_modules/*"

I understood that you are building with BuckleScript, so this should go into bsconfig.json, not a dune file. I think (haven't tried) you can pass the option like this:

"ppx-flags": [["bisect_ppx/ppx", "--exclusions", "bisect.exclude"]]

like so.

Then, the file patterns are regexps, not globs, so

file regexp ".*_test.bs.js"

and you won't be able to exclude node_modules from your project, because instrumentation of bs-bastet is controlled from the upstream node_modules/bs-bastet/bsconfig.json, and is best disabled by rescript-lang/rescript#3761.

it seems like bisect has not supported relative path of exclusion file:

"ppx-flags": [["bisect_ppx/ppx", "--exclusions", "bisect.exclude"]]

where got errors:

> relude@0.61.0 build /Users/jihchi/Repos/relude > bsb -make-world [11/11] Building src/runtime/bucklescript/runtime-Bisect.cmj [81/81] Building bastet/src/Test-BsBastet.cmj [5/5] Building src/jest.cmj [1/456] Building __tests__/Relude_Eq_test.reast FAILED: __tests__/Relude_Eq_test.reast /Users/jihchi/Repos/relude/node_modules/bs-platform/darwin/bsc.exe -w +A-4-40-42 -warn-error +A -color always -ppx '/Users/jihchi/Repos/relude/node_modules/bisect_ppx/ppx --exclusions bisect.exclude' -bs-no-version-header -bs-super-errors -o __tests__/Relude_Eq_test.reast -bs-syntax-only -bs-binary-ast /Users/jihchi/Repos/relude/__tests__/Relude_Eq_test.re File "_none_", line 1: Error: I/O error: bisect.exclude: No such file or directory We've found a bug for you! /Users/jihchi/Repos/relude/__tests__/Relude_Eq_test.re Error while running external preprocessor Command line: /Users/jihchi/Repos/relude/node_modules/bisect_ppx/ppx --exclusions bisect.exclude '/var/folders/wm/lp071lgd09nb3_mv_9pthcs00000gn/T/camlppx7aa76b' '/var/folders/wm/lp071lgd09nb3_mv_9pthcs00000gn/T/camlppx94f0b2' ( ...omitted repeated error with different re file... )

And then, I changed to absolute path:

"ppx-flags": [ ["bisect_ppx/ppx", "--exclusions", "/Users/jihchi/Repos/relude/bisect.exclude"] ]

where got another errors:

> relude@0.61.0 build /Users/jihchi/Repos/relude > bsb -make-world [11/11] Building src/runtime/bucklescript/runtime-Bisect.cmj [81/81] Building bastet/src/Test-BsBastet.cmj [5/5] Building src/jest.cmj [1/456] Building __tests__/Relude_Eq_test.reast FAILED: __tests__/Relude_Eq_test.reast /Users/jihchi/Repos/relude/node_modules/bs-platform/darwin/bsc.exe -w +A-4-40-42 -warn-error +A -color always -ppx '/Users/jihchi/Repos/relude/node_modules/bisect_ppx/ppx --exclusions /Users/jihchi/Repos/relude/bisect.exclude' -bs-no-version-header -bs-super-errors -o __tests__/Relude_Eq_test.reast -bs-syntax-only -bs-binary-ast /Users/jihchi/Repos/relude/__tests__/Relude_Eq_test.re open Jest open Expect open Relude.Globals [@@@coverage exclude_file] ;;afterAll Bisect.Runtime.write_coverage_data ( ... omitted re content ...) File "/var/folders/wm/lp071lgd09nb3_mv_9pthcs00000gn/T/camlppx931ed4", line 1: Error: I can't decide whether /var/folders/wm/lp071lgd09nb3_mv_9pthcs00000gn/T/camlppx931ed4 is an implement ation or interface file We've found a bug for you! /Users/jihchi/Repos/relude/__tests__/Relude_Eq_test.re Error while running external preprocessor Command line: /Users/jihchi/Repos/relude/node_modules/bisect_ppx/ppx --exclusions /Users/jihchi/Repos/relude/bisect.exclude '/var/folders/wm/lp071lgd09nb3_mv_9pthcs00000gn/T/camlppx08d86e' '/var/folders/wm/lp071lgd09nb3_mv_9pthcs00000gn/T/camlppx931ed4' ( ...omitted repeated error with different re file... )

These are probably two issues:

BuckleScript is running Bisect from some unexpected directory (Bisect does support relative paths).

There is something wrong with assumptions Bisect makes about the order of arguments coming from BuckleScript.

Could you push your latest code, that triggers these errors, into this branch, and I'll fix as much of this as I can in Bisect?

Ok aantron/bisect_ppx#310 and aantron/bisect_ppx#311 fixed it at least in my testing. I'm waiting for CI to build binaries, and then I'll push release 2.3.2 to npm. I looked at the report, and it looks like test cases were excluded successfully. bs-bastet is still included, but that's waiting on BuckleScript.

2.3.2 is in npm with the fixes.

I looked at the report, and it looks like test cases were excluded successfully.

I guess that's not what I checked, since the exclude attributes were still inside each test file :/

I looked at the report, and it looks like test cases were excluded successfully.

I guess that's not what I checked, since the exclude attributes were still inside each test file :/

--exclusions works with version >=2.3.2. thanks for speedy fixing on bisect ppx!

BTW, I was wrong about bisect.exclude, it should be .re extension:

relude/bisect.exclude

Lines 1 to 2 in 777d04d

file regexp ".*_test\.re$"

file regexp ".*\/testUtils\/.*\.re$"

Latest code coverage report: https://coveralls.io/builds/30182377

aantron · 2020-04-19T08:46:42Z

package.json

@@ -32,6 +33,7 @@
  "license": "MIT",
  "devDependencies": {
    "@glennsl/bs-jest": "^0.5.1",
+    "bisect_ppx": "^2.3.1",


bisect_ppx has to be a regular dep; see aantron/bisect_ppx#309 (comment). This is because the ppx-flags in bsconfig.json cause Bisect to always be called. It's just when BISECT_ENABLE=yes is missing, Bisect leaves the code clean and uninstrumented. But the code still gets passed through Bisect.

Once BuckleScript releases dev ppx-flags (rescript-lang/rescript#3761) in the next release, it will be correct to move bisect_ppx to devDependencies, which is where it properly belongs :)

aantron · 2020-04-20T02:57:56Z

If you can find a way to add an exit hook to Jest in one place, please let me know. Having to add afterAll to each file really is awkward. I'd be happy to offer better instructions. We had a discussion about that starting here recently: aantron/bisect_ppx#307 (comment).

Bisect will need a command-line flag for excluding files soon, as part of the improved future BuckleScript integration I've linked too many times :) I might as well add that now, so you can avoid a separate exclusions file, which is also awkward. It's going to allow passing regexps on the command line (i.e. through ppx-flags).

aantron · 2020-04-20T05:52:55Z

Bisect_ppx master now allows dropping the bisect.exclude file in favor of

  "ppx-flags": [
    ["bisect_ppx/ppx",
      "--exclude-files", ".*_test\\.re",
      "--exclude-files", ".*\/testUtils\/.*\\.re"]
  ]

but I want to hold off on a release until the Jest exit hook issue.

mlms13 · 2020-04-20T12:28:17Z

Just wanted to thank you both for the work you're doing on this! I'm excited to see where it ends up.

andywhite37 · 2020-04-21T02:01:31Z

Yeah, thank you for taking a stab at this!

aantron · 2020-04-21T07:56:43Z

Ok, bisect_ppx@2.4.0 is now in npm. In addition to being able to delete bisect.exclude in favor of ppx-flags, you should also be able to delete jest.setup.js, because it has been upstreamed into Bisect (thanks!). In package.json,

"setupFilesAfterEnv": ["bisect_ppx/src/runtime/bucklescript/jest.js"]

should be enough.

Thanks for pushing this so we can smooth out the rough corners of the integration.

There is still some work left on the BuckleScript side to exclude bs-bastet and make Bisect into a true dev-only dependency.

jihchi · 2020-04-21T08:07:38Z

This PR is better and cleaner now!

Thanks @aantron for the help on bisect ppx and make this happen!

jihchi · 2020-04-21T08:24:13Z

the coverage report is also better and more sense now, take src/Relude_NonEmpty.re for example:

Bisect (new, generated by npx bisect-ppx-report html)
Jest (old)

aantron · 2020-04-21T08:29:22Z

Great :)

Is that a comparison of Bisect (new) and Jest (old)? I got confused because of the big screenshot :)

aantron · 2020-04-21T08:30:55Z

BTW, it's best to use the local HTML report (bisect-ppx-report html), because it shows expression-level coverage. Coveralls only supports per-line coverage, which can be confusing when there are several expressions on one line, or it can be not so clear what exactly about an expression triggered it to be instrumented.

aantron · 2020-04-21T08:32:24Z

I basically use Coveralls only for the GitHub status and/or nag comment, or if anyone wants to examine the coverage without generating a report locally. As a developer, I use the HTML report during development (or the summary report for super quick iteration when I'm not trying to cover specific problem lines exactly yet).

mlms13 · 2020-04-26T16:57:03Z

I notice the title is no longer "WIP" and the PR is no longer marked as a draft! :) How are we feeling about this? I glanced through the PR, and I trust the two of you to self-review more than I trust myself on this one, but it looks good to me.

aantron · 2020-04-26T17:00:40Z

It looks right to me.

mlms13 · 2020-04-26T17:05:59Z

Oh wow, I just noticed that this gives Coveralls everything it needs to show the real report:

previously, attempted to show .bs.js files which aren't checked in: https://coveralls.io/builds/30212942/source?filename=lib/js/src/array/Relude_Array_Base.bs.js
this branch, showing the real report for Reason files: https://coveralls.io/builds/30334367/source?filename=src/array/Relude_Array_Base.re

This is super exciting, and I'm ready to merge if you feel good about it @jihchi

andywhite37 · 2020-04-26T17:37:27Z

I think my only reservation is that bisect-ppx has to be listed as a dependency at this time, rather than a devDependency. How close are they to supporting ppxs as devDependencies? If it's a long way off, we can probably just merge this, but I'd rather it be a devDependency.

aantron · 2020-04-26T17:41:29Z

@bobzhang has suggested that the feature needed for Bisect_ppx to become a devDependency may be included in the next BuckleScript release.

mlms13 · 2020-04-26T17:41:44Z

Since bastet already has this listed as a dependency (for the same reason), we're already bringing this as a transitive dependency, so I'm not too concerned about having to add it to our package.json (though I agree it would be nice for Bucklescript to support dev ppxs).

andywhite37 · 2020-04-26T17:43:06Z

Ok, we might as well just merge it then. I'll open an issue to make it a devDependency when that is possible to do.

jihchi · 2020-04-27T00:55:07Z

Thanks again @aantron for the help.

Thanks @mlms13 , @andywhite37

jihchi added 2 commits April 19, 2020 14:49

Add bisect_ppx library as a dev dependency

1bce8b1

Invoke write converage data in each test file

611527e

jihchi changed the title ~~Use bisect PPX instead of Jest coverage~~ WIP: Use bisect PPX instead of Jest coverage Apr 19, 2020

jihchi added 2 commits April 19, 2020 15:37

Update gitlab actions for coverage

377eeb9

BISECT_ENABLE should be applied during build time

b062582

jihchi mentioned this pull request Apr 19, 2020

How to exclude "node_modules" in BuckleScript/Jest project? aantron/bisect_ppx#309

Closed

aantron reviewed Apr 19, 2020

View reviewed changes

Move bisect_ppx to dependencies

5534dee

This was referenced Apr 19, 2020

Support taking extra options from BuckleScript aantron/bisect_ppx#310

Closed

Debug path to exclude file on BuckleScript aantron/bisect_ppx#311

Closed

jihchi added 3 commits April 19, 2020 21:35

Add exclustions argument to ppx

7dc2fbc

Remove [@coverage exclude_file]; in favor of --exclusions

3de615f

Fix incorrect regexp in bisect.exclude

777d04d

This was referenced Apr 20, 2020

Use Jest in the BuckleScript starter repo, or make a new one aantron/bisect_ppx#312

Closed

Support file exclusions passed through PPX flags aantron/bisect_ppx#313

Closed

Figure out how to add a global exit hook in Jest aantron/bisect_ppx#314

Closed

jihchi added 2 commits April 20, 2020 15:30

Wirte coverage data once after all test suites

f6f3211

Use "setupFilesAfterEnv" to invoke bisect to write code coverage

3eb79cc

Use --exclude-files and bisect Jest hook

1975bb4

Update Developers.md for coverage

3b9b07e

jihchi marked this pull request as ready for review April 25, 2020 05:46

jihchi changed the title ~~WIP: Use bisect PPX instead of Jest coverage~~ Use bisect PPX instead of Jest coverage Apr 25, 2020

jihchi mentioned this pull request Apr 26, 2020

Run bisect conditionally wyze/bs-react-testing-library#20

Merged

mlms13 merged commit 0f70f29 into reazen:master Apr 26, 2020

jihchi deleted the use_bisect branch April 27, 2020 00:54

aantron mentioned this pull request Jun 3, 2020

bsconfig: per-directory and dev ppx-flags needed for Bisect_ppx (coverage) rescript-lang/rescript#3761

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use bisect PPX instead of Jest coverage #261

Use bisect PPX instead of Jest coverage #261

jihchi commented Apr 19, 2020 •

edited

Loading

jihchi commented Apr 19, 2020

aantron Apr 19, 2020

jihchi Apr 19, 2020

aantron Apr 19, 2020 •

edited

Loading

jihchi Apr 19, 2020

aantron Apr 19, 2020

aantron Apr 19, 2020

aantron Apr 19, 2020

aantron Apr 19, 2020

jihchi Apr 20, 2020

jihchi Apr 20, 2020

aantron Apr 19, 2020

aantron commented Apr 20, 2020 •

edited

Loading

aantron commented Apr 20, 2020

mlms13 commented Apr 20, 2020

andywhite37 commented Apr 21, 2020

aantron commented Apr 21, 2020 •

edited

Loading

jihchi commented Apr 21, 2020 •

edited

Loading

jihchi commented Apr 21, 2020 •

edited

Loading

aantron commented Apr 21, 2020

aantron commented Apr 21, 2020

aantron commented Apr 21, 2020

mlms13 commented Apr 26, 2020

aantron commented Apr 26, 2020

mlms13 commented Apr 26, 2020

andywhite37 commented Apr 26, 2020

aantron commented Apr 26, 2020 •

edited

Loading

mlms13 commented Apr 26, 2020

andywhite37 commented Apr 26, 2020

jihchi commented Apr 27, 2020

	file regexp ".*_test\.re$"
	file regexp ".\/testUtils\/.\.re$"

Use bisect PPX instead of Jest coverage #261

Use bisect PPX instead of Jest coverage #261

Conversation

jihchi commented Apr 19, 2020 • edited Loading

jihchi commented Apr 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aantron Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

relude/bisect.exclude Lines 1 to 2 in 777d04d file regexp ".*_test\.re$" file regexp ".*\/testUtils\/.*\.re$"

Choose a reason for hiding this comment

aantron commented Apr 20, 2020 • edited Loading

aantron commented Apr 20, 2020

mlms13 commented Apr 20, 2020

andywhite37 commented Apr 21, 2020

aantron commented Apr 21, 2020 • edited Loading

jihchi commented Apr 21, 2020 • edited Loading

jihchi commented Apr 21, 2020 • edited Loading

aantron commented Apr 21, 2020

aantron commented Apr 21, 2020

aantron commented Apr 21, 2020

mlms13 commented Apr 26, 2020

aantron commented Apr 26, 2020

mlms13 commented Apr 26, 2020

andywhite37 commented Apr 26, 2020

aantron commented Apr 26, 2020 • edited Loading

mlms13 commented Apr 26, 2020

andywhite37 commented Apr 26, 2020

jihchi commented Apr 27, 2020

jihchi commented Apr 19, 2020 •

edited

Loading

aantron Apr 19, 2020 •

edited

Loading

relude/bisect.exclude

Lines 1 to 2 in 777d04d

file regexp "._test\.re$"

file regexp ".\/testUtils\/.*\.re$"

aantron commented Apr 20, 2020 •

edited

Loading

aantron commented Apr 21, 2020 •

edited

Loading

jihchi commented Apr 21, 2020 •

edited

Loading

jihchi commented Apr 21, 2020 •

edited

Loading

aantron commented Apr 26, 2020 •

edited

Loading