Add nyc code coverage tool integration

Review Request #10195 — Created Oct. 4, 2018 and updated

Information

ReviewBot
master
73a2f5f...

Reviewers

Integration with ReviewBot has been added for the nyc code coverage
tool.

This tool can be configured to run on most JavaScript setups by
providing a list of install steps and test command in the integration
configuration.

It utilizes the 'json-summary' backend to provide basic per-file
thresholds and opens issues on files that do not meet these thresholds.

Documentation has been added for this tool.

Tool has been tested with a basic JavaScript test repository with
multiple thresholds, using npm and jasmine-node. Other frameworks
have had basic testing done.

Description From Last Updated

In Description, there is a typo 'utalizes' and ReviewBot should be Review Bot

ilawilaw

Typo in description: 'utalizes'

brenniebrennie

F841 local variable 'fp' is assigned to but never used

reviewbotreviewbot

F821 undefined name 'f'

reviewbotreviewbot

E265 block comment should start with '# '

reviewbotreviewbot

Nit: blank line between these.

brenniebrennie

This doesn't need to be in the with block.

brenniebrennie

Does nyc work on JSX (.jsx)? Does it work on TypeScript .ts/.tsx?

brenniebrennie

a

brenniebrennie

You will want to use the os.path module here. / is not guaranteed to be the path separator on every …

brenniebrennie

Can we pass in cwd from the parent method? If there are lots of files, that is a lot of …

brenniebrennie

You do not need to append the path sep. That is what os.path.join is for.

brenniebrennie

os.path.join(t) is just t. You don't need it.

brenniebrennie

In what case does f.get_patched_file_path() return None ?

brenniebrennie

Comments should be full sentences, i.e., they should begin with a capital letter and end with a period.

brenniebrennie

If you're using the first_line logic you added in your other patch, you'll want to mark this review request as …

brenniebrennie

Missing a period.

brenniebrennie

What if there are quoted substrings? This should use shlex.split().

brenniebrennie

Let's call this either i or index.

brenniebrennie

Missing period.

brenniebrennie

Remove this blank line.

brenniebrennie

Needs a period.

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

Missing a period.

brenniebrennie

``istanbul``

brenniebrennie

``npm``

brenniebrennie

:file:`package.json`

brenniebrennie

``nodejs``

brenniebrennie

:file:`package.json` ``test``

brenniebrennie

``nodejs``

brenniebrennie

This should be a rst link. It is missing a period.

brenniebrennie

Should be listed alphabetically?

ilawilaw

test suite instead of test-suite to be consistent

ilawilaw

Parentheses are not needed here.

ilawilaw

Parentheses are not needed here.

ilawilaw

Should probably mention for what language(s) it does code coverage. Also since the actual name of the tool is Istanbul …

brenniebrennie

Should probably mention for what language(s) it does code coverage.

brenniebrennie

how about %s: %s%% coverage less than minimum threshold (%s%%) just to make it clear that its about coverage

brenniebrennie

Same comments here about Istanbul vs nyc.

brenniebrennie

Here too

brenniebrennie
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

alextechcc
alextechcc
brennie
  1. 
      
  2. bot/reviewbot/tools/nyc.py (Diff revision 3)
     
     
     
    Show all issues

    Nit: blank line between these.

  3. bot/reviewbot/tools/nyc.py (Diff revision 3)
     
     
     
    Show all issues

    This doesn't need to be in the with block.

  4. bot/reviewbot/tools/nyc.py (Diff revision 3)
     
     
    Show all issues

    Does nyc work on JSX (.jsx)? Does it work on TypeScript .ts/.tsx?

  5. bot/reviewbot/tools/nyc.py (Diff revision 3)
     
     
    Show all issues

    a

  6. bot/reviewbot/tools/nyc.py (Diff revision 3)
     
     
     
    Show all issues

    You will want to use the os.path module here. / is not guaranteed to be the path separator on every platform. Technically you could run the reviewbot worker on Windows if you needed to run a Windows-only tool.

    import os
    
    # ...
    
    file_coverage = coverage[os.path.join(os.getcwd(), path)]
    
  7. bot/reviewbot/tools/nyc.py (Diff revision 3)
     
     
    Show all issues

    Can we pass in cwd from the parent method? If there are lots of files, that is a lot of calls to os.getcwd(), which isn't going to change.

  8. 
      
alextechcc
brennie
  1. 
      
  2. bot/reviewbot/tools/nyc.py (Diff revision 4)
     
     
    Show all issues

    You do not need to append the path sep. That is what os.path.join is for.

  3. bot/reviewbot/tools/nyc.py (Diff revision 4)
     
     
    Show all issues

    os.path.join(t) is just t. You don't need it.

  4. bot/reviewbot/tools/nyc.py (Diff revision 4)
     
     
    Show all issues

    In what case does f.get_patched_file_path() return None ?

    1. I saw it included in most other tools, looks like it returns None on an API error.

  5. bot/reviewbot/tools/nyc.py (Diff revision 4)
     
     
    Show all issues

    Comments should be full sentences, i.e., they should begin with a capital letter and end with a period.

  6. bot/reviewbot/tools/nyc.py (Diff revision 4)
     
     
     
    Show all issues

    If you're using the first_line logic you added in your other patch, you'll want to mark this review request as depending on the other.

  7. 
      
alextechcc
alextechcc
alextechcc
alextechcc
alextechcc
alextechcc
brennie
  1. 
      
  2. bot/reviewbot/tools/nyc.py (Diff revision 8)
     
     
     
     
     
     
     

    Do we want to just pass this as a file (or input) to /bin/sh ?

  3. bot/reviewbot/tools/nyc.py (Diff revision 8)
     
     
    Show all issues

    Missing a period.

  4. bot/reviewbot/tools/nyc.py (Diff revision 8)
     
     
    Show all issues

    What if there are quoted substrings? This should use shlex.split().

  5. bot/reviewbot/tools/nyc.py (Diff revision 8)
     
     
    Show all issues

    Let's call this either i or index.

  6. bot/reviewbot/tools/nyc.py (Diff revision 8)
     
     
    Show all issues

    Missing period.

  7. bot/reviewbot/tools/nyc.py (Diff revision 8)
     
     
    Show all issues

    Remove this blank line.

  8. bot/reviewbot/tools/nyc.py (Diff revision 8)
     
     
    Show all issues

    Needs a period.

  9. bot/reviewbot/tools/nyc.py (Diff revision 8)
     
     
     
    Show all issues

    Blank line between these.

  10. bot/reviewbot/tools/nyc.py (Diff revision 8)
     
     
     
    Show all issues

    Blank line between these.

  11. bot/reviewbot/tools/nyc.py (Diff revision 8)
     
     
    Show all issues

    Missing a period.

  12. docs/reviewbot/tools/nyc.rst (Diff revision 8)
     
     
    Show all issues

    ``istanbul``
    
  13. docs/reviewbot/tools/nyc.rst (Diff revision 8)
     
     
    Show all issues

    ``npm``
    
  14. docs/reviewbot/tools/nyc.rst (Diff revision 8)
     
     
    Show all issues

    :file:`package.json`
    
  15. docs/reviewbot/tools/nyc.rst (Diff revision 8)
     
     
    Show all issues

    ``nodejs``
    
  16. docs/reviewbot/tools/nyc.rst (Diff revision 8)
     
     
    Show all issues

    :file:`package.json`
    

    ``test``
    
  17. docs/reviewbot/tools/nyc.rst (Diff revision 8)
     
     
    Show all issues

    ``nodejs``
    
  18. docs/reviewbot/tools/nyc.rst (Diff revision 8)
     
     
    Show all issues

    This should be a rst link. It is missing a period.

  19. 
      
alextechcc
alextechcc
ilaw
  1. 
      
  2. Show all issues

    In Description, there is a typo 'utalizes' and ReviewBot should be Review Bot

  3. README.rst (Diff revision 9)
     
     
    Show all issues

    Should be listed alphabetically?

    1. Sorry, must be missing something, isn't it alphabetical?

  4. bot/reviewbot/tools/nyc.py (Diff revision 9)
     
     
    Show all issues

    test suite instead of test-suite to be consistent

  5. bot/reviewbot/tools/nyc.py (Diff revision 9)
     
     
    Show all issues

    Parentheses are not needed here.

  6. bot/reviewbot/tools/nyc.py (Diff revision 9)
     
     
    Show all issues

    Parentheses are not needed here.

  7. docs/reviewbot/tools/nyc.rst (Diff revision 9)
     
     

    Why is it necessary to provide the test command?

  8. 
      
alextechcc
alextechcc
brennie
  1. 
      
  2. Show all issues

    Typo in description: 'utalizes'

  3. README.rst (Diff revision 10)
     
     
    Show all issues

    Should probably mention for what language(s) it does code coverage.

    Also since the actual name of the tool is Istanbul (whereas nyc is just the cli) should this be something like:

    Istanbul (nyc)

  4. bot/README.rst (Diff revision 10)
     
     
    Show all issues

    Should probably mention for what language(s) it does code coverage.

  5. bot/reviewbot/tools/nyc.py (Diff revision 10)
     
     
    Show all issues

    how about %s: %s%% coverage less than minimum threshold (%s%%) just to make it clear that its about coverage

  6. docs/reviewbot/tools/nyc.rst (Diff revision 10)
     
     
    Show all issues

    Same comments here about Istanbul vs nyc.

  7. extension/README.rst (Diff revision 10)
     
     
    Show all issues

    Here too

  8. 
      
alextechcc
Review request changed
Description:
   

Integration with ReviewBot has been added for the nyc code coverage

    tool.

   
   

This tool can be configured to run on most JavaScript setups by

    providing a list of install steps and test command in the integration
    configuration.

   
~  

It utalizes the 'json-summary' backend to provide basic per-file

  ~

It utilizes the 'json-summary' backend to provide basic per-file

    thresholds and opens issues on files that do not meet these thresholds.

   
   

Documentation has been added for this tool.

Commit:
b120855e9876bd5a2212f9f216bc48a8f2a4efe3
73a2f5f66d71b3222edef64ca4900610b982a896

Checks run (2 succeeded)

flake8 passed.
JSHint passed.