Merge pull request #1050 from microsoft/benibenj/narrow-smelt

Fix unused-files-patterns check and add tests

Author: benibenj

src/package.ts

@@ -1968,7 +1968,7 @@ export async function ls(options: ILSOptions = {}): Promise<void> {
 	if (options.tree) {
 		const printableFileStructure = await util.generateFileStructureTree(
 			getDefaultPackageName(manifest, options),
-			files.map(f => ({ origin: f, tree: f }))
+			files.map(f => ({ origin: path.join(cwd, f), tree: f }))
 		);
 		console.log(printableFileStructure.join('\n'));
 	} else {
@@ -2011,8 +2011,23 @@ export async function printAndValidatePackagedFiles(files: IFile[], cwd: string,
 	// Throw an error if the extension uses the files property in package.json and 
 	// the package does not include at least one file for each include pattern
 	else if (manifest.files !== undefined && manifest.files.length > 0 && !options.allowUnusedFilesPattern) {
-		const originalFilePaths = files.map(f => util.vsixPathToFilePath(f.path));
-		const unusedIncludePatterns = manifest.files.filter(includePattern => !originalFilePaths.some(filePath => minimatch(filePath, includePattern, MinimatchOptions)));
+		const localPaths = (files.filter(f => !isInMemoryFile(f)) as ILocalFile[]).map(f => util.normalize(f.localPath));
+		const filesIncludePatterns = manifest.files.map(includePattern => util.normalize(path.join(cwd, includePattern)));
+
+		const unusedIncludePatterns = filesIncludePatterns.filter(includePattern => {
+			// Check if the pattern provided by the user matches any file in the package
+			if (localPaths.some(localFilePath => minimatch(localFilePath, includePattern, MinimatchOptions))) {
+				return false;
+			}
+			// Check if the pattern provided by the user matches any folder in the package
+			if (!/(^|\/)[^/]*\*[^/]*$/.test(includePattern)) {
+				includePattern = (/\/$/.test(includePattern) ? `${includePattern}**` : `${includePattern}/**`);
+				return !localPaths.some(localFilePath => minimatch(localFilePath, includePattern, MinimatchOptions));
+			}
+			// Pattern does not match any file or folder
+			return true;
+		});
+
 		if (unusedIncludePatterns.length > 0) {
 			let message = '';
 			message += `The following include patterns in the ${chalk.bold('"files"')} property in package.json do not match any files packaged in the extension:\n`;
@@ -2030,7 +2045,7 @@ export async function printAndValidatePackagedFiles(files: IFile[], cwd: string,
 		getDefaultPackageName(manifest, options),
 		files.map(f => ({
 			// File path relative to the extension root
-			origin: util.vsixPathToFilePath(f.path),
+			origin: !isInMemoryFile(f) ? f.localPath : util.vsixPathToFilePath(f.path),
 			// File path in the VSIX
 			tree: f.path
 		})),

src/test/fixtures/manifestFiles/LICENSE

@@ -0,0 +1 @@
+LICENSE...

src/test/fixtures/manifestFiles/foo3/bar3/hello.txt

@@ -0,0 +1,12 @@
+Some text in here!
+Some text in here!
+Some text in here!
+Some text in here!
+Some text in here!
+Some text in here!
+Some text in here!
+Some text in here!
+Some text in here!
+Some text in here!
+Some text in here!
+Some text in here!

src/test/fixtures/manifestFiles/package.json

@@ -3,5 +3,5 @@
 	"publisher": "joaomoreno",
 	"version": "1.0.0",
 	"engines": { "vscode": "*" },
-	"files": ["foo", "foo2/bar2/include.me", "*/bar3/**", "package.json"]
+	"files": ["foo", "foo2/bar2/include.me", "*/bar3/**", "package.json", "LICENSE"]
 }

src/test/package.test.ts

@@ -14,6 +14,7 @@ import {
 	versionBump,
 	VSIX,
 	LicenseProcessor,
+	printAndValidatePackagedFiles,
 } from '../package';
 import { ManifestPackage } from '../manifest';
 import * as path from 'path';
@@ -88,6 +89,63 @@ function createManifest(extra: Partial<ManifestPackage> = {}): ManifestPackage {
 	};
 }
 
+const PROCESS_ERROR_MESSAGE = 'PROCESS ERROR';
+async function testPrintAndValidatePackagedFiles(files: IFile[], cwd: string, manifest: ManifestPackage, options: IPackageOptions, errorExpected: boolean, warningExpected: boolean): Promise<void> {
+	const originalLogError = log.error;
+	const originalLogWarn = log.warn;
+	const originalProcessExit = process.exit;
+	const warns: string[] = [];
+	const errors: string[] = [];
+	let exited = false;
+	let errorThrown: string | undefined;
+	log.error = (message: string) => errors.push(message);
+	log.warn = (message: string) => warns.push(message);
+	process.exit = (() => { exited = true; throw Error(PROCESS_ERROR_MESSAGE); }) as () => never;
+
+	try {
+		await printAndValidatePackagedFiles(files, cwd, manifest, options);
+	} catch (e: any) {
+		if (e instanceof Error && e.message !== PROCESS_ERROR_MESSAGE) {
+			errorThrown = e.message + '\n' + e.stack;
+		}
+	} finally {
+		process.exit = originalProcessExit;
+		log.error = originalLogError;
+		log.warn = originalLogWarn;
+	}
+
+	// Validate that the correct number of errors and warnings were thrown
+	const messages = [];
+
+	if (errorExpected !== !!errors.length) {
+		if (errors.length) {
+			messages.push(...errors);
+		} else {
+			messages.push('Expected an error');
+		}
+	}
+
+	if (warningExpected !== !!warns.length) {
+		if (warns.length) {
+			messages.push(...warns);
+		} else {
+			messages.push('Expected a warning');
+		}
+	}
+
+	if (!errorExpected && exited) {
+		messages.push('Process exited');
+	}
+
+	if (!errorExpected && !!errorThrown && !exited) {
+		messages.push('Error thrown: ' + errorThrown);
+	}
+
+	if (messages.length) {
+		throw new Error(messages.join('\n'));
+	}
+}
+
 describe('collect', function () {
 	this.timeout(60000);
 
@@ -133,22 +191,55 @@ describe('collect', function () {
 		]);
 	});
 
-	it('should include content of manifest.files', async () => {
+	it('manifest.files', async () => {
 		const cwd = fixture('manifestFiles');
 		const manifest = await readManifest(cwd);
 		const files = await collect(manifest, { cwd });
 		const names = files.map(f => f.path).sort();
 
+		await testPrintAndValidatePackagedFiles(files, cwd, manifest, {}, false, false);
+
 		assert.deepStrictEqual(names, [
 			'[Content_Types].xml',
 			'extension.vsixmanifest',
+			'extension/LICENSE.txt',
 			'extension/foo/bar/hello.txt',
 			'extension/foo2/bar2/include.me',
 			'extension/foo3/bar3/hello.txt',
 			'extension/package.json',
 		]);
 	});
 
+	it('manifest.files unused-files-patterns check 1', async () => {
+		const cwd = fixture('manifestFiles');
+		const manifest = await readManifest(cwd);
+
+		const manifestCopy = { ...manifest, files: [...manifest.files ?? [], 'extension/foo/bar/bye.txt'] };
+		const files = await collect(manifestCopy, { cwd });
+
+		await testPrintAndValidatePackagedFiles(files, cwd, manifestCopy, {}, true, false);
+	});
+
+	it('manifest.files unused-files-patterns check 2', async () => {
+		const cwd = fixture('manifestFiles');
+		const manifest = await readManifest(cwd);
+
+		const manifestCopy = { ...manifest, files: [...manifest.files ?? [], 'extension/fo'] };
+		const files = await collect(manifestCopy, { cwd });
+
+		await testPrintAndValidatePackagedFiles(files, cwd, manifestCopy, {}, true, false);
+	});
+
+	it('manifest.files unused-files-patterns check 3', async () => {
+		const cwd = fixture('manifestFiles');
+		const manifest = await readManifest(cwd);
+
+		const manifestCopy = { ...manifest, files: ['**'] };
+		const files = await collect(manifestCopy, { cwd });
+
+		await testPrintAndValidatePackagedFiles(files, cwd, manifestCopy, {}, false, false);
+	});
+
 	it('should ignore devDependencies', () => {
 		const cwd = fixture('devDependencies');
 		return readManifest(cwd)

src/util.ts

@@ -284,6 +284,10 @@ export async function generateFileStructureTree(rootFolder: string, filePaths: {
 		const parts = filePath.split('/');
 		let currentLevel = folderTree;
 		parts.forEach(part => {
+			if (currentLevel === undefined) {
+				throw new Error(`currentLevel is undefined for ${part} in ${filePath}`);
+			}
+
 			if (typeof currentLevel[part] === 'number') {
 				currentLevel[part] = size;
 			} else if (currentLevel[part]) {