Summary

Add nyc code coverage tool integration

Review Request #10195 — Created Oct. 4, 2018 and updated Dec. 19, 2018, 9:47 p.m.

Information

Owner

alextechcc

Repository

ReviewBot

Branch

master

Bugs

Depends On

~~10280~~, ~~10095~~

Commit

73a2f5f...

Reviewers

Groups

reviewbot, students

People

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

Testing Done

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.

Issues

Description	From	Last Updated
In Description, there is a typo 'utalizes' and ReviewBot should be Review Bot	ilaw	Nov. 30, 2018, 5:53 a.m.
Typo in description: 'utalizes'	brennie	Dec. 19, 2018, 9:47 p.m.
F841 local variable 'fp' is assigned to but never used	reviewbot	Oct. 4, 2018, 6:52 p.m.
F821 undefined name 'f'	reviewbot	Oct. 4, 2018, 6:52 p.m.
E265 block comment should start with '# '	reviewbot	Oct. 4, 2018, 6:52 p.m.
Nit: blank line between these.	brennie	Oct. 12, 2018, 3:16 a.m.
This doesn't need to be in the with block.	brennie	Oct. 12, 2018, 3:17 a.m.
Does nyc work on JSX (.jsx)? Does it work on TypeScript .ts/.tsx?	brennie	Oct. 12, 2018, 3:17 a.m.
a	brennie	Oct. 12, 2018, 3:17 a.m.
You will want to use the os.path module here. / is not guaranteed to be the path separator on every …	brennie	Oct. 12, 2018, 3:17 a.m.
Can we pass in cwd from the parent method? If there are lots of files, that is a lot of …	brennie	Oct. 12, 2018, 3:17 a.m.
You do not need to append the path sep. That is what os.path.join is for.	brennie	Oct. 13, 2018, 11:07 p.m.
os.path.join(t) is just t. You don't need it.	brennie	Oct. 13, 2018, 11:05 p.m.
In what case does f.get_patched_file_path() return None ?	brennie	Oct. 18, 2018, 4:22 p.m.
Comments should be full sentences, i.e., they should begin with a capital letter and end with a period.	brennie	Oct. 13, 2018, 11:09 p.m.
If you're using the first_line logic you added in your other patch, you'll want to mark this review request as …	brennie	Oct. 13, 2018, 11 p.m.
Missing a period.	brennie	Nov. 16, 2018, 5:48 a.m.
What if there are quoted substrings? This should use shlex.split().	brennie	Nov. 16, 2018, 5:28 a.m.
Let's call this either i or index.	brennie	Nov. 16, 2018, 5:41 a.m.
Missing period.	brennie	Nov. 16, 2018, 5:41 a.m.
Remove this blank line.	brennie	Nov. 16, 2018, 5:41 a.m.
Needs a period.	brennie	Nov. 16, 2018, 5:42 a.m.
Blank line between these.	brennie	Nov. 16, 2018, 5:42 a.m.
Blank line between these.	brennie	Nov. 16, 2018, 5:42 a.m.
Missing a period.	brennie	Nov. 16, 2018, 5:42 a.m.
``istanbul``	brennie	Nov. 16, 2018, 5:44 a.m.
``npm``	brennie	Nov. 16, 2018, 5:48 a.m.
:file:`package.json`	brennie	Nov. 16, 2018, 5:45 a.m.
``nodejs``	brennie	Nov. 16, 2018, 5:45 a.m.
:file:`package.json` ``test``	brennie	Nov. 16, 2018, 5:45 a.m.
``nodejs``	brennie	Nov. 16, 2018, 5:45 a.m.
This should be a rst link. It is missing a period.	brennie	Nov. 16, 2018, 5:48 a.m.
Should be listed alphabetically?	ilaw	Nov. 30, 2018, 6 a.m.
test suite instead of test-suite to be consistent	ilaw	Nov. 30, 2018, 6:01 a.m.
Parentheses are not needed here.	ilaw	Nov. 30, 2018, 6:02 a.m.
Parentheses are not needed here.	ilaw	Nov. 30, 2018, 6:02 a.m.
Should probably mention for what language(s) it does code coverage. Also since the actual name of the tool is Istanbul …	brennie	Dec. 19, 2018, 9:38 p.m.
Should probably mention for what language(s) it does code coverage.	brennie	Dec. 19, 2018, 9:38 p.m.
how about %s: %s%% coverage less than minimum threshold (%s%%) just to make it clear that its about coverage	brennie	Dec. 19, 2018, 9:35 p.m.
Same comments here about Istanbul vs nyc.	brennie	Dec. 19, 2018, 9:38 p.m.
Here too	brennie	Dec. 19, 2018, 9:38 p.m.

flake8 failed.

JSHint passed.

flake8

bot/reviewbot/tools/nyc.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F841 local variable 'fp' is assigned to but never used
```
bot/reviewbot/tools/nyc.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F821 undefined name 'f'
```
bot/reviewbot/tools/nyc.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E265 block comment should start with '# '
```

Commit:

7b4bd0eba22e059e65813fcd12c27696baf93d86

1454491dcee593a8b1c05393b7977fdd89488a2a

Diff:

Revision 2 (+144)

Show changes

	bot/setup.py
	bot/reviewbot/tools/nyc.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

1454491dcee593a8b1c05393b7977fdd89488a2a

22376009836b5673d918cc945ceaa24023589619

Diff:

Revision 3 (+141)

Show changes

	bot/setup.py
	bot/reviewbot/tools/nyc.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

bot/reviewbot/tools/nyc.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Nit: blank line between these.
```
bot/reviewbot/tools/nyc.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This doesn't need to be in the with block.
```
bot/reviewbot/tools/nyc.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Does nyc work on JSX (.jsx)? Does it work on TypeScript .ts/.tsx?
```
bot/reviewbot/tools/nyc.py (Diff revision 3)
The issue has been resolved. Show all issues
```
a
```

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

The issue has been resolved. 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)]

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

The issue has been resolved. 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.

Change Summary:

Less spam by combining per-file messages. Code nitpick cleanup, only consider files that nyc actually tested.

Commit:

22376009836b5673d918cc945ceaa24023589619

121a4f862aadd3d5ee257959ffe53686bab9b45d

Diff:

Revision 4 (+25 -13)

Show changes

bot/reviewbot/tools/nyc.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

bot/reviewbot/tools/nyc.py (Diff revision 4)
The issue has been resolved. Show all issues
```
You do not need to append the path sep. That is what os.path.join is for.
```
bot/reviewbot/tools/nyc.py (Diff revision 4)
The issue has been resolved. Show all issues
```
os.path.join(t) is just t. You don't need it.
```

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

The issue has been resolved. Show all issues

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

alextechcc

Oct. 13, 2018, 11:09 p.m.

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

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

The issue has been resolved. Show all issues

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

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

The issue has been resolved. 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.

Change Summary:: Depends on whole-file fix
Depends On:: ~~10219 - [WIP] Potential whole-file fix for file modification when commenting~~

Change Summary:

Rebased on 10095 (Add ability for bots to make general comments)

Depends On:

~~10219 - [WIP] Potential whole-file fix for file modification when commenting~~

~~10095 - Adding the ability for bots to make general comments~~

Commit:

121a4f862aadd3d5ee257959ffe53686bab9b45d

bf2de3b357ab2bd534159c6d94dbb5bf1e72fa58

Diff:

Revision 5 (+165)

Show changes

	bot/setup.py
	bot/reviewbot/tools/nyc.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Added documentation, only comment if there are issues and the file in question is modified, much nicer comments that explain issue, added config section to manually implant a .nycrc.

Commit:

bf2de3b357ab2bd534159c6d94dbb5bf1e72fa58

4cbd43b1a5af4553cfc2d2e264d020aa1ec42fcf

Diff:

Revision 6 (+248)

Show changes

	README.rst
	bot/README.rst
	bot/setup.py
	bot/reviewbot/tools/nyc.py
	docs/reviewbot/tools/index.rst
	docs/reviewbot/tools/nyc.rst

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Depends On:: ~~10280 - File modification checking multi-line support and cleanup~~

Change Summary:

Big cleanup, removed dependency on being run in an npm environment, updated documentation. NOTE: some debug print statements, ignore those

Commit:

4cbd43b1a5af4553cfc2d2e264d020aa1ec42fcf

d6d30e3b7b17fc49ebade6ac307526f1d63611c8

Diff:

Revision 7 (+273)

Show changes

	README.rst
	bot/README.rst
	bot/setup.py
	bot/reviewbot/tools/nyc.py
	docs/reviewbot/tools/index.rst
	docs/reviewbot/tools/nyc.rst

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

d6d30e3b7b17fc49ebade6ac307526f1d63611c8

d79b12c01896208be65cff84146434c123c59adc

Diff:

Revision 8 (+271)

Show changes

	README.rst
	bot/README.rst
	bot/setup.py
	bot/reviewbot/tools/nyc.py
	docs/reviewbot/tools/index.rst
	docs/reviewbot/tools/nyc.rst

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

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

bot/reviewbot/tools/nyc.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Missing a period.
```
bot/reviewbot/tools/nyc.py (Diff revision 8)
The issue has been resolved. Show all issues
```
What if there are quoted substrings? This should use shlex.split().
```
bot/reviewbot/tools/nyc.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Let's call this either i or index.
```
bot/reviewbot/tools/nyc.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Missing period.
```
bot/reviewbot/tools/nyc.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Remove this blank line.
```
bot/reviewbot/tools/nyc.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Needs a period.
```
bot/reviewbot/tools/nyc.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Blank line between these.
```
bot/reviewbot/tools/nyc.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Blank line between these.
```
bot/reviewbot/tools/nyc.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Missing a period.
```
docs/reviewbot/tools/nyc.rst (Diff revision 8)
The issue has been resolved. Show all issues
```
``istanbul``
```
docs/reviewbot/tools/nyc.rst (Diff revision 8)
The issue has been resolved. Show all issues
```
``npm``
```
docs/reviewbot/tools/nyc.rst (Diff revision 8)
The issue has been resolved. Show all issues
```
:file:`package.json`
```
docs/reviewbot/tools/nyc.rst (Diff revision 8)
The issue has been resolved. Show all issues
```
``nodejs``
```

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

The issue has been resolved. Show all issues

:file:`package.json`

``test``

docs/reviewbot/tools/nyc.rst (Diff revision 8)
The issue has been resolved. Show all issues
```
``nodejs``
```
docs/reviewbot/tools/nyc.rst (Diff revision 8)
The issue has been resolved. Show all issues
```
This should be a rst link. It is missing a period.
```

Commit:

d79b12c01896208be65cff84146434c123c59adc

d7d10622567613fdd59e23f6a480fab1b49fbd2d

Diff:

Revision 9 (+272)

Show changes

	README.rst
	bot/README.rst
	bot/setup.py
	bot/reviewbot/tools/nyc.py
	docs/reviewbot/tools/index.rst
	docs/reviewbot/tools/nyc.rst

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:: Removed WIP tag, updated description fields.
Summary:: [WIP] Adding nyc tool to ReviewBot
Adding nyc tool to ReviewBot
Description:: ~
[WIP] Adding nyc tool to ReviewBot
~
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.
Testing Done:: +
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.

The issue has been resolved. Show all issues

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

README.rst (Diff revision 9)
The issue has been resolved. Show all issues
```
Should be listed alphabetically?
```
1. alextechcc
  
  Nov. 30, 2018, 5:57 a.m.
  Sorry, must be missing something, isn't it alphabetical?
bot/reviewbot/tools/nyc.py (Diff revision 9)
The issue has been resolved. Show all issues
```
test suite instead of test-suite to be consistent
```
bot/reviewbot/tools/nyc.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Parentheses are not needed here.
```
bot/reviewbot/tools/nyc.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Parentheses are not needed here.
```

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

Why is it necessary to provide the test command?

Commit:

d7d10622567613fdd59e23f6a480fab1b49fbd2d

b120855e9876bd5a2212f9f216bc48a8f2a4efe3

Diff:

Revision 10 (+275)

Show changes

	README.rst
	bot/README.rst
	bot/setup.py
	bot/reviewbot/tools/nyc.py
	docs/reviewbot/tools/index.rst
	docs/reviewbot/tools/nyc.rst
	extension/README.rst

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Summary:: Adding nyc tool to ReviewBot
Add nyc code coverage tool integration

The issue has been resolved. Show all issues
```
Typo in description: 'utalizes'
```

README.rst (Diff revision 10)

The issue has been resolved. 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)

bot/README.rst (Diff revision 10)

The issue has been resolved. Show all issues

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

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

The issue has been resolved. Show all issues

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

docs/reviewbot/tools/nyc.rst (Diff revision 10)
The issue has been resolved. Show all issues
```
Same comments here about Istanbul vs nyc.
```
extension/README.rst (Diff revision 10)
The issue has been resolved. Show all issues
```
Here too
```

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

	README.rst
	bot/README.rst
	bot/setup.py
	bot/reviewbot/tools/nyc.py
	docs/reviewbot/tools/index.rst
	docs/reviewbot/tools/nyc.rst
	extension/README.rst

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

~		[WIP] Adding nyc tool to ReviewBot
	~	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.