Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added basic support for [hockeyapp] #2055

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 83 additions & 0 deletions services/hockeyapp/hockeyapp.service.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
'use strict'

const Joi = require('joi')
const { renderVersionBadge } = require('../../lib/version')
const BaseJsonService = require('../base-json')
const { NotFound } = require('../errors')

const hockeyappSchema = Joi.object({
app_versions: Joi.array().items(
Joi.object({
version: Joi.string().required(),
shortversion: Joi.string().required(),
})
),
}).required()

module.exports = class Hockeyapp extends BaseJsonService {
async fetch({ apptoken, appid }) {
const url = `https://rink.hockeyapp.net/api/2/apps/${appid}/app_versions`
const options = { headers: { 'X-HockeyAppToken': apptoken } }
return this._requestJson({
url,
schema: hockeyappSchema,
options: options,
})
}

static render({ version }) {
return renderVersionBadge({ version })
}

async handle({ apptoken, appid }) {
const json = await this.fetch({ apptoken, appid })

if (json.app_versions === undefined || json.app_versions.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your schema says that the app_versions key must be an array.
How does the situation where json.app_versions === undefined occur?
Also, given there are some custom criteria here for the 'not found' response, can we have service tests covering these cases please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The undefined check is just a sanity check but as it will be checked by Joi anyways I will remove the undefined check and rely on that. Also adding the test case you mentioned.

throw new NotFound()
}

const latestVersionObject = json.app_versions[0]

let version

// need this check because hockeyapp handles build numbers differntly for iOS and Android
if (
!latestVersionObject.version.includes(latestVersionObject.shortversion)
) {
version = `${latestVersionObject.shortversion}.${
latestVersionObject.version
}`
} else {
version = latestVersionObject.version
}

return this.constructor.render({ version })
}

// Metadata
static get defaultBadgeData() {
return { label: 'hockeyapp' }
}

static get category() {
return 'version'
}

static get url() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this signature should now be static get route()

return {
base: 'hockeyapp/v',
format: '(.+)/(.+)',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can switch this to use pattern now: remove the format and capture keys and replace them with:

pattern: ':apioken+/:appid+'

that's assuming those two params both need the full (.+) capture group

capture: ['apptoken', 'appid'],
}
}

static get examples() {
return [
{
exampleUrl: 'hockeyapp/simple',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've set up a staging deploy for this at https://shields-staging-pr-2055.herokuapp.com/#/ and this example give me an invalid message: https://shields-staging-pr-2055.herokuapp.com/hockeyapp/v/hockeyapp/simple.svg Can you take a look. The example should return a valid response

Copy link
Contributor Author

@TobiasRoeddiger TobiasRoeddiger Sep 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue here as with the test case that hits the live api - which account should I use for this example?

urlPattern: ':apptoken/:appid',
staticExample: this.render({ version: '1.0' }),
},
]
}
}
44 changes: 44 additions & 0 deletions services/hockeyapp/hockeyapp.tester.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
'use strict'

const createServiceTester = require('../create-service-tester')

const { isVPlusDottedVersionAtLeastOne } = require('../test-validators')

const t = createServiceTester()
module.exports = t

t.create('Invalid AppToken or AppId')
.get('hockeyapp/v/1234/1234.json')
.expectJSON({ name: 'hockeyapp', value: 'invalid' })

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have at least one valid case test in the suite which hits the live API and doesn't mock the response. I realise it is unusual practice for unit tests to depend on an external service ("don't test what you don't own") but we integrate with hundreds of services. If an upstream API changes or goes away, we need a test to start failing on the daily build otherwise we won't notice it.

Copy link
Contributor Author

@TobiasRoeddiger TobiasRoeddiger Sep 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally understand that but we would have to rely on an app token that has to be obtained with a real account at https://hockeyapp.net - how do you handle this situation?

t.create('Android App')
.get('hockeyapp/v/15011995/30071996.json')
.intercept(nock =>
nock('https://rink.hockeyapp.net')
.get('/api/2/apps/30071996/app_versions')
.reply(200, {
app_versions: [
{
version: '24',
shortversion: '1.0',
},
],
})
)
.expectJSON({ name: 'hockeyapp', value: isVPlusDottedVersionAtLeastOne })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify my previous comment:

We use .expectJSON() when comparing to an object literal e.g: .expectJSON({ name: 'hockeyapp', value: 'invalid' }) but .expectJSONTypes() is necessary when we're validating against an expression, so these ones still need to be .expectJSONTypes(). You'll probably find these are failing.

Sorry this is a bit fiddly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying. Will fix this.


t.create('iOS App')
.get('hockeyapp/v/15011995/30071996.json')
.intercept(nock =>
nock('https://rink.hockeyapp.net')
.get('/api/2/apps/30071996/app_versions')
.reply(200, {
app_versions: [
{
version: '1.0.24',
shortversion: '1.0',
},
],
})
)
.expectJSON({ name: 'hockeyapp', value: isVPlusDottedVersionAtLeastOne })