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)
     
     
     

    Nit: blank line between these.

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

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

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

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

  5. bot/reviewbot/tools/nyc.py (Diff revision 3)
     
     
  6. bot/reviewbot/tools/nyc.py (Diff revision 3)
     
     
     

    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)
     
     

    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)
     
     

    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)
     
     

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

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

    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)
     
     

    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)
     
     
     

    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)
     
     

    Missing a period.

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

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

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

    Let's call this either i or index.

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

    Missing period.

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

    Remove this blank line.

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

    Needs a period.

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

    Blank line between these.

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

    Blank line between these.

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

    Missing a period.

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

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

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

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

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

    :file:`package.json`
    

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

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

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

  19. 
      
alextechcc
alextechcc
ilaw
  1. 
      
  2. In Description, there is a typo 'utalizes' and ReviewBot should be Review Bot

  3. README.rst (Diff revision 9)
     
     

    Should be listed alphabetically?

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

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

    test suite instead of test-suite to be consistent

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

    Parentheses are not needed here.

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

    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. Typo in description: 'utalizes'

  3. README.rst (Diff revision 10)
     
     

    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)
     
     

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

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

    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)
     
     

    Same comments here about Istanbul vs nyc.

  7. extension/README.rst (Diff revision 10)
     
     

    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

Diff:

Revision 11 (+275)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...