aantron / bisect_ppx

Code coverage for OCaml and ReScript
http://aantron.github.io/bisect_ppx/demo/
MIT License
302 stars 60 forks source link

Help with BuckleScript and bs-jest #307

Closed wyze closed 4 years ago

wyze commented 4 years ago

Hello and great project!

This is more of a help request than an issue, but I am trying to get coverage report to work when running tests via bs-jest.

Nothing is outputted when following the BuckleScript documentation. However if I add the Bisect.Runtime.write_coverage_data_on_exit(); line inside the module (not tests), and build and run the file node file.bs.js I do get output, but the coverage is at 0%.

Any help is greatly appreciated, and you can checkout the project I am trying it on here: https://github.com/wyze/bs-react-testing-library

aantron commented 4 years ago

@wyze What commands are you running to test with coverage? Can you show them? Are you including BISECT_ENABLE=yes? (this is a temporary workaround until https://github.com/BuckleScript/bucklescript/issues/3761).

wyze commented 4 years ago

Thanks for taking a look. I just pushed a branch of me attempting to get it to work. https://github.com/wyze/bs-react-testing-library/tree/bisect_ppx

Yes, I added that env variable before yarn build. On that branch you can run yarn test or npm run test and it should clean and rebuild and run the tests.

aantron commented 4 years ago

Could you please give the exact commands you enter at the terminal to try to get coverage on that branch?

wyze commented 4 years ago

I use yarn test.

aantron commented 4 years ago

I patched like this to get a coverage report:

--- a/src/__tests__/ReactTestingLibrary_test.re
+++ b/src/__tests__/ReactTestingLibrary_test.re
@@ -1,6 +1,6 @@
 open Jest;

-Bisect.Runtime.write_coverage_data_on_exit();
+afterAll(Bisect.Runtime.write_coverage_data);

 module Greeting = {
   [@react.component]

It seems that Jest isn't triggering process.on('exit') hooks. I'll add a note to the README about the workaround above, but I think it won't be suitable for projects that have multiple test files. Is there a better way to do this with Jest?

You may also want to add [@coverage exclude_file]; in your test file to exclude from the report, leaving only your code proper. And, don't forget to rm -f *.coverage before running the tests :)

wyze commented 4 years ago

Awesome! I should have tried those hooks! Yes I will add the things to .gitignore. Now I can switch my Reason projects to Codecov!

I don't mind doing a PR to update the readme and the bsb-starter-project with extra instructions for bs-jest.

aantron commented 4 years ago

I suppose one way to do this in projects with multiple test files would be to ask users to do

afterAll(() => {
  Bisect.Runtime.write_coverage_data();
  Bisect.Runtime.reset_counters();
});

in each test file. reset_counters is only exposed on native right now, so I'd need to patch Bisect. It's a pretty awkward method. If you (or anyone reading this issue in the future) know or find out how to add an exit hook globally to the whole Jest run, please let me know :)

aantron commented 4 years ago

Awesome! I should have tried those hooks! Yes I will add the things to .gitignore. Now I can switch my Reason projects to Codecov!

Great :) Regarding .gitignore, if that's in reference to rm -f *.coverage, what I meant is that you should delete the .coverage files from previous runs before each new run, otherwise the reporter will combine them all into one report, and this could show falsely inflated coverage. BTW nice coverage in your project :)

I don't mind doing a PR to update the readme and the bsb-starter-project with extra instructions for bs-jest.

I would be happy to merge such a PR :)

aantron commented 4 years ago

Perhaps rm -f *.coverage also belongs in the starter repo README...

wyze commented 4 years ago

It does look like there is this, https://jestjs.io/docs/en/configuration#globalteardown-string, but it is all JS so not sure if that would work for you.

aantron commented 4 years ago

Thanks!

It looks complicated enough that I'll keep it as a note until someone genuinely needs Bisect to support that.

wyze commented 4 years ago

So, I went and tried the same setup in another project, but when running with my test:coverage command, it throws some errors and doesn't complete. If you have some more free time, it would be awesome for you to take a look!

CI: https://github.com/wyze/bs-jest-dom/pull/13/checks?check_run_id=581289039 PR: https://github.com/wyze/bs-jest-dom/pull/13

aantron commented 4 years ago

So, I went and tried the same setup in another project, but when running with my test:coverage command, it throws some errors and doesn't complete.

That's almost certainly an issue with Bisect_ppx not respecting the special meaning of ## in BuckleScript. I opened #308 to fix that. I'll do it in the coming days, then release 2.3.1 with the fix.

aantron commented 4 years ago

I updated the README with instructions to use afterAll if using Jest, and also exposed reset_counters (as reset_coverage_data) in the BuckleScript Bisect_ppx runtime, in case Bisect_ppx needs to switch recommending that later.