Add nyc code coverage tool integration

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

alextechcc
ReviewBot
master
10280, 10095
b120855...
reviewbot, students

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
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.

  • 0
  • 0
  • 32
  • 3
  • 35
Description From Last Updated
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
Review request changed

Summary:

-Adding nyc tool to ReviewBot
+Add nyc code coverage tool integration
Loading...