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/assertions.ts

@@ -0,0 +1,130 @@
+import { normalizePathCase, isFileSystemCaseSensitive } from 'pyright-internal/common/pathUtils';
+import { PyrightFileSystem } from 'pyright-internal/pyrightFileSystem';
+import { createFromRealFileSystem } from 'pyright-internal/common/realFileSystem';
+
+export enum SeenCondition {
+    AlwaysFalse = 'always-false',
+    AlwaysTrue = 'always-true',
+    Mixed = 'mixed'
+}
+
+export class AssertionError extends Error {
+    constructor(message: string) {
+        super(message);
+        this.name = 'AssertionError';
+    }
+}
+
+// Private global state - never export directly
+let _assertionFlags = {
+    pathNormalizationChecks: false,
+    otherChecks: false
+};
+let _context = '';
+const _sometimesResults = new Map<string, Map<string, SeenCondition>>();
+
+export function setGlobalAssertionFlags(pathNormalizationChecks: boolean, otherChecks: boolean): void {
+    _assertionFlags.pathNormalizationChecks = pathNormalizationChecks;
+    _assertionFlags.otherChecks = otherChecks;
+}
+
+export function setGlobalContext(context: string): void {
+    _context = context;
+}
+
+// Internal implementation functions
+function assertAlwaysImpl(enableFlag: boolean, check: () => boolean, message: () => string): void {
+    if (!enableFlag) return;
+
+    if (!check()) {
+        throw new AssertionError(message());
+    }
+}
+
+function assertSometimesImpl(enableFlag: boolean, check: () => boolean, key: string): void {
+    if (!enableFlag) return;
+
+    const ctx = _context;
+    if (ctx === '') {
+        throw new AssertionError('Context must be set before calling assertSometimes');
+    }
+
+    let ctxMap = _sometimesResults.get(key);
+    if (!ctxMap) {
+        ctxMap = new Map();
+        _sometimesResults.set(key, ctxMap);
+    }
+
+    const result = check() ? SeenCondition.AlwaysTrue : SeenCondition.AlwaysFalse;
+    const prev = ctxMap.get(ctx);
+
+    if (prev === undefined) {
+        ctxMap.set(ctx, result);
+    } else if (prev !== result) {
+        ctxMap.set(ctx, SeenCondition.Mixed);
+    }
+}
+
+const _fs = new PyrightFileSystem(createFromRealFileSystem());
+
+export function assertAlways(check: () => boolean, message: () => string): void {
+    assertAlwaysImpl(_assertionFlags.otherChecks, check, message);
+}
+
+export function assertSometimes(check: () => boolean, key: string): void {
+    assertSometimesImpl(_assertionFlags.otherChecks, check, key);
+}
+
+export function assertNeverNormalized(path: string): void {
+    const normalized = normalizePathCase(_fs, path);
+    assertAlwaysImpl(
+        _assertionFlags.pathNormalizationChecks,
+        () => normalized !== path,
+        () => `Path should not be normalized but was: ${path}`
+    );
+}
+
+export function assertAlwaysNormalized(path: string): void {
+    const normalized = normalizePathCase(_fs, path);
+    assertAlwaysImpl(
+        _assertionFlags.pathNormalizationChecks,
+        () => normalized === path,
+        () => `Path should be normalized but was not: ${path} -> ${normalized}`
+    );
+}
+
+export function assertSometimesNormalized(path: string, key: string): void {
+    const normalized = normalizePathCase(_fs, path);
+    assertSometimesImpl(
+        _assertionFlags.pathNormalizationChecks,
+        () => normalized === path,
+        key
+    );
+}
+
+// Monoidal combination logic
+function combine(a: SeenCondition, b: SeenCondition): SeenCondition {
+    if (a === b) return a;
+    if (a === SeenCondition.Mixed || b === SeenCondition.Mixed) {
+        return SeenCondition.Mixed;
+    }
+    // AlwaysTrue + AlwaysFalse = Mixed
+    return SeenCondition.Mixed;
+}
+
+export function checkSometimesAssertions(): Map<string, SeenCondition> {
+    const summary = new Map<string, SeenCondition>();
+
+    for (const [key, ctxMap] of _sometimesResults) {
+        let agg: SeenCondition | undefined;
+        for (const state of ctxMap.values()) {
+            agg = agg === undefined ? state : combine(agg, state);
+            if (agg === SeenCondition.Mixed) break;
+        }
+        if (agg !== undefined) {
+            summary.set(key, agg);
+        }
+    }
+
+    return summary;
+}

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);
+        }
     }
 }