[hhvm] #1499 - update hhvm service #1508

bkdotcom · 2018-02-16T16:59:32Z

http://hhvm.h4cc.de... -> https://php-eye.com/api/v1/package...

shields-ci · 2018-02-16T17:01:37Z

	Messages
📖	✨ Thanks for your contribution to Shields, @bkdotcom!

Generated by 🚫 dangerJS

chris48s · 2018-02-16T17:50:17Z

Thanks for submitting a PR for this. I'll submit comments for review shortly

chris48s

I've left a few minor inline comments.

Additionally, could you possibly add a small test suite for your code. This will help to verify that the code works correctly and will hopefully allow an error like the one you've reported in #1499 to be flagged up earlier in future.

A minimal test suite for this badge should cover the following cases:

expected behaviour
package not found
connection error
unexpected response

There is documentation on writing tests in https://github.com/badges/shields/blob/master/service-tests/README.md but something like https://github.com/badges/shields/blob/master/service-tests/requires.js or https://github.com/badges/shields/blob/master/service-tests/shippable.js would be a good example to crib from for this badge.

If you create your tests in a file called hhvm.js, edit your PR title to something like "[hhvm] my pull request title" and Circle CI will run the tests for that service when you push additional commits.

Thanks again.

chris48s · 2018-02-16T17:51:15Z

server.js

+      var verInfo = {};
+      for (var i = 0, count = data.versions.length; i < count; i++) {
+        verInfo = data.versions[i];
+        if (verInfo.name == branch) {


Could you use a strict comparison here please (triple equals)

chris48s · 2018-02-16T17:52:32Z

server.js

+        verInfo = data.versions[i];
+        if (verInfo.name == branch) {
+          switch (verInfo.travis.hhvm) {
+          case 0:


This switch statement would be easier to read if the cases were indented by a block (sorry to leave feedback on indenting :( this should really be picked up by a linting rule!)

will do (I copy/pasted a switch statement from elsewhere in the code)

chris48s · 2018-02-16T17:54:01Z

server.js

-    // Remove the leading slash.
-    apiUrl += '?branch=' + branch.slice(1);
-  }
+  var apiUrl = 'https://php-eye.com/api/v1/package/'+user+'.json';


Could you update the variable declarations to use the ES6 style let and const as applicable while you're working on this code.

bkdotcom · 2018-02-16T18:24:59Z

I'll create some tests asap when I actually have time to get the environment up and running... so far the edits were simply done on github.com

chris48s

Great. Thanks for adding tests. This is shaping up nicely. I've left a few more comments about the tests but I think we're fairly close to a final check and getting this merged :)

chris48s · 2018-02-17T12:35:04Z

service-tests/hhvm.js

+
+t.create('invalid package name')
+    .get('/frodo/is-not-a-package.json')
+    .expectJSON({ name: 'hhvm', value: 'invalid' });


The 'invalid' label should be used when there is a problem generating the response (e.g: the json response could not be parsed). There is a handy fixture you can import using const { invalidJSON } = require('./helpers/response-fixtures'); which allows you to mock an invalid response like this

shields/service-tests/cdnjs.js

Lines 28 to 34 in 75503ec

t.create('cdnjs (unexpected response)')

.get('/v/jquery.json')

.intercept(nock => nock('https://api.cdnjs.com')

.get('/libraries/jquery?fields=version')

.reply(invalidJSON)

)

.expectJSON({name: 'cdnjs', value: 'invalid'});

In the case where the package or branch is not found, we should return a 'not found' rather than 'invalid'. It looks like where the repo is not found, php-eye returns a 404: https://php-eye.com/api/v1/package/frodo/not-a-package.json This is the most common case so we have a handler for it checkErrorResponse which can be used like this

shields/server.js

Lines 6157 to 6160 in 75503ec

if (checkErrorResponse(badgeData, err, res, 'repo not found')) {

sendBadge(format, badgeData);

return;

}

In this case, it would also be helpful to have distinct 'repo not found'/'package not found' and 'branch not found' messages.

checkErrorResponse will also handle the 'inaccessible' case, so you can remove the if (err != null) ... block and replace with a block like the one above.

chris48s · 2018-02-17T12:35:40Z

service-tests/hhvm.js

+
+t.create('get symfony status (default branch - not tested)')
+    .get('/symfony/symfony.json')
+    .expectJSONTypes(Joi.object().keys({ name: 'hhvm', value: 'not tested' }));


A bit of project convention: We use .expectJSON() if we're comparing the response to an object literal and .expectJSONTypes() if we're checking the type of the response keys or validating against a regex.

chris48s · 2018-02-17T12:36:49Z

service-tests/hhvm.js

+
+t.create('gets yii 1.1.19 status')
+    .get('/yiisoft/yii/1.1.19.json')
+    .expectJSON({ name: 'hhvm', value: 'partially tested' });


I'm a bit concerned that these tests may be a bit brittle or tied to particular projects. I also initially found the way the service tests are done on this project a bit confusing (there's some good discussion on it in this issue. It can be a bit tricky when your tests depend on external services but the sweet spot we're aiming for is that we want the tests to fail if php-eye change their JSON format or move their endpoint, but the tests shouldn't tell us the service integration is broken if Yii framework decide to change their approach to testing on HHVM, for example.

I'd suggest that the valid test cases (with/without branch) should just check against a regex like /^(tested|partially tested|not tested|maybe untested)$/. If you also want to test the logic that leads to particular responses, you could use a test with a mocked response like this one:

shields/service-tests/homebrew.js

Lines 19 to 25 in 17b4a70

t.create('homebrew (valid, mocked response)')

.get('/v/cake.json')

.intercept(nock => nock('http://formulae.brew.sh')

.get('/formula/cake/version')

.reply(200, {stable: '0.23.0', devel: null, head: null})

)

.expectJSON({name: 'homebrew', value: 'v0.23.0'});

so it is less brittle.

How does that sound?

better tests: including invalidJSON fixture

bkdotcom · 2018-02-18T04:56:21Z

@chris48s thanks for the feedback, I believe I've incorporated all of your suggestions

chris48s

Excellent, thanks very much. I think this is good to merge but one of the maintainers will need to take it over the line.

RedSparr0w · 2018-02-19T21:52:48Z

Thanks for contributing, changes look good to me.
Merged!

Thanks for reviewing @chris48s 😄

badges#1499 - update hhvm service

0451dc1

http://hhvm.h4cc.de... -> https://php-eye.com/api/v1/package...

bkdotcom added 3 commits February 16, 2018 11:05

fix typo

c7e065a

add switch breaks

d915f0c

typo

473b6a3

chris48s reviewed Feb 16, 2018

View reviewed changes

const & let vs var

403ed79

bkdotcom changed the title ~~#1499 - update hhvm service~~ [hhvm] #1499 - update hhvm service Feb 16, 2018

bkdotcom added 3 commits February 16, 2018 16:01

hhvm service tests

d515421

unused const/var

2e417aa

alias master & dev-master

ea79b22

chris48s reviewed Feb 17, 2018

View reviewed changes

improved status messages - repo not found, branch not found

fcf8421

better tests: including invalidJSON fixture

chris48s approved these changes Feb 18, 2018

View reviewed changes

RedSparr0w merged commit ae6dd71 into badges:master Feb 19, 2018

PyvesB mentioned this pull request Nov 18, 2018

Move [hhvm] tests to right folder #2351

Merged

This was referenced Apr 26, 2021

[Snyk] Security upgrade redis from 2.6.5 to 3.1.1 dineshvgp/shields#15

Open

[Snyk] Security upgrade redis from 2.6.5 to 3.1.1 mkg20001/shields#11

Open

[Snyk] Security upgrade redis from 2.8.0 to 3.1.1 Richienb/shields#15

Closed

Richienb mentioned this pull request Oct 5, 2022

[Snyk] Fix for 8 vulnerabilities Richienb/shields#23

Closed

snyk-bot mentioned this pull request Oct 6, 2022

[Snyk] Fix for 8 vulnerabilities Richienb/shields#24

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[hhvm] #1499 - update hhvm service #1508

[hhvm] #1499 - update hhvm service #1508

bkdotcom commented Feb 16, 2018

shields-ci commented Feb 16, 2018 •

edited

Loading

chris48s commented Feb 16, 2018

chris48s left a comment

chris48s Feb 16, 2018

chris48s Feb 16, 2018

bkdotcom Feb 16, 2018

chris48s Feb 16, 2018

bkdotcom commented Feb 16, 2018

chris48s left a comment

chris48s Feb 17, 2018

chris48s Feb 17, 2018

chris48s Feb 17, 2018

bkdotcom commented Feb 18, 2018

chris48s left a comment

RedSparr0w commented Feb 19, 2018

	t.create('cdnjs (unexpected response)')
	.get('/v/jquery.json')
	.intercept(nock => nock('https://api.cdnjs.com')
	.get('/libraries/jquery?fields=version')
	.reply(invalidJSON)
	)
	.expectJSON({name: 'cdnjs', value: 'invalid'});

	if (checkErrorResponse(badgeData, err, res, 'repo not found')) {
	sendBadge(format, badgeData);
	return;
	}

	t.create('homebrew (valid, mocked response)')
	.get('/v/cake.json')
	.intercept(nock => nock('http://formulae.brew.sh')
	.get('/formula/cake/version')
	.reply(200, {stable: '0.23.0', devel: null, head: null})
	)
	.expectJSON({name: 'homebrew', value: 'v0.23.0'});

[hhvm] #1499 - update hhvm service #1508

[hhvm] #1499 - update hhvm service #1508

Conversation

bkdotcom commented Feb 16, 2018

shields-ci commented Feb 16, 2018 • edited Loading

chris48s commented Feb 16, 2018

chris48s left a comment

Choose a reason for hiding this comment

chris48s Feb 16, 2018

Choose a reason for hiding this comment

chris48s Feb 16, 2018

Choose a reason for hiding this comment

bkdotcom Feb 16, 2018

Choose a reason for hiding this comment

chris48s Feb 16, 2018

Choose a reason for hiding this comment

bkdotcom commented Feb 16, 2018

chris48s left a comment

Choose a reason for hiding this comment

chris48s Feb 17, 2018

Choose a reason for hiding this comment

chris48s Feb 17, 2018

Choose a reason for hiding this comment

chris48s Feb 17, 2018

Choose a reason for hiding this comment

bkdotcom commented Feb 18, 2018

chris48s left a comment

Choose a reason for hiding this comment

RedSparr0w commented Feb 19, 2018

shields-ci commented Feb 16, 2018 •

edited

Loading