Add test for ls() on a symlink to a directory

[nfischer]

This is one of our missing areas of coverage: https://codecov.io/gh/shelljs/shelljs/src/a187bd1b36ce28a5af214607257506ee28e1beb6/src/ls.js#L80

This shouldn't be too tricky to test, but I'm not sure if extra precautions need to be taken for Windows.

[dwi2]

Hello.

I tried to come up a patch for this issue but I found out something interesting. In short, I believe these issue is already covered by https://github.com/shelljs/shelljs/blob/master/test/ls.js#L333. The report of codecov might be false negative.

I did the following things:

1. It's kind of silly but I injected console.log in lines between https://github.com/shelljs/shelljs/blob/master/src/ls.js#L82 and https://github.com/shelljs/shelljs/blob/master/src/ls.js#L86, then ran unit test locally. I did see these console.log got printed.

2. Ran test locally via npm run test and the coverage report is a little bit different from https://codecov.io/gh/shelljs/shelljs/src/master/src/ls.js .
   My local coverage report looks like this: (on Mac OSX 10.11.6 with node v8.10.0)

----------------|----------|----------|----------|----------|-------------------|
File            |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------------|----------|----------|----------|----------|-------------------|
All files       |    95.81 |    89.21 |    99.24 |     96.6 |                   |
 shelljs        |      100 |      100 |      100 |      100 |                   |
  commands.js   |      100 |      100 |      100 |      100 |                   |
  global.js     |      100 |      100 |      100 |      100 |                   |
  plugin.js     |      100 |      100 |      100 |      100 |                   |
  shell.js      |      100 |      100 |      100 |      100 |                   |
 shelljs/src    |    95.74 |    89.21 |    99.22 |    96.54 |                   |
  cat.js        |      100 |      100 |      100 |      100 |                   |
  cd.js         |      100 |     87.5 |      100 |      100 |                35 |
  chmod.js      |    89.41 |    90.12 |      100 |    89.41 |... 81,183,187,192 |
  common.js     |    97.94 |    94.07 |    96.55 |     98.4 |       260,327,358 |
  cp.js         |    89.84 |    87.76 |      100 |    91.13 |... 76,177,178,179 |
  dirs.js       |    97.18 |    93.88 |      100 |    97.18 |             27,29 |
  echo.js       |      100 |    83.33 |      100 |      100 |                43 |
  error.js      |      100 |      100 |      100 |      100 |                   |
  exec-child.js |      100 |      100 |      100 |      100 |                   |
  exec.js       |    93.51 |    86.49 |      100 |     97.1 |           132,133 |
  find.js       |    95.45 |     62.5 |      100 |    95.45 |                34 |
  grep.js       |      100 |      100 |      100 |      100 |                   |
  head.js       |      100 |      100 |      100 |      100 |                   |
  ln.js         |    96.88 |    79.17 |      100 |    96.88 |                53 |
  ls.js         |    97.87 |       90 |      100 |    97.87 |                64 |
...
----------------|----------|----------|----------|----------|-------------------|

For ls.js, there is only https://github.com/shelljs/shelljs/blob/master/src/ls.js#64 is not covered, because this test is not run on Windows.
In order to see more detail, I change reporter type to html

node_modules/.bin/nyc --reporter=html --reporter=lcov node_modules/.bin/ava test/*.js

I put the html format report here.

3. I read through unit test and source code of ls.js and I think this issue is probably covered by https://github.com/shelljs/shelljs/blob/master/test/ls.js#L333 already.

Anyway, I'll try to find a linux machine and run test coverage to see if I can reproduce the same result.

[nfischer]

In short, I believe these issue is already covered by /test/ls.js@master#L333. The report of codecov might be false negative.

Looks like you're right!

I guess we should figure out the false negative then. Thanks for starting the investigation. As an idea: you could put console.log() in that branch in src/ls.js and check travis for the log message.

[dwi2]

I created a PR to verify this on travis

It looks like all tests on Travis run through https://github.com/shelljs/shelljs/blob/master/src/ls.js#L82 to #L86.

Environement	Log
Node.js v4 on Linux	https://travis-ci.org/shelljs/shelljs/jobs/368458405#L943
Node.js v4 on MacOSX	https://travis-ci.org/shelljs/shelljs/jobs/368458406#L561
Node.js v5 on Linux	https://travis-ci.org/shelljs/shelljs/jobs/368458407#L1677
Node.js v5 on MacOSX	https://travis-ci.org/shelljs/shelljs/jobs/368458408#L1411
Node.js v6 on Linux	https://travis-ci.org/shelljs/shelljs/jobs/368458409#L1677
Node.js v6 on MacOSX	https://travis-ci.org/shelljs/shelljs/jobs/368458410#L1411
Node.js v7 on Linux	https://travis-ci.org/shelljs/shelljs/jobs/368458411#L1680
Node.js v7 on MacOSX	https://travis-ci.org/shelljs/shelljs/jobs/368458412#L1410
Node.js v8 on Linux	https://travis-ci.org/shelljs/shelljs/jobs/368458413#L770
Node.js v8 on MacOSX	https://travis-ci.org/shelljs/shelljs/jobs/368458414#L387
Node.js v9 on Linux	https://travis-ci.org/shelljs/shelljs/jobs/368458415#L770
Node.js v9 on MacOSX	https://travis-ci.org/shelljs/shelljs/jobs/368458416#L386

detail log

[nfischer]

Well, that's strange. This does indeed seem to be a codecov bug (nyc reports coverage correctly). Thanks @dwi2 for catching this!

In that case, I would welcome a PR to add /* istanbul ignore next */ (and to add a comment to explain this is to workaround a codecov problem).