fix: Make tests more robust & fix path-handling bugs on macOS

.gitignore

@@ -6,6 +6,9 @@ yarn-debug.log*
 yarn-error.log*
 lerna-debug.log*
 
+# Editor files
+.idea/
+
 # Diagnostic reports (https://nodejs.org/api/report.html)
 report.[0-9]*.[0-9]*.[0-9]*.[0-9]*.json
 

packages/pyright-internal/src/analyzer/program.ts

@@ -331,7 +331,15 @@ export class Program {
 
     addTrackedFile(filePath: string, isThirdPartyImport = false, isInPyTypedPackage = false): SourceFile {
         let sourceFileInfo = this.getSourceFileInfo(filePath);
-        const importName = this._getImportNameForFile(filePath);
+        let importName = this._getImportNameForFile(filePath);
+        // HACK(scip-python): When adding tracked files for imports, we end up passing
+        // normalized paths as the argument. However, _getImportNameForFile seemingly
+        // needs a non-normalized path, which cannot be recovered directly from a
+        // normalized path. However, in practice, the non-normalized path seems to
+        // be stored on the sourceFileInfo, so attempt to use that instead.
+        if (importName === '' && sourceFileInfo) {
+            importName = this._getImportNameForFile(sourceFileInfo.sourceFile.getFilePath());
+        }
 
         if (sourceFileInfo) {
             // The module name may have changed based on updates to the

packages/pyright-scip/CONTRIBUTING.md

@@ -73,9 +73,32 @@ node ./index.js <other args>
 npm run check-snapshots
 ```
 
+#### Filter specific snapshot tests
+
+Use the `--filter-tests` flag to run only specific snapshot tests:
+```bash
+# Using npm scripts (note the -- to pass arguments)
+npm run check-snapshots -- --filter-tests test1,test2,test3
+```
+
+Available snapshot tests can be found in `snapshots/input/`.
+
 Using a different Python version other than the one specified
 in `.tool-versions` may also lead to errors.
 
+## Making changes to Pyright internals
+
+When modifying code in the `pyright-internal` package:
+
+1. Keep changes minimal: Every change introduces a risk of
+   merge conflicts. Adding doc comments is fine, but avoid
+   changing functionality if possible. Instead of changing
+   access modifiers, prefer copying small functions into
+   scip-pyright logic.
+2. Use a `NOTE(scip-python):` prefix when adding comments to
+   make it clearer which comments were added by upstream
+   maintainers vs us.
+
 ## Publishing releases
 
 1. Change the version in `packages/pyright-scip/package.json`

packages/pyright-scip/src/config.ts

@@ -17,6 +17,7 @@ import {
 } from 'pyright-internal/common/pathUtils';
 import { PyrightFileSystem } from 'pyright-internal/pyrightFileSystem';
 import { ScipConfig } from './lib';
+import { sendStatus } from './status';
 
 const configFileNames = ['scip-pyrightconfig.json', 'pyrightconfig.json'];
 const pyprojectTomlName = 'pyproject.toml';
@@ -26,6 +27,8 @@ export class ScipPyrightConfig {
     _configFilePath: string | undefined;
     _configOptions: ConfigOptions;
 
+    // Use this for debug logging only. For sending user messages, use sendStatus.
+    // Some old code does not respect this.
     _console: Console = console;
     _typeCheckingMode = 'basic';
 
@@ -100,7 +103,7 @@ export class ScipPyrightConfig {
             if (configFilePath) {
                 projectRoot = getDirectoryPath(configFilePath);
             } else {
-                this._console.log(`No configuration file found.`);
+                sendStatus(`No configuration file found.`);
                 configFilePath = undefined;
             }
         }
@@ -115,9 +118,9 @@ export class ScipPyrightConfig {
 
             if (pyprojectFilePath) {
                 projectRoot = getDirectoryPath(pyprojectFilePath);
-                this._console.log(`pyproject.toml file found at ${projectRoot}.`);
+                sendStatus(`pyproject.toml file found at ${projectRoot}.`);
             } else {
-                this._console.log(`No pyproject.toml file found.`);
+                sendStatus(`No pyproject.toml file found.`);
             }
         }
 
@@ -180,7 +183,7 @@ export class ScipPyrightConfig {
             this._console.info(`Loading configuration file at ${configFilePath}`);
             configJsonObj = this._parseJsonConfigFile(configFilePath);
         } else if (pyprojectFilePath) {
-            this._console.info(`Loading pyproject.toml file at ${pyprojectFilePath}`);
+            sendStatus(`Loading pyproject.toml file at ${pyprojectFilePath}`);
             configJsonObj = this._parsePyprojectTomlFile(pyprojectFilePath);
         }
 

packages/pyright-scip/src/indexer.ts

@@ -117,15 +117,15 @@ export class Indexer {
             this.projectFiles = targetFiles;
         }
 
-        console.log('Total Project Files', this.projectFiles.size);
+        sendStatus(`Total Project Files ${this.projectFiles.size}`);
 
         const host = new FullAccessHost(fs);
         this.importResolver = new ImportResolver(fs, this.pyrightConfig, host);
 
         this.program = new Program(this.importResolver, this.pyrightConfig);
-        // Normalize paths to ensure consistency with other code paths.
-        const normalizedProjectFiles = [...this.projectFiles].map((path: string) => normalizePathCase(fs, path));
-        this.program.setTrackedFiles(normalizedProjectFiles);
+        // setTrackedFiles internally handles path normalization, so we don't normalize
+        // paths here.
+        this.program.setTrackedFiles([...this.projectFiles]);
 
         if (scipConfig.projectNamespace) {
             setProjectNamespace(scipConfig.projectName, this.scipConfig.projectNamespace!);
@@ -194,7 +194,9 @@ export class Indexer {
         let projectSourceFiles: SourceFile[] = [];
         withStatus('Index workspace and track project files', () => {
             this.program.indexWorkspace((filepath: string) => {
-                // Filter out filepaths not part of this project
+                // Do not index files outside the project because SCIP doesn't support it.
+                //
+                // Both filepath and this.scipConfig.projectRoot are NOT normalized.
                 if (filepath.indexOf(this.scipConfig.projectRoot) != 0) {
                     return;
                 }
@@ -204,6 +206,7 @@ export class Indexer {
 
                 let requestsImport = sourceFile.getImports();
                 requestsImport.forEach((entry) =>
+                    // entry.resolvedPaths are all normalized.
                     entry.resolvedPaths.forEach((value) => {
                         this.program.addTrackedFile(value, true, false);
                     })

packages/pyright-scip/src/lib.ts

@@ -310,10 +310,10 @@ export function writeSnapshot(outputPath: string, obtained: string): void {
     fs.writeFileSync(outputPath, obtained, { flag: 'w' });
 }
 
-export function diffSnapshot(outputPath: string, obtained: string): void {
+export function diffSnapshot(outputPath: string, obtained: string): 'equal' | 'different' {
     let existing = fs.readFileSync(outputPath, { encoding: 'utf8' });
     if (obtained === existing) {
-        return;
+        return 'equal';
     }
 
     console.error(
@@ -326,7 +326,7 @@ export function diffSnapshot(outputPath: string, obtained: string): void {
             '(what the current code produces). Run the command "npm run update-snapshots" to accept the new behavior.'
         )
     );
-    exit(1);
+    return 'different';
 }
 
 function occurrencesByLine(a: scip.Occurrence, b: scip.Occurrence): number {

packages/pyright-scip/src/main-impl.ts

@@ -10,7 +10,8 @@ import { sendStatus, setQuiet, setShowProgressRateLimit } from './status';
 import { Indexer } from './indexer';
 import { exit } from 'process';
 
-function indexAction(options: IndexOptions): void {
+
+export function indexAction(options: IndexOptions): void {
     setQuiet(options.quiet);
     if (options.showProgressRateLimit !== undefined) {
         setShowProgressRateLimit(options.showProgressRateLimit);
@@ -91,6 +92,8 @@ function snapshotAction(snapshotRoot: string, options: SnapshotOptions): void {
 
         const scipIndexPath = path.join(projectRoot, options.output);
         const scipIndex = scip.Index.deserializeBinary(fs.readFileSync(scipIndexPath));
+
+        let hasDiff = false;
         for (const doc of scipIndex.documents) {
             if (doc.relative_path.startsWith('..')) {
                 continue;
@@ -103,11 +106,15 @@ function snapshotAction(snapshotRoot: string, options: SnapshotOptions): void {
             const outputPath = path.resolve(outputDirectory, snapshotDir, relativeToInputDirectory);
 
             if (options.check) {
-                diffSnapshot(outputPath, obtained);
+                const diffResult = diffSnapshot(outputPath, obtained);
+                hasDiff = hasDiff || diffResult === 'different';
             } else {
                 writeSnapshot(outputPath, obtained);
             }
         }
+        if (hasDiff) {
+            exit(1);
+        }
     }
 }
 

packages/pyright-scip/src/test-runner.ts

@@ -0,0 +1,162 @@
+import * as fs from 'fs';
+import * as path from 'path';
+import { join } from 'path';
+
+export interface TestFailure {
+    testName: string;
+    type: 'empty-scip-index' | 'missing-output' | 'content-mismatch' | 'orphaned-output' | 'caught-exception';
+    message: string;
+}
+
+export interface ValidationResults {
+    passed: string[];
+    failed: TestFailure[];
+    skipped: string[];
+}
+
+export interface TestRunnerOptions {
+    snapshotRoot: string;
+    filterTests?: string;
+    failFast: boolean;
+    quiet: boolean;
+    mode: 'check' | 'update';
+}
+
+export interface SingleTestOptions {
+    check: boolean;
+    quiet: boolean;
+}
+
+function validateFilterTestNames(inputDirectory: string, filterTestNames: string[]): void {
+    const availableTests = fs.readdirSync(inputDirectory);
+    const missingTests = filterTestNames.filter(name => !availableTests.includes(name));
+
+    if (missingTests.length > 0) {
+        console.error(`ERROR: The following test names were not found: ${missingTests.join(', ')}. Available tests: ${availableTests.join(', ')}`);
+        process.exit(1);
+    }
+}
+
+function handleOrphanedOutputs(inputTests: Set<string>, outputDirectory: string, mode: 'check' | 'update'): TestFailure[] {
+    if (!fs.existsSync(outputDirectory)) {
+        return [];
+    }
+
+    const outputTests = fs.readdirSync(outputDirectory);
+    const orphanedOutputs: TestFailure[] = [];
+
+    for (const outputTest of outputTests) {
+        if (inputTests.has(outputTest)) {
+            continue;
+        }
+        if (mode === 'update') {
+            const orphanedPath = path.join(outputDirectory, outputTest);
+            fs.rmSync(orphanedPath, { recursive: true, force: true });
+            console.log(`Delete output folder with no corresponding input folder: ${outputTest}`);
+            continue;
+        }
+        orphanedOutputs.push({
+            testName: outputTest,
+            type: 'orphaned-output',
+            message: `Output folder exists but no corresponding input folder found`
+        });
+    }
+
+    return orphanedOutputs;
+}
+
+function reportResults(results: ValidationResults): void {
+    const totalTests = results.passed.length + results.failed.length + results.skipped.length;
+    console.assert(totalTests > 0, 'No tests found');
+
+    for (const failure of results.failed) {
+        console.error(`FAIL [${failure.testName}]: ${failure.message}`);
+    }
+
+    let summaryStr = `\n${results.passed.length}/${totalTests} tests passed, ${results.failed.length} failed`;
+    if (results.skipped.length > 0) {
+        summaryStr += `, ${results.skipped.length} skipped`;
+    }
+    console.log(summaryStr);
+
+    if (results.failed.length > 0) {
+        process.exit(1);
+    }
+}
+
+export class TestRunner {
+    constructor(private options: TestRunnerOptions) {}
+
+    runTests(
+        runSingleTest: (testName: string, inputDir: string, outputDir: string) => ValidationResults
+    ): void {
+        const inputDirectory = path.resolve(join(this.options.snapshotRoot, 'input'));
+        const outputDirectory = path.resolve(join(this.options.snapshotRoot, 'output'));
+
+        const results: ValidationResults = {
+            passed: [],
+            failed: [],
+            skipped: []
+        };
+
+        let snapshotDirectories = fs.readdirSync(inputDirectory);
+
+        const orphanedOutputs = handleOrphanedOutputs(new Set(snapshotDirectories), outputDirectory, this.options.mode);
+        if (orphanedOutputs.length > 0) {
+            results.failed.push(...orphanedOutputs);
+            if (this.options.failFast) {
+                reportResults(results);
+                return;
+            }
+        }
+
+        if (this.options.filterTests) {
+            const filterTestNames = this.options.filterTests.split(',').map(name => name.trim());
+            validateFilterTestNames(inputDirectory, filterTestNames);
+            snapshotDirectories = snapshotDirectories.filter(dir => filterTestNames.includes(dir));
+            if (snapshotDirectories.length === 0) {
+                console.error(`No tests found matching filter: ${this.options.filterTests}`);
+                process.exit(1);
+            }
+        }
+
+        for (let i = 0; i < snapshotDirectories.length; i++) {
+            const testName = snapshotDirectories[i];
+            if (!this.options.quiet) {
+                console.log(`--- Running snapshot test: ${testName} ---`);
+            }
+
+            let testResults: ValidationResults;
+            try {
+                testResults = runSingleTest(
+                    testName, 
+                    inputDirectory, 
+                    outputDirectory,
+                );
+            } catch (error) {
+                testResults = {
+                    passed: [],
+                    failed: [{
+                        testName,
+                        type: 'caught-exception',
+                        message: `Test runner failed: ${error}`
+                    }],
+                    skipped: []
+                };
+            }
+
+            results.passed.push(...testResults.passed);
+            results.failed.push(...testResults.failed);
+
+            if (this.options.failFast && testResults.failed.length > 0) {
+                for (let j = i + 1; j < snapshotDirectories.length; j++) {
+                    results.skipped.push(snapshotDirectories[j]);
+                }
+                reportResults(results);
+                return;
+            }
+        }
+
+        reportResults(results);
+    }
+}