enb / enb-bem-specs

BEM specs for ENB
Other
14 stars 22 forks source link

Include uncovered files with zero coverage #29

Closed dragn closed 9 years ago

dragn commented 9 years ago

Files that are not covered by specs should, obviously, be included in statistics with zero coverage.

blond commented 9 years ago

/cc @SevInf

blond commented 9 years ago

@dragn Hi! Sorry for the long wait, I was hoping that @SevInf will help with review this PR.

Could you give a real example with uncovered files?

dragn commented 9 years ago

Hi. (I've added a minor fix that passes root path to istanbul matcher). About the example... I would add a block with uncovered code to the existing "silly" example, but the thing is: current setup does not use correct path during istrumentation. You can see here that borschik-tech-istanbul uses current process' working directory, which does not correspond to the root context of "silly" example. Anyway, here is my attempt to add a block with uncovered code to "silly" example: https://github.com/dragn/enb-bem-specs/tree/example But the report generated by istanbul looks as follows:

> node_modules/.bin/istanbul report text --include examples/silly/coverage.json
--------------------------------|-----------|-----------|-----------|-----------|
File                            |   % Stmts |% Branches |   % Funcs |   % Lines |
--------------------------------|-----------|-----------|-----------|-----------|
   blocks/block/                |         0 |       100 |         0 |         0 |
      block.js                  |         0 |       100 |         0 |         0 |
   blocks/uncovered/            |         0 |       100 |         0 |         0 |
      uncovered.js              |         0 |       100 |         0 |         0 |
   examples/silly/blocks/block/ |     33.33 |       100 |         0 |     33.33 |
      block.js                  |     33.33 |       100 |         0 |     33.33 |
--------------------------------|-----------|-----------|-----------|-----------|
All files                       |     11.11 |       100 |         0 |     11.11 |
--------------------------------|-----------|-----------|-----------|-----------|

See, what I'm talking about? Paths, instrumented by borschik, should be relative to where coverage,json is, but they are not. On the other hand, everything is fine, when block directories are located at the root of the project. BTW, we are successfully using my setup for our project for a few months now (not open-sourced).

blond commented 9 years ago

Thanks for the detailed description of the problem!

I do not like that this strategy (include uncovered files) is not native for istanbul. It is more like unit-coverage. But I understand that it is very useful for BEM.

I propose to solve this problem differently.

At this point we have all the js files.

We can build a dummy coverage.json with zero coverage for all blocks before run tests. And after starting tests this file will be overwritten.

@dragn, What do you think?

blond commented 9 years ago

@dragn, ping.

dragn commented 9 years ago

@blond, well I've tried to implement the approach you were talking about, but it did not make it simpler. If we write the coverage.json with all files with zero-coverage in plugin.js, we still need to read that in runner.js and merge it with the output of test runs. As it's implemented now, we take the list of all sources, exclude those already covered by specs, and put them in the resulted data with zero coverage. That means that at this step we need to know which files are already covered. I think that current implementation is okay. (readability is so-so, but istanbul's API is not the best out there...)

blond commented 9 years ago

@dragn ok, I agree ;)

I created Pull Request to borschik-tech-istanbul with root option: https://github.com/bem/borschik-tech-istanbul/pull/4.

After it change will appear in borschik-tech-istanbul then we can correct the error found in your PR.

blond commented 9 years ago

I fixed bug with root in https://github.com/enb-bem/enb-bem-specs/compare/b3431f941be3b72b5c6c1e42059ff0d8c80890b2...3c44b0962227ea8a181b07e6181012a40a68e1b1

-------------------|-----------|-----------|-----------|-----------|
File               |   % Stmts |% Branches |   % Funcs |   % Lines |
-------------------|-----------|-----------|-----------|-----------|
   block/          |     33.33 |       100 |         0 |     33.33 |
      block.js     |     33.33 |       100 |         0 |     33.33 |
   uncovered/      |         0 |       100 |         0 |         0 |
      uncovered.js |         0 |       100 |         0 |         0 |
-------------------|-----------|-----------|-----------|-----------|
All files          |     16.67 |       100 |         0 |     16.67 |
-------------------|-----------|-----------|-----------|-----------|
dragn commented 9 years ago

Awesome! Thanks!

blond commented 9 years ago

Fixed in v0.5.7.