[NSP] badge with tests #1156

danielbankhead · 2017-10-11T17:46:30Z

A continuation of #803. Scoped to nsp/npm/:package:format for future-proofing.

paulmelnikow · 2017-10-11T18:37:53Z

Hi! Thanks for this.

Could you review the CI logs?

The new service tests didn't run in the first CI pass, though I added brackets around [NSP] in the PR title and hopefully they will in the next one.

danielbankhead · 2017-10-11T19:46:55Z

I can't get the test to run locally via npm run test:services -- --only=nsp (server refuses to start), so I'm pretty shooting in the dark. I've fixed the linter issue, but I'm not sure why the second set of tests are now failing.

paulmelnikow · 2017-10-11T19:52:05Z

Does npm test work locally? Are you on Node 6+?

danielbankhead · 2017-10-11T20:28:00Z

Got it! The local testing issue was a timing problem (the tests timeout if it takes more than 5 seconds to start - not sure why it takes so long on my machine).

As for the badge generator it was a minor match-related bug.

paulmelnikow · 2017-10-11T20:34:41Z

service-tests/nsp.js

+
+t.create('get a package that does not exist')
+  .get('/npm/some-unknown-package.json')
+  .expectJSONTypes(Joi.object().keys({name: 'nsp', value: 'n/a'}))


Is it possible to distinguish between package not found and a server error? In the first case, it would be good to provide a more helpful error message. In the second case, would you please use invalid, which is consistent with the other badges?

That's fair. Any preferred colorscheme for each? How would you recommend catching server errors?

I'd suggest figuring out what a response looks like for a non-existent package, and testing for that specifically. Once you've identified and excluded that case, if available === false I'd say that's a server error.

Most of the other badges use the default color for errors.

paulmelnikow · 2017-10-11T20:36:06Z

service-tests/nsp.js

+
+t.create('get a package without vulnerabilities')
+  .get('/npm/bronze.json')
+  .expectJSONTypes(Joi.object().keys({name: 'nsp', value: 'no known vulnerabilities'}))


Since these are both literals, you can write this a bit more concisely:

.expectJSON({ name: 'nsp', value: 'no known vulnerabilities' })

paulmelnikow · 2017-10-12T04:26:43Z

server.js

+    if (response !== null && response.statusCode === 404) {
+      badgeData.text[1] = 'invalid';
+    } else if (error !== null || jsonResults === undefined) {
+      badgeData.text[1] = 'error';


Though error might be more descriptive, for consistency, this one should be invalid. The one before should be something like package not found.

By the way, does NSP support scoped packages like @cycle/core? If so could you add a test to make sure it works for that too? It might require different quoting or a different regex.

It looks like NSP supports scoped packages. I'll update the implementation to support them.

I'll also add support for specific package versions - perhaps /nsp/npm/l/:package.:format for the latest version and /nsp/npm/v/:package/:version.:format for a specific version?

To be consistent with the other badges, /nsp/npm/:package.:format for latest and /nsp/npm/:package/:version.:format for specific.

/l/ usually means license and /v/ means version. The equivalent here would be something like /nsp/count/npm/, if you thought there might be badges for other kinds of nsp data in the future.

danielbankhead · 2017-10-12T21:26:20Z

I'm not sure why this latest build failed - I'm not using any escape characters where it claims I am [actual line], but I was able to add support for both version and scoped package in this latest commit. Locally I'm not getting the error at all.

paulmelnikow · 2017-10-12T21:33:46Z

Travis tests on a commit created by merging your branch into master, which explains line number discrepancies with the branch (and tooling discrepancies).

If you merge latest master into your dev branch and push that, you'll have parity locally with CI. Don't forget to npm install.

paulmelnikow · 2017-10-12T21:38:59Z

server.js

+
+        sendBadge(format, badgeData);
+      } else {
+        nspRequestOptions.body.package.version = body.version;


Perhaps this would be easier to reason about if it passed the version into getNspResults as a parameter.

paulmelnikow · 2017-10-12T21:42:29Z

service-tests/nsp.js

+
+const t = new ServiceTester({id: 'nsp', title: 'Node Security Platform'})
+
+const formats = {


I love the idea of generating a bunch of tests!

paulmelnikow · 2017-10-12T21:42:40Z

service-tests/nsp.js

+
+const formatsKeys = Object.keys(formats)
+
+for (let i = 0, len = formatsKeys.length; i < len; i++) {


You could write it like this:

Object.keys(formats).forEach(format => { });

paulmelnikow · 2017-10-12T21:43:24Z

server.js

+  const pieces = match.input.split('/');
+  let badgeData = getBadgeData('nsp', data);
+  let format = '';
+  let nspRequestOptions = {


Since these identifiers are not reassigned, they can all be const.

paulmelnikow · 2017-10-12T22:57:55Z

templates/flat-square-template.svg

-    <text x="{{=((it.widths[0]+it.logoWidth+it.logoPadding)/2)*10}}" y="140" transform="scale(0.1)">{{=it.escapeXml(it.text[0])}}</text>
-    <text x="{{=(it.widths[0]+it.widths[1]/2-1)*10}}" y="140" transform="scale(0.1)">{{=it.escapeXml(it.text[1])}}</text>
+    <text x="{{=(((it.widths[0]+it.logoWidth+it.logoPadding)/2)+1)*10}}" y="140" transform="scale(0.1)" textLength="{{=(it.widths[0]-(10+it.logoWidth+it.logoPadding))*10}}" lengthAdjust="spacing">{{=it.escapeXml(it.text[0])}}</text>
+    <text x="{{=(it.widths[0]+it.widths[1]/2-1)*10}}" y="140" transform="scale(0.1)" textLength="{{=(it.widths[1]-10)*10}}" lengthAdjust="spacing">{{=it.escapeXml(it.text[1])}}</text>


Could you reset these files? e.g. git checkout master templates/flat-square-template.svg, and then commit, should do it.

paulmelnikow · 2017-10-13T14:16:32Z

server.js

+
+    request(npmURL, npmRequestOptions, (error, response, body) => {
+      if (response !== null && response.statusCode === 404) {
+        // NOTE: in POST requests nsp does not distinguish between 'package not found' and 'no known vulnerabilities'. To keep consistency in the use case where a version is provided (which skips `getNpmVersionThenNspResults()` altogether) we'll say 'no known vulnerabilities' since it is technically true in both cases


Could you break this comment around 80 characters for legibility?

paulmelnikow · 2017-10-13T14:19:39Z

server.js

+  // D: /nsp/npm/@:scope/:package/:version.:format
+  const pieces = match.input.split('/');
+  const badgeData = getBadgeData('nsp', data);
+  const nspRequestOptions = {


It's difficult to understand how data is flowing between the main body of this badge provider and the local functions. To make this clearer, could you declare this nspRequestOptions structure inside getNspResults instead? You could pass in the package name as a parameter, like you're doing with the package version.

paulmelnikow · 2017-10-13T14:22:34Z

server.js

+    });
+  }
+
+  function separateAndSetFormat (str = '') {


This is confusing. Could you pull the format out of the regex match and assign it right up top? That way this can be a pure function / would not need to set format. It's the way all the other badges handle it.

paulmelnikow · 2017-10-13T15:57:52Z

Much clearer, thank you!

Added nsp integration with tests

ecd4576

paulmelnikow changed the title ~~NSP badge with tests~~ [NSP] badge with tests Oct 11, 2017

paulmelnikow added the service-badge Accepted and actionable changes, features, and bugs label Oct 11, 2017

Make linter happy

5bb81d0

Fix matching bug

4177edc

paulmelnikow reviewed Oct 11, 2017

View reviewed changes

Distinguish between package not found and server error

4e818e2

paulmelnikow reviewed Oct 12, 2017

View reviewed changes

Added support for versions and scoped packages

69dae7e

paulmelnikow reviewed Oct 12, 2017

View reviewed changes

danielbankhead added 6 commits October 12, 2017 17:47

Merge from shields/master

03daccf

Cleanup

ed35943

Update from badges/master

ae9be7c

Fix conflict

90653ca

Update from badges/master

16c1031

Fix unnecessary escape characters

20b439d

paulmelnikow reviewed Oct 12, 2017

View reviewed changes

Reset template files

057a066

paulmelnikow reviewed Oct 13, 2017

View reviewed changes

danielbankhead added 2 commits October 13, 2017 11:22

Cleanup

3cd91bb

Merge remote-tracking branch 'badges/master'

44615c7

paulmelnikow merged commit a073a2b into badges:master Oct 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NSP] badge with tests #1156

[NSP] badge with tests #1156

danielbankhead commented Oct 11, 2017 •

edited

paulmelnikow commented Oct 11, 2017

danielbankhead commented Oct 11, 2017

paulmelnikow commented Oct 11, 2017

danielbankhead commented Oct 11, 2017

paulmelnikow Oct 11, 2017

danielbankhead Oct 11, 2017

paulmelnikow Oct 11, 2017

paulmelnikow Oct 11, 2017

paulmelnikow Oct 12, 2017

danielbankhead Oct 12, 2017

danielbankhead Oct 12, 2017 •

edited

paulmelnikow Oct 12, 2017

danielbankhead commented Oct 12, 2017

paulmelnikow commented Oct 12, 2017

paulmelnikow Oct 12, 2017

paulmelnikow Oct 12, 2017

paulmelnikow Oct 12, 2017

paulmelnikow Oct 12, 2017

paulmelnikow Oct 12, 2017

paulmelnikow Oct 13, 2017

paulmelnikow Oct 13, 2017

paulmelnikow Oct 13, 2017

paulmelnikow commented Oct 13, 2017


		const t = new ServiceTester({id: 'nsp', title: 'Node Security Platform'})

		const formats = {


		const formatsKeys = Object.keys(formats)

		for (let i = 0, len = formatsKeys.length; i < len; i++) {

[NSP] badge with tests #1156

[NSP] badge with tests #1156

Conversation

danielbankhead commented Oct 11, 2017 • edited

paulmelnikow commented Oct 11, 2017

danielbankhead commented Oct 11, 2017

paulmelnikow commented Oct 11, 2017

danielbankhead commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbankhead Oct 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbankhead commented Oct 12, 2017

paulmelnikow commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmelnikow commented Oct 13, 2017

danielbankhead commented Oct 11, 2017 •

edited

danielbankhead Oct 12, 2017 •

edited