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.

Sign up for GitHub

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

Fix [Vcpkg] version service for different version fields #8945

Merged
merged 7 commits into from
Mar 4, 2023
Merged

Fix [Vcpkg] version service for different version fields #8945

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
40c2ae7
Fix [Vcpkg] version service for different version fields
njakob Feb 25, 2023
79d48e6
Use match one to enforce a single alternative
njakob Feb 27, 2023
23a169a
Mock different version field test cases
njakob Feb 27, 2023
55e959a
Merge branch 'badges:master' into fix-vcpkg-version
njakob Feb 27, 2023
404c373
Merge branch 'badges:master' into fix-vcpkg-version
njakob Mar 1, 2023
7534498
Extract version field parsing as separate helper
njakob Mar 1, 2023
a8ca33c
Merge branch 'master' into fix-vcpkg-version
repo-ranger[bot] Mar 4, 2023
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
30 changes: 25 additions & 5 deletions services/vcpkg/vcpkg-version.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,20 @@ import { fetchJsonFromRepo } from '../github/github-common-fetch.js'
import { renderVersionBadge } from '../version.js'
import { NotFound } from '../index.js'

const vcpkgManifestSchema = Joi.object({
version: Joi.string().required(),
}).required()
Copy link
Member

Choose a reason for hiding this comment

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

Can add a comment here linking to https://learn.microsoft.com/en-us/vcpkg/reference/vcpkg-json?source=recommendations#version to make it a bit more obvious what is going on here

Copy link
Contributor Author

@njakob njakob Feb 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That's a good idea to directly link the documentation, I added it directly as a comment!

const vcpkgManifestSchema = Joi.alternatives().try(
Copy link
Member

Choose a reason for hiding this comment

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

If the input file contains more than one of these, it is an invalid manifest and we won't know which one to pick. Lets use Joi's 'one' match mode https://joi.dev/api/?v=17.8.1#alternativesmatchmode to fail if more than one exists.

Copy link
Contributor Author

@njakob njakob Feb 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I wasn't aware of this distinction, that's great you picked it up. I adapted the schema accordingly!

Joi.object({
version: Joi.string().required(),
}).required(),
Joi.object({
'version-date': Joi.string().required(),
}).required(),
Joi.object({
'version-semver': Joi.string().required(),
}).required(),
Joi.object({
'version-string': Joi.string().required(),
}).required()
)

export default class VcpkgVersion extends ConditionalGithubAuthV3Service {
static category = 'version'
Expand All @@ -29,14 +40,23 @@ export default class VcpkgVersion extends ConditionalGithubAuthV3Service {

async handle({ portName }) {
try {
const { version } = await fetchJsonFromRepo(this, {
const payload = await fetchJsonFromRepo(this, {
schema: vcpkgManifestSchema,
user: 'microsoft',
repo: 'vcpkg',
branch: 'master',
filename: `ports/${portName}/vcpkg.json`,
})
return this.constructor.render({ version })
if (payload['version-date']) {
return this.constructor.render({ version: payload['version-date'] })
}
if (payload['version-semver']) {
return this.constructor.render({ version: payload['version-semver'] })
}
if (payload['version-string']) {
return this.constructor.render({ version: payload['version-string'] })
}
return this.constructor.render({ version: payload.version })
} catch (error) {
if (error instanceof NotFound) {
throw new NotFound({
Expand Down
18 changes: 15 additions & 3 deletions services/vcpkg/vcpkg-version.tester.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,23 @@
import { isSemver } from '../test-validators.js'
import Joi from 'joi'
import { createServiceTester } from '../tester.js'

export const t = await createServiceTester()

t.create('gets the port version of entt')
t.create('gets the port version of entt using `version` field')
.get('/entt.json')
.expectBadge({ label: 'vcpkg', message: isSemver })
.expectBadge({ label: 'vcpkg', color: 'blue', message: Joi.string() })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: Related to the other comment, in order to avoid having tests failing if the version pattern changes (e.g. date format is changed to semantic versioning), we only test if the version has been properly extracted by looking at the color of the badge.

Copy link
Member

Choose a reason for hiding this comment

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

How about we keep one "live" test and test this level of detail by either:

  • splitting out the logic for choosing a version field into its own function and unit testing it (have a look for *.spec.js files in services/ for examples) or
  • using mocked tests for each detail case (have a look for *.tester.js that include .intercept files in services/ for examples)

If we pick a project that uses the version-semver field and validate with isSemver for the integration test, that should be stable enough.

Copy link
Contributor Author

@njakob njakob Mar 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Since I ran into some issues mocking these tests (it was only working locally), I went for the approach of extracting the version parsing helper.

The remaining live test is checking a port that currently exposes its version through the version-semver as you suggested.


t.create('gets the port version of opengl using `version-date` field')
.get('/opengl.json')
.expectBadge({ label: 'vcpkg', color: 'blue', message: Joi.string() })

t.create('gets the port version of nlohmann-json using `version-semver` field')
.get('/nlohmann-json.json')
.expectBadge({ label: 'vcpkg', color: 'blue', message: Joi.string() })

t.create('gets the port version of 7zip using `version-string` field')
.get('/7zip.json')
.expectBadge({ label: 'vcpkg', color: 'blue', message: Joi.string() })

t.create('returns not found for invalid port')
.get('/this-port-does-not-exist.json')
Expand Down