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

njakob · 2023-02-25T22:24:00Z

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: opengl
version-semver: nlohmann-json
version-string: 7zip
version: entt

github-actions · 2023-02-25T22:25:15Z

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

Generated by 🚫 dangerJS against 7534498

services/vcpkg/vcpkg-version.tester.js

njakob · 2023-02-25T22:31:05Z

services/vcpkg/vcpkg-version.tester.js

  .get('/entt.json')
-  .expectBadge({ label: 'vcpkg', message: isSemver })
+  .expectBadge({ label: 'vcpkg', color: 'blue', message: Joi.string() })


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.

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.

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.

chris48s · 2023-02-26T16:17:42Z

services/vcpkg/vcpkg-version.service.js

@@ -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()


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

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

chris48s · 2023-02-26T16:17:49Z

services/vcpkg/vcpkg-version.service.js

-const vcpkgManifestSchema = Joi.object({
-  version: Joi.string().required(),
-}).required()
+const vcpkgManifestSchema = Joi.alternatives().try(


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.

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

chris48s · 2023-02-26T16:19:03Z

services/vcpkg/vcpkg-version.tester.js

  .get('/entt.json')
-  .expectBadge({ label: 'vcpkg', message: isSemver })
+  .expectBadge({ label: 'vcpkg', color: 'blue', message: Joi.string() })


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.

njakob · 2023-02-27T21:22:57Z

I'm not completely sure why some checks are failing after the latest changes as it seems unrelated. So you have an idea @chris48s?

chris48s · 2023-03-01T20:19:01Z

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 npm run test:services -- --only="vcpkg"

njakob · 2023-03-01T22:28:23Z

services/vcpkg/vcpkg-version-helpers.js

+import { InvalidResponse } from '../index.js'
+
+export function parseVersionFromVcpkgManifest(manifest) {
+  if (manifest['version-date']) {


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.

njakob · 2023-03-01T22:33:31Z

I went into a slightly different direction of unit testing the parsing helper @chris48s, now all the (new) tests are passing!

Fix [Vcpkg] version service for different version fields

40c2ae7

njakob commented Feb 25, 2023

View reviewed changes

services/vcpkg/vcpkg-version.tester.js Outdated Show resolved Hide resolved

njakob commented Feb 25, 2023

View reviewed changes

chris48s reviewed Feb 26, 2023

View reviewed changes

njakob and others added 3 commits February 27, 2023 22:02

Use match one to enforce a single alternative

79d48e6

Mock different version field test cases

23a169a

Merge branch 'badges:master' into fix-vcpkg-version

55e959a

njakob requested a review from chris48s February 28, 2023 08:13

njakob and others added 2 commits March 1, 2023 21:39

Merge branch 'badges:master' into fix-vcpkg-version

404c373

Extract version field parsing as separate helper

7534498

njakob commented Mar 1, 2023

View reviewed changes

chris48s approved these changes Mar 4, 2023

View reviewed changes

chris48s added service-badge Accepted and actionable changes, features, and bugs squash when passing labels Mar 4, 2023

Merge branch 'master' into fix-vcpkg-version

a8ca33c

repo-ranger bot merged commit 4a5bf53 into badges:master Mar 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

njakob commented Feb 25, 2023

github-actions bot commented Feb 25, 2023 •

edited

Loading

njakob Feb 25, 2023

chris48s Feb 26, 2023

njakob Mar 1, 2023 •

edited

Loading

chris48s Feb 26, 2023

njakob Feb 27, 2023 •

edited

Loading

chris48s Feb 26, 2023

njakob Feb 27, 2023 •

edited

Loading

chris48s Feb 26, 2023

njakob commented Feb 27, 2023

chris48s commented Mar 1, 2023

njakob Mar 1, 2023 •

edited

Loading

njakob commented Mar 1, 2023

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

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

Conversation

njakob commented Feb 25, 2023

github-actions bot commented Feb 25, 2023 • edited Loading

njakob Feb 25, 2023

Choose a reason for hiding this comment

chris48s Feb 26, 2023

Choose a reason for hiding this comment

njakob Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

chris48s Feb 26, 2023

Choose a reason for hiding this comment

njakob Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

chris48s Feb 26, 2023

Choose a reason for hiding this comment

njakob Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

chris48s Feb 26, 2023

Choose a reason for hiding this comment

njakob commented Feb 27, 2023

chris48s commented Mar 1, 2023

njakob Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

njakob commented Mar 1, 2023

github-actions bot commented Feb 25, 2023 •

edited

Loading

njakob Mar 1, 2023 •

edited

Loading

njakob Feb 27, 2023 •

edited

Loading

njakob Feb 27, 2023 •

edited

Loading

njakob Mar 1, 2023 •

edited

Loading