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

Improve errors in checkManifest #2605

Merged
merged 5 commits into from
Jul 29, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@
"@types/node": "18.14.2",
"@typescript-eslint/eslint-plugin": "^5.42.1",
"@typescript-eslint/parser": "^5.42.1",
"chromedriver": "^125.0.1",
"chromedriver": "^127.0.0",
"depcheck": "^1.4.7",
"eslint": "^8.27.0",
"eslint-config-prettier": "^8.5.0",
Expand Down
2 changes: 1 addition & 1 deletion packages/snaps-utils/coverage.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@
"branches": 99.73,
"functions": 98.9,
"lines": 99.43,
"statements": 96.31
"statements": 96.32
}
2 changes: 1 addition & 1 deletion packages/snaps-utils/src/manifest/manifest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ describe('checkManifest', () => {

const { reports } = await checkManifest(BASE_PATH);
expect(reports.map(({ message }) => message)).toStrictEqual([
'Failed to validate localization file "/snap/locales/en.json": At path: messages -- Expected an object, but received: "foo".',
'Failed to validate localization file "/snap/locales/en.json": At path: messages Expected a value of type record, but received: "foo".',
]);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ describe('isLocalizationFile', () => {
);

expect(report).toHaveBeenCalledWith(
'Failed to validate localization file "/foo": At path: messages -- Expected an object, but received: "foo".',
'Failed to validate localization file "/foo": At path: messages Expected a value of type record, but received: "foo".',
);
});
});
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { validate } from '@metamask/superstruct';

import { LocalizationFileStruct } from '../../localization';
import { getStructFailureMessage } from '../../structs';
import type { ValidatorMeta } from '../validator-types';

/**
Expand All @@ -13,9 +14,17 @@ export const isLocalizationFile: ValidatorMeta = {
const [error] = validate(file.result, LocalizationFileStruct);

if (error) {
context.report(
`Failed to validate localization file "${file.path}": ${error.message}.`,
);
for (const failure of error.failures()) {
context.report(
`Failed to validate localization file "${
file.path
}": ${getStructFailureMessage(
LocalizationFileStruct,
failure,
false,
)}`,
);
}
}
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ describe('isPackageJson', () => {
);

expect(report).toHaveBeenCalledWith(
'"package.json" is invalid: Expected SemVer version, got "foo"',
'"package.json" is invalid: At path: version — Expected SemVer version, got "foo".',
);
});
});
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { validate } from '@metamask/superstruct';

import { getStructFailureMessage } from '../../structs';
import { NpmSnapFileNames, NpmSnapPackageJsonStruct } from '../../types';
import type { ValidatorMeta } from '../validator-types';

Expand All @@ -19,7 +20,13 @@ export const isPackageJson: ValidatorMeta = {
if (error) {
for (const failure of error.failures()) {
context.report(
`"${NpmSnapFileNames.PackageJson}" is invalid: ${failure.message}`,
`"${
NpmSnapFileNames.PackageJson
}" is invalid: ${getStructFailureMessage(
NpmSnapPackageJsonStruct,
failure,
false,
)}`,
);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ describe('isSnapManifest', () => {
);

expect(report).toHaveBeenCalledWith(
'"snap.manifest.json" is invalid: Expected an object, but received: "foo"',
'"snap.manifest.json" is invalid: Expected a value of type object, but received: "foo".',
);
});
});
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { validate } from '@metamask/superstruct';

import { getStructFailureMessage } from '../../structs';
import { NpmSnapFileNames } from '../../types';
import { SnapManifestStruct } from '../validation';
import type { ValidatorMeta } from '../validator-types';
Expand All @@ -17,7 +18,11 @@ export const isSnapManifest: ValidatorMeta = {
if (error) {
for (const failure of error.failures()) {
context.report(
`"${NpmSnapFileNames.Manifest}" is invalid: ${failure.message}`,
`"${NpmSnapFileNames.Manifest}" is invalid: ${getStructFailureMessage(
SnapManifestStruct,
failure,
false,
)}`,
);
}
}
Expand Down
5 changes: 5 additions & 0 deletions packages/snaps-utils/src/structs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,11 @@ export function getStructFailureMessage<Type, Schema>(
return `${prefix}${message}.`;
}

// Refinements we built ourselves have nice error messages
if (failure.refinement !== undefined) {
return `${prefix}${failure.message}.`;
}

return `${prefix}Expected a value of type ${color(
failure.type,
green,
Expand Down
21 changes: 12 additions & 9 deletions packages/snaps-webpack-plugin/src/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -252,15 +252,18 @@ describe('SnapsWebpackPlugin', () => {
],
});

await expect(
bundle({
options: {
eval: false,
manifestPath: '/snap.manifest.json',
writeManifest: false,
},
}),
).rejects.toThrow('Manifest Error: The manifest is invalid.\nfoo\nbar');
const { stats } = await bundle({
options: {
eval: false,
manifestPath: '/snap.manifest.json',
writeManifest: false,
},
});

// eslint-disable-next-line jest/prefer-strict-equal
expect(stats.compilation.errors.map((error) => error.message)).toEqual(
expect.arrayContaining(['foo', 'bar']),
);
});

it('logs manifest warnings', async () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/snaps-webpack-plugin/src/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,8 @@ export default class SnapsWebpackPlugin {
.map((report) => report.message);

if (errors.length > 0) {
throw new Error(
`Manifest Error: The manifest is invalid.\n${errors.join('\n')}`,
compilation.errors.push(
...errors.map((error) => new WebpackError(error)),
);
}

Expand Down
4 changes: 2 additions & 2 deletions scripts/install-chrome.sh
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ set -u
set -o pipefail

# To get the latest version, see <https://www.ubuntuupdates.org/ppa/google_chrome?dist=stable>
CHROME_VERSION='125.0.6422.76-1'
CHROME_VERSION='127.0.6533.72-1'
CHROME_BINARY="google-chrome-stable_${CHROME_VERSION}_amd64.deb"
CHROME_BINARY_URL="https://dl.google.com/linux/chrome/deb/pool/main/g/google-chrome-stable/${CHROME_BINARY}"

# To retrieve this checksum, run the `wget` and `shasum` commands below
CHROME_BINARY_SHA512SUM='0c221bca2bfaf198018f8d1649da2ae3120e3a3e27dcf9c16170a6b05302728d28caf8af172bdd6e34b56d3b6cc7769b4a17def250c92a569871565d167dc866'
CHROME_BINARY_SHA512SUM='54097b33fbe8bf485273f25446b5da36fdae1b4a3d1722f3b245b63f7337f5acdae1788000ca7b6817e8197c675785f2c94e487426126c734e7ca725affb7f2a'

wget -O "${CHROME_BINARY}" -t 5 "${CHROME_BINARY_URL}"

Expand Down
10 changes: 5 additions & 5 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -10440,9 +10440,9 @@ __metadata:
languageName: node
linkType: hard

"chromedriver@npm:^125.0.1":
version: 125.0.1
resolution: "chromedriver@npm:125.0.1"
"chromedriver@npm:^127.0.0":
version: 127.0.0
resolution: "chromedriver@npm:127.0.0"
dependencies:
"@testim/chrome-version": ^1.1.4
axios: ^1.6.7
Expand All @@ -10453,7 +10453,7 @@ __metadata:
tcp-port-used: ^1.0.2
bin:
chromedriver: bin/chromedriver
checksum: 755a866f19f64f4d2599844971566fe1c79659260f266ab53b8c943bfe0517fd9d1435e6c5dc7146c5266afcf7d11f748b5b981940cbcc5511856cba1996d292
checksum: f860fe6671aefd4c08c33e045340821e69cad8546790f4def69fedf2be0c8c27458d0a75f52a6394c06478f581fcad66e7e453f6d8dd352e6915f9a9e9b6e721
languageName: node
linkType: hard

Expand Down Expand Up @@ -20306,7 +20306,7 @@ __metadata:
"@types/node": 18.14.2
"@typescript-eslint/eslint-plugin": ^5.42.1
"@typescript-eslint/parser": ^5.42.1
chromedriver: ^125.0.1
chromedriver: ^127.0.0
depcheck: ^1.4.7
eslint: ^8.27.0
eslint-config-prettier: ^8.5.0
Expand Down
Loading