Fix [Vcpkg] version service for different version fields #8945
Conversation
| .get('/entt.json') | ||
| .expectBadge({ label: 'vcpkg', message: isSemver }) | ||
| .expectBadge({ label: 'vcpkg', color: 'blue', message: Joi.string() }) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.jsfiles inservices/for examples) or - using mocked tests for each detail case (have a look for
*.tester.jsthat include.interceptfiles inservices/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.
There was a problem hiding this comment.
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.
| @@ -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() | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
That's a good idea to directly link the documentation, I added it directly as a comment!
| const vcpkgManifestSchema = Joi.object({ | ||
| version: Joi.string().required(), | ||
| }).required() | ||
| const vcpkgManifestSchema = Joi.alternatives().try( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I wasn't aware of this distinction, that's great you picked it up. I adapted the schema accordingly!
| .get('/entt.json') | ||
| .expectBadge({ label: 'vcpkg', message: isSemver }) | ||
| .expectBadge({ label: 'vcpkg', color: 'blue', message: Joi.string() }) |
There was a problem hiding this comment.
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.jsfiles inservices/for examples) or - using mocked tests for each detail case (have a look for
*.tester.jsthat include.interceptfiles inservices/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.
|
I'm not completely sure why some checks are failing after the latest changes as it seems unrelated. So you have an idea @chris48s? |
|
The docker and package test failures were transient. I restarted them. The service test failures are legit. Are you able to see the summary report on https://github.com/badges/shields/actions/runs/4286928999 ? "Mocks not yet satisfied" normally means there is an issue with the mock setup and the outbound API request we're making is not matching any mocked request. You should see the same failures locally if you run |
| import { InvalidResponse } from '../index.js' | ||
|
|
||
| export function parseVersionFromVcpkgManifest(manifest) { | ||
| if (manifest['version-date']) { |
There was a problem hiding this comment.
note: It would have been better to also validate the schema directly inside the helper. However, this would require working around the current validation API in place through the service class and directly using validate or the Joi schema validation.
However, looking at other usages of fetchJsonFromRepo, I left the schema validation outside of this parsing helper.
|
I went into a slightly different direction of unit testing the parsing helper @chris48s, now all the (new) tests are passing! |
This PR fixes the missing cases for other version fields that can be present in Vcpkg manifest which were not considered in the initial implementation (#8923).
This includes the following cases (see documentation):
version-date:openglversion-semver:nlohmann-jsonversion-string:7zipversion:entt