Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {
getPackageJson,
getSnapManifest,
} from '@metamask/snaps-utils/test-utils';
import type { SemVerVersion } from '@metamask/utils';
import normalFs from 'fs';
import { dirname } from 'path';
import type { Configuration } from 'webpack';
Expand Down Expand Up @@ -58,7 +57,7 @@ describe('build', () => {
beforeEach(async () => {
const { manifest } = await getMockSnapFilesWithUpdatedChecksum({
manifest: getSnapManifest({
platformVersion: getPlatformVersion() as SemVerVersion,
platformVersion: getPlatformVersion(),
}),
});

Expand Down
46 changes: 44 additions & 2 deletions packages/snaps-utils/src/manifest/manifest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@ import {
import { NpmSnapFileNames } from '../types';

jest.mock('fs');
jest.mock('../fs', () => ({
...jest.requireActual('../fs'),
useFileSystemCache:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This just disables the file system cache. We mock network requests anyway, and doing this makes testing a bit easier.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does this make easier?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I couldn't figure out how to test the new invalid platform version warning showing up without changing this. It would always cache the correct version.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure I understand how this changes that, the mock returns the platform version from getPlatformVersion anyways 🤔

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The test replaces the fetch mock.

fetchMock.mockResponseOnce(MOCK_GITHUB_RESPONSE).mockResponseOnce(
  JSON.stringify({
    dependencies: {
      '@metamask/snaps-sdk': '1.0.0',
    },
  }),
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe try if clearing it out in resetFileSystem fixes it? Otherwise fine to merge as-is

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The cache seems to be written to the real file system, or at least I can't find where it is in the virtual file system 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would expect something like this:
await fs.rm('./node_modules/.cache/snaps', { recursive: true, force: true });

Does that not work?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It doesn't, I'm not seeing the node_modules/.cache folder anywhere in the VFS

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Strange 🤔

<Type>(_key: string, _ttl: number, fn: () => Promise<Type>) =>
async () =>
fn(),
}));

const BASE_PATH = '/snap';
const MANIFEST_PATH = join(BASE_PATH, NpmSnapFileNames.Manifest);
Expand Down Expand Up @@ -157,6 +164,39 @@ describe('checkManifest', () => {
expect(version).toBe('1.0.0');
});

it('includes new validation warnings', async () => {
Comment thread
Mrtenz marked this conversation as resolved.
fetchMock.mockResponseOnce(MOCK_GITHUB_RESPONSE).mockResponseOnce(
JSON.stringify({
dependencies: {
'@metamask/snaps-sdk': '1.0.0',
},
}),
);

const manifest = getSnapManifest({
shasum: '29MYwcRiruhy9BEJpN/TBIhxoD3t0P4OdXztV9rW8tc=',
});
delete manifest.platformVersion;

await fs.writeFile(MANIFEST_PATH, JSON.stringify(manifest));

const { files, updated, reports } = await checkManifest(BASE_PATH);
const unfixed = reports.filter((report) => !report.wasFixed);
const fixed = reports.filter((report) => report.wasFixed);

const defaultManifest = await getDefaultManifest();

expect(files?.manifest.result).toStrictEqual(defaultManifest);
expect(updated).toBe(true);
expect(unfixed).toHaveLength(1);
expect(fixed).toHaveLength(2);

const file = await readJsonFile<SnapManifest>(MANIFEST_PATH);
const { source, version } = file.result;
expect(source.shasum).toBe(defaultManifest.source.shasum);
expect(version).toBe('1.0.0');
});

it('returns a warning if package.json is missing recommended fields', async () => {
const { repository, ...packageJson } = getPackageJson();

Expand Down Expand Up @@ -348,7 +388,7 @@ describe('runFixes', () => {
const rule: ValidatorMeta = {
severity: 'error',
semanticCheck(_, context) {
context.report('Always fail', (files) => files);
context.report('always-fail', 'Always fail', (files) => files);
},
};

Expand All @@ -360,7 +400,9 @@ describe('runFixes', () => {
expect(fixesResults).toStrictEqual({
files,
updated: false,
reports: [{ severity: 'error', message: 'Always fail' }],
reports: [
{ id: 'always-fail', severity: 'error', message: 'Always fail' },
],
});
});
});
Expand Down
22 changes: 15 additions & 7 deletions packages/snaps-utils/src/manifest/manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,8 @@ export async function runFixes(
assert(fixResults.files);
fixResults.files.manifest = fixResults.files.manifest.clone();

const mergedReports: ValidatorReport[] = deepClone(fixResults.reports);

for (
let attempts = 1;
shouldRunFixes && attempts <= MAX_ATTEMPTS;
Expand Down Expand Up @@ -259,15 +261,21 @@ export async function runFixes(

fixResults = await runValidators(fixResults.files, rules);
shouldRunFixes = hasFixes(fixResults, errorsOnly);

mergedReports.push(
...fixResults.reports.filter(
Comment thread
Mrtenz marked this conversation as resolved.
Outdated
(report) =>
!mergedReports.find((mergedReport) => mergedReport.id === report.id),
Comment thread
Mrtenz marked this conversation as resolved.
Outdated
),
);
}

const initialReports: (CheckManifestReport & ValidatorReport)[] = deepClone(
results.reports,
);
const allReports: (CheckManifestReport & ValidatorReport)[] =
deepClone(mergedReports);

// Was fixed
if (!shouldRunFixes) {
for (const report of initialReports) {
for (const report of allReports) {
if (report.fix) {
report.wasFixed = true;
delete report.fix;
Expand All @@ -277,18 +285,18 @@ export async function runFixes(
return {
files: fixResults.files,
updated: true,
reports: initialReports,
reports: allReports,
};
}

for (const report of initialReports) {
for (const report of allReports) {
delete report.fix;
}

return {
files: results.files,
updated: false,
reports: initialReports,
reports: allReports,
};
}

Expand Down
6 changes: 4 additions & 2 deletions packages/snaps-utils/src/manifest/validator-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,12 @@ export type ValidatorContextOptions = {
export type ValidatorSeverity = 'error' | 'warning';

export type ValidatorContext = {
readonly report: (message: string, fix?: ValidatorFix) => void;
readonly report: (id: string, message: string, fix?: ValidatorFix) => void;
readonly options?: ValidatorContextOptions;
};

export type ValidatorReport = {
id: string;
severity: ValidatorSeverity;
message: string;
fix?: ValidatorFix;
Expand All @@ -41,7 +42,8 @@ export type ValidatorMeta = {
severity: ValidatorSeverity;

/**
* 1. Run the validator on unverified files to ensure that the files are structurally sound.
* 1. Run the validator on unverified files to ensure that the files are
* structurally sound.
*
* @param files - Files to be verified
* @param context - Validator context to report errors
Expand Down
16 changes: 14 additions & 2 deletions packages/snaps-utils/src/manifest/validator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,22 @@ class Context implements ValidatorContext {
this.#options = options;
}

report(message: string, fix?: ValidatorFix): void {
/**
* Report a validation error or warning.
*
* @param id - The unique identifier for the report.
* @param message - The message describing the validation issue.
* @param fix - An optional fix function that can be used to automatically
* resolve the issue.
*/
report(id: string, message: string, fix?: ValidatorFix): void {
assert(this.#nextSeverity !== undefined);

this.reports.push({
severity: this.#nextSeverity,
id,
message,
fix,
severity: this.#nextSeverity,
});
}

Expand Down Expand Up @@ -80,6 +90,7 @@ export async function runValidators(
context.prepareForValidator({
severity: rule.severity,
});

await rule.structureCheck?.(files, context);
}

Expand All @@ -93,6 +104,7 @@ export async function runValidators(
context.prepareForValidator({
severity: rule.severity,
});

await rule.semanticCheck?.(files as SnapFiles, context);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ describe('checksum', () => {

expect(report).toHaveBeenCalledTimes(1);
expect(report).toHaveBeenCalledWith(
'checksum',
expect.stringContaining(
'"snap.manifest.json" "shasum" field does not match computed shasum.',
),
Expand All @@ -38,7 +39,7 @@ describe('checksum', () => {
const manifest = getSnapManifest({ shasum: 'foobar' });

let fix: ValidatorFix | undefined;
const report = (_: any, fixer?: ValidatorFix) => {
const report = (_id: string, _message: string, fixer?: ValidatorFix) => {
assert(fixer !== undefined);
fix = fixer;
};
Expand Down
1 change: 1 addition & 0 deletions packages/snaps-utils/src/manifest/validators/checksum.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export const checksum: ValidatorMeta = {
const expectedChecksum = await getSnapChecksum(fetchedFiles);
if (gotChecksum !== expectedChecksum) {
context.report(
'checksum',
`"${NpmSnapFileNames.Manifest}" "shasum" field does not match computed shasum. Got "${gotChecksum}", expected "${expectedChecksum}".`,
async ({ manifest }) => {
manifest.source.shasum = expectedChecksum;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ describe('expectedFiles', () => {

expect(report).toHaveBeenCalledTimes(1);
expect(report).toHaveBeenCalledWith(
'expected-files',
expect.stringContaining('Missing file'),
);
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ export const expectedFiles: ValidatorMeta = {
structureCheck(files, context) {
for (const expectedFile of EXPECTED_SNAP_FILES) {
if (!files[expectedFile]) {
context.report(`Missing file "${SnapFileNameFromKey[expectedFile]}".`);
context.report(
'expected-files',
Comment thread
Mrtenz marked this conversation as resolved.
Outdated
`Missing file "${SnapFileNameFromKey[expectedFile]}".`,
);
Comment thread
cursor[bot] marked this conversation as resolved.
}
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ describe('iconDeclared', () => {
});

expect(report).toHaveBeenCalledWith(
'icon-declared',
'No icon found in the Snap manifest. It is recommended to include an icon for the Snap. See https://docs.metamask.io/snaps/how-to/design-a-snap/#guidelines-at-a-glance for more information.',
);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ export const iconDeclared: ValidatorMeta = {
semanticCheck(files, context) {
if (!files.manifest.result.source.location.npm.iconPath) {
context.report(
'icon-declared',
'No icon found in the Snap manifest. It is recommended to include an icon for the Snap. See https://docs.metamask.io/snaps/how-to/design-a-snap/#guidelines-at-a-glance for more information.',
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ describe('iconDimensions', () => {
});

expect(report).toHaveBeenCalledWith(
'icon-dimensions',
'The icon in the Snap manifest is not square. It is recommended to use a square icon for the Snap.',
);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export const iconDimensions: ValidatorMeta = {
const dimensions = getSvgDimensions(files.svgIcon.toString());
if (dimensions && dimensions?.height !== dimensions.width) {
context.report(
'icon-dimensions',
'The icon in the Snap manifest is not square. It is recommended to use a square icon for the Snap.',
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ describe('iconMissing', () => {
await iconMissing.semanticCheck(files, { report });

expect(report).toHaveBeenCalledWith(
'icon-missing',
'Could not find icon "images/icon.svg".',
);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export const iconMissing: ValidatorMeta = {
semanticCheck(files, context) {
const { iconPath } = files.manifest.result.source.location.npm;
if (iconPath && !files.svgIcon) {
context.report(`Could not find icon "${iconPath}".`);
context.report('icon-missing', `Could not find icon "${iconPath}".`);
}
},
};
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ describe('isLocalizationFile', () => {
);

expect(report).toHaveBeenCalledWith(
'is-localization-file',
'Failed to validate localization file "/foo": 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 @@ -16,6 +16,7 @@ export const isLocalizationFile: ValidatorMeta = {
if (error) {
for (const failure of error.failures()) {
context.report(
'is-localization-file',
Comment thread
cursor[bot] marked this conversation as resolved.
Outdated
Comment thread
Mrtenz marked this conversation as resolved.
Outdated
`Failed to validate localization file "${
file.path
}": ${getStructFailureMessage(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ describe('isPackageJson', () => {
);

expect(report).toHaveBeenCalledWith(
'is-package-json',
'"package.json" is invalid: At path: version — Expected SemVer version, got "foo".',
);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export const isPackageJson: ValidatorMeta = {
if (error) {
for (const failure of error.failures()) {
context.report(
'is-package-json',
Comment thread
Mrtenz marked this conversation as resolved.
Outdated
`"${
NpmSnapFileNames.PackageJson
}" is invalid: ${getStructFailureMessage(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ describe('isSnapIcon', () => {
{ report },
);

expect(report).toHaveBeenCalledWith('Snap icon must be a valid SVG.');
expect(report).toHaveBeenCalledWith(
'is-snap-icon',
'Snap icon must be a valid SVG.',
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export const isSnapIcon: ValidatorMeta = {
assertIsSnapIcon(files.svgIcon);
} catch (error) {
assert(error instanceof Error);
context.report(error.message);
context.report('is-snap-icon', error.message);
}
},
};
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ describe('isSnapManifest', () => {
);

expect(report).toHaveBeenCalledWith(
'is-snap-manifest',
'"snap.manifest.json" is invalid: Expected a value of type object, but received: "foo".',
);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export const isSnapManifest: ValidatorMeta = {
if (error) {
for (const failure of error.failures()) {
context.report(
'is-snap-manifest',
Comment thread
Mrtenz marked this conversation as resolved.
Outdated
`"${NpmSnapFileNames.Manifest}" is invalid: ${getStructFailureMessage(
SnapManifestStruct,
failure,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ describe('manifestLocalization', () => {
await manifestLocalization.semanticCheck(files, { report });

expect(report).toHaveBeenCalledWith(
'manifest-localization',
'Failed to localize Snap manifest: Failed to translate "{{ name }}": No translation found for "name" in "en" file.',
);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export const manifestLocalization: ValidatorMeta = {
getLocalizedSnapManifest(manifest, locale, localizations);
} catch (error) {
context.report(
'manifest-localization',
`Failed to localize Snap manifest: ${getErrorMessage(error)}`,
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ describe('packageJsonRecommendedFields', () => {
await packageJsonRecommendedFields.semanticCheck(files, { report });

expect(report).toHaveBeenCalledWith(
'package-json-recommended-fields',
'Missing recommended package.json property: "repository".',
);
});
Expand Down
Loading
Loading