support using release or tag name in [GitHub] Release version badge #7075
Conversation
|
chris48s
changed the title
| @@ -20,6 +20,10 @@ t.create('Prerelease') | |||
| color: Joi.string().allow('blue', 'orange').required(), | |||
| }) | |||
|
|
|||
| t.create('Release (release name instead of tag name)') | |||
| .get('/v/release/expressjs/express.json?display_name=release') | |||
| .expectBadge({ label: 'release', message: isSemver, color: 'blue' }) | |||
There was a problem hiding this comment.
I can see we've got the transform logic already under test in the .spec file. This test is kinda odd though because we're running it on a repo that doesn't use named releases so all we're really testing is that display_name=release is a valid param. It seems like a test I'll read in a year's time and be confused by anyway :) We could switch this to testing against a repo that uses named releases, although it is possibly then brittle in future. Another option would be to mock it e.g:
t.create('Release (custom release name)')
.get('/v/release/expressjs/express.json')
.intercept(nock =>
nock('https://api.github.com')
.get('/repos/expressjs/express/releases/latest')
.reply(200, {
assets: [],
name: 'custom release name',
tag_name: '1.0.0',
prerelease: false,
})
)
.expectBadge({
label: 'release',
message: 'custom release name',
color: 'blue',
})There was a problem hiding this comment.
so all we're really testing is that display_name=release is a valid param
Yup, precisely. I'm not opposed to adding another test, but feel like it's a fairly common pattern to tack on a test that simply covers the query param when we've got other unit tests covering the associated logic.
Would you still like to see a new test or perhaps even a comment to the effect of "this is just testing the query param..." etc.?
There was a problem hiding this comment.
Yeah maybe just a comment explaining this would be useful.
There was a problem hiding this comment.
added in 9078a35
Resolves #6763