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

Improve testing coverage for the Jade CLI application #1805

Merged
merged 8 commits into from Jan 3, 2015

Conversation

Projects
None yet
3 participants
@TimothyGu
Member

TimothyGu commented Jan 2, 2015

This PR can be understood to have two parts:

  1. improve test coverage
  2. allow tracking test coverage of bin/jade.js

Improving test coverage

For the first part, I have added tests for:

  • --obj option of bin/jade.js
  • reading from stdin and the stdin() function
  • routines for prettifying the template name, i.e. the replace() call in getNameFromFileName()

Test coverage report

For the second part I had to use some "hacks" and workarounds, because, unfortunately, istanbul is not as flexible as I would like it to be:

  1. It does not support tracking JavaScript scripts spawned by child_process.spawn() or child_process.exec()`. Therefore, it is necessary to execute separate istanbul instances for every command spawned. This leads to the second problem:

  2. Istanbul does not support "concatenating" or appending coverage data. This means that, unlike gcov for C programs, when you run a new istanbul cover it overwrites the earlier data.

    To work around this, I made istanbul output data to cov-pt*/ directories with the --dir option, and made the initial parent process output to cov-pt0/ rather than coverage/. Then, I added a script to concatenate all the data together (with the istanbul API) and produce coverage/. It's not ideal, and if you have a better solution please point out the solution.

There is also the problem for the watching mode test: when passing SIGINT (or ^C) to jade when it is still watching the file, the exit handler is not called, therefore terminating istanbul as well. There is not another way of cleanly kill the process while it is watching I can think of, so I have added commit b079489 which makes SIGINT go through the exit handler. I do not expect it to change the behavior of bin/jade.js else than the return code, which callers would not depend on any way.

To-do

With the changes applied, one can see that the coverage for bin/jade.js is not nearly as green as lib/. This is a to-do list for future improvements of test coverage:

Easier:

  • Setting --obj to a file with JSON data
  • Setting options in --obj (not just locals)
  • Test compiling an entire directory
  • Test specifying output dir with --out
  • Test error handling for different things:
    • File not found

Harder:

  • Test if --help works properly
  • Test inclusion with --path
  • Test error handling for different things:
    • SIGINT when reading from stdin
    • In watch mode (program should not terminate)
@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Jan 2, 2015

Member

Current coverage:

File Statements Branches Functions Lines
jade.js 63.39% (71 / 112) 54.55% (30 / 55) 72.22% (13 / 18) 67.96% (70 / 103)
Member

TimothyGu commented Jan 2, 2015

Current coverage:

File Statements Branches Functions Lines
jade.js 63.39% (71 / 112) 54.55% (30 / 55) 72.22% (13 / 18) 67.96% (70 / 103)
@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Jan 2, 2015

Member
File % Stmts % Branches % Funcs % Lines
bin/jade.js 63.39 54.55 72.22 67.96
examples/ 50 20 66.67 47.83
lib/ 98.8 95.62 100 98.98
compiler.js 100 99.16 100 100
doctypes.js 100 100 100 100
filters.js 100 100 100 100
index.js 97.35 75.95 100 97.32
inline-tags.js 100 100 100 100
lexer.js 98.48 98.21 100 99.2
parser.js 99.73 98.76 100 99.72
runtime.js 95.7 95.29 100 95.51
utils.js 88.89 72.73 100 88.24
nodes/ 99.53 97.78 96.77 100
----------------------------------------- ----------- ----------- ----------- -----------
All files 95.84 93 96.48 96.39
Last release (not counting bin/jade.js) 98.16 95.2 98.9 98.34
Member

TimothyGu commented Jan 2, 2015

File % Stmts % Branches % Funcs % Lines
bin/jade.js 63.39 54.55 72.22 67.96
examples/ 50 20 66.67 47.83
lib/ 98.8 95.62 100 98.98
compiler.js 100 99.16 100 100
doctypes.js 100 100 100 100
filters.js 100 100 100 100
index.js 97.35 75.95 100 97.32
inline-tags.js 100 100 100 100
lexer.js 98.48 98.21 100 99.2
parser.js 99.73 98.76 100 99.72
runtime.js 95.7 95.29 100 95.51
utils.js 88.89 72.73 100 88.24
nodes/ 99.53 97.78 96.77 100
----------------------------------------- ----------- ----------- ----------- -----------
All files 95.84 93 96.48 96.39
Last release (not counting bin/jade.js) 98.16 95.2 98.9 98.34
@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Jan 3, 2015

Member

This is excellent work. Thanks for this. To make this work on Windows, we should use the rimraf npm module in place of the rm command, && in place of ; and node path/to/script.js in place of ./path/to/script.js in package.json

Member

ForbesLindesay commented Jan 3, 2015

This is excellent work. Thanks for this. To make this work on Windows, we should use the rimraf npm module in place of the rm command, && in place of ; and node path/to/script.js in place of ./path/to/script.js in package.json

@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Jan 3, 2015

Member

@ForbesLindesay said:

To make this work on Windows, we should use the rimraf npm module in place of the rm command

A separate dep just for rm -rf, really?

&& in place of ;

I intentionally changed that because I would want my coverage to run even if my test failed.

and node path/to/script.js in place of ./path/to/script.js in package.json

This is understandable, but then there are platforms like Debian where you have to use nodejs, but I would not expect the npm install to reach here anyway if that is the case ;)

Member

TimothyGu commented Jan 3, 2015

@ForbesLindesay said:

To make this work on Windows, we should use the rimraf npm module in place of the rm command

A separate dep just for rm -rf, really?

&& in place of ;

I intentionally changed that because I would want my coverage to run even if my test failed.

and node path/to/script.js in place of ./path/to/script.js in package.json

This is understandable, but then there are platforms like Debian where you have to use nodejs, but I would not expect the npm install to reach here anyway if that is the case ;)

@hemanth

This comment has been minimized.

Show comment
Hide comment
@hemanth

hemanth Jan 3, 2015

Member

👍 One of the best PRs I have come across here.

Member

hemanth commented Jan 3, 2015

👍 One of the best PRs I have come across here.

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Jan 3, 2015

Member

Generally the coverage will fail if the tests fail, so I prefer only running coverage if tests pass. Yes, sadly we need a separate dep for rm -rf. I think people who use nodejs rather than node probably have the same problem either way.

Member

ForbesLindesay commented Jan 3, 2015

Generally the coverage will fail if the tests fail, so I prefer only running coverage if tests pass. Yes, sadly we need a separate dep for rm -rf. I think people who use nodejs rather than node probably have the same problem either way.

@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Jan 3, 2015

Member

@ForbesLindesay actually rimraf is already in devdeps so I don't need to add it.

Member

TimothyGu commented Jan 3, 2015

@ForbesLindesay actually rimraf is already in devdeps so I don't need to add it.

@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Jan 3, 2015

Member

@ForbesLindesay rebased and changed.

Member

TimothyGu commented Jan 3, 2015

@ForbesLindesay rebased and changed.

* coverage HTML that covers all the individual results.
*
* E.g.
* $ cov-combine.js cov-pt[0-9]/coverage.json

This comment has been minimized.

@TimothyGu

TimothyGu Jan 3, 2015

Member

BTW, I wanted to write

cov-combine.js cov-pt*/coverage.json

here but then JS recognized */ as end of comment block so...

@TimothyGu

TimothyGu Jan 3, 2015

Member

BTW, I wanted to write

cov-combine.js cov-pt*/coverage.json

here but then JS recognized */ as end of comment block so...

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Jan 3, 2015

Member

node node_modules/rimraf/bin.js cov-pt* can be replaced with rimraf cov-pt*. You only need to call via node when it's not in PATH

We should see if we can get something useful merged in to Istanbul upstream to make this easier for other people to do (and reduce the amount of code we have to maintain as part of jade)

Other than that one small point, this looks good.

Member

ForbesLindesay commented Jan 3, 2015

node node_modules/rimraf/bin.js cov-pt* can be replaced with rimraf cov-pt*. You only need to call via node when it's not in PATH

We should see if we can get something useful merged in to Istanbul upstream to make this easier for other people to do (and reduce the amount of code we have to maintain as part of jade)

Other than that one small point, this looks good.

TimothyGu added some commits Jan 1, 2015

Support tracking code coverage in bin/jade.js
Unfortunately, istanbul is not as flexible as I would like it to be:

1. It does not support tracking JavaScript scripts spawned by
   `child_process.spawn()` or `child_process.exec()`. Therefore, it is
   necessary to execute separate istanbul instances for every command
   spawned. This leads to the second problem:

2. Istanbul does not support "concatenating" or appending coverage data.
   This means that, unlike gcov for C programs, when you run a new
   `istanbul cover` it overwrites the earlier data.

   To work around this, I made istanbul output data to cov-pt*
   directories with the initial `istanbul cover mocha` outputting to
   cov-pt0, and added a script to concatenate all the data together
   (with the istanbul API) and produce coverage/. It's not ideal, and if
   you have a better solution please file a ticket.
@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Jan 3, 2015

Member

@ForbesLindesay thanks for the review. Merged.

Member

TimothyGu commented Jan 3, 2015

@ForbesLindesay thanks for the review. Merged.

TimothyGu added a commit that referenced this pull request Jan 3, 2015

Merge pull request #1805 from jadejs/bin-cov
Improve testing coverage for the Jade CLI application

@TimothyGu TimothyGu merged commit 3e9fc2b into master Jan 3, 2015

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details

@TimothyGu TimothyGu deleted the bin-cov branch Jan 3, 2015

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