Skip to content

Commit 235614c

Browse files
fix: Make tests more robust & fix path-handling bugs on macOS (#183)
So it looks like basically there was a bug where the indexer was generating 0 documents on macOS (related to the recent bug fix in https://github.com/sourcegraph/scip-python/pull/177/files where we started normalizing the file paths passed to setTrackedFiles). 😬 However, the tests were passing on macOS not because they were correct, but because the zero documents case bypassed the updating/diffing process. 😢 This patch changes the test diffing logic to be more robust, in handling various scenarios such as 0 documents. It also introduces a couple of flags, one for filtering tests, and one for running tests with fail-fast behavior. On fixing the tests, I discovered some bugs due to improper handling of case-sensitivity, so I fixed those (which is why the snapshots are unchanged and the tests are passing on macOS). For some of these, I'm not super sure about Pyright behavior, so I added a little assertions library inspired by Antithesis so that we can test the coverage of different cases by our test suite on macOS. See also: https://antithesis.com/docs/best_practices/sometimes_assertions/
1 parent a29134b commit 235614c

File tree

11 files changed

+653
-56
lines changed

11 files changed

+653
-56
lines changed

.gitignore

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@ yarn-debug.log*
66
yarn-error.log*
77
lerna-debug.log*
88

9+
# Editor files
10+
.idea/
11+
912
# Diagnostic reports (https://nodejs.org/api/report.html)
1013
report.[0-9]*.[0-9]*.[0-9]*.[0-9]*.json
1114

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

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -331,7 +331,15 @@ export class Program {
331331

332332
addTrackedFile(filePath: string, isThirdPartyImport = false, isInPyTypedPackage = false): SourceFile {
333333
let sourceFileInfo = this.getSourceFileInfo(filePath);
334-
const importName = this._getImportNameForFile(filePath);
334+
let importName = this._getImportNameForFile(filePath);
335+
// HACK(scip-python): When adding tracked files for imports, we end up passing
336+
// normalized paths as the argument. However, _getImportNameForFile seemingly
337+
// needs a non-normalized path, which cannot be recovered directly from a
338+
// normalized path. However, in practice, the non-normalized path seems to
339+
// be stored on the sourceFileInfo, so attempt to use that instead.
340+
if (importName === '' && sourceFileInfo) {
341+
importName = this._getImportNameForFile(sourceFileInfo.sourceFile.getFilePath());
342+
}
335343

336344
if (sourceFileInfo) {
337345
// The module name may have changed based on updates to the

packages/pyright-scip/CONTRIBUTING.md

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,9 +73,32 @@ node ./index.js <other args>
7373
npm run check-snapshots
7474
```
7575

76+
#### Filter specific snapshot tests
77+
78+
Use the `--filter-tests` flag to run only specific snapshot tests:
79+
```bash
80+
# Using npm scripts (note the -- to pass arguments)
81+
npm run check-snapshots -- --filter-tests test1,test2,test3
82+
```
83+
84+
Available snapshot tests can be found in `snapshots/input/`.
85+
7686
Using a different Python version other than the one specified
7787
in `.tool-versions` may also lead to errors.
7888

89+
## Making changes to Pyright internals
90+
91+
When modifying code in the `pyright-internal` package:
92+
93+
1. Keep changes minimal: Every change introduces a risk of
94+
merge conflicts. Adding doc comments is fine, but avoid
95+
changing functionality if possible. Instead of changing
96+
access modifiers, prefer copying small functions into
97+
scip-pyright logic.
98+
2. Use a `NOTE(scip-python):` prefix when adding comments to
99+
make it clearer which comments were added by upstream
100+
maintainers vs us.
101+
79102
## Publishing releases
80103

81104
1. Change the version in `packages/pyright-scip/package.json`
Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
import { normalizePathCase, isFileSystemCaseSensitive } from 'pyright-internal/common/pathUtils';
2+
import { PyrightFileSystem } from 'pyright-internal/pyrightFileSystem';
3+
import { createFromRealFileSystem } from 'pyright-internal/common/realFileSystem';
4+
5+
export enum SeenCondition {
6+
AlwaysFalse = 'always-false',
7+
AlwaysTrue = 'always-true',
8+
Mixed = 'mixed'
9+
}
10+
11+
export class AssertionError extends Error {
12+
constructor(message: string) {
13+
super(message);
14+
this.name = 'AssertionError';
15+
}
16+
}
17+
18+
// Private global state - never export directly
19+
let _assertionFlags = {
20+
pathNormalizationChecks: false,
21+
otherChecks: false
22+
};
23+
let _context = '';
24+
const _sometimesResults = new Map<string, Map<string, SeenCondition>>();
25+
26+
export function setGlobalAssertionFlags(pathNormalizationChecks: boolean, otherChecks: boolean): void {
27+
_assertionFlags.pathNormalizationChecks = pathNormalizationChecks;
28+
_assertionFlags.otherChecks = otherChecks;
29+
}
30+
31+
export function setGlobalContext(context: string): void {
32+
_context = context;
33+
}
34+
35+
// Internal implementation functions
36+
function assertAlwaysImpl(enableFlag: boolean, check: () => boolean, message: () => string): void {
37+
if (!enableFlag) return;
38+
39+
if (!check()) {
40+
throw new AssertionError(message());
41+
}
42+
}
43+
44+
function assertSometimesImpl(enableFlag: boolean, check: () => boolean, key: string): void {
45+
if (!enableFlag) return;
46+
47+
const ctx = _context;
48+
if (ctx === '') {
49+
throw new AssertionError('Context must be set before calling assertSometimes');
50+
}
51+
52+
let ctxMap = _sometimesResults.get(key);
53+
if (!ctxMap) {
54+
ctxMap = new Map();
55+
_sometimesResults.set(key, ctxMap);
56+
}
57+
58+
const result = check() ? SeenCondition.AlwaysTrue : SeenCondition.AlwaysFalse;
59+
const prev = ctxMap.get(ctx);
60+
61+
if (prev === undefined) {
62+
ctxMap.set(ctx, result);
63+
} else if (prev !== result) {
64+
ctxMap.set(ctx, SeenCondition.Mixed);
65+
}
66+
}
67+
68+
const _fs = new PyrightFileSystem(createFromRealFileSystem());
69+
70+
export function assertAlways(check: () => boolean, message: () => string): void {
71+
assertAlwaysImpl(_assertionFlags.otherChecks, check, message);
72+
}
73+
74+
export function assertSometimes(check: () => boolean, key: string): void {
75+
assertSometimesImpl(_assertionFlags.otherChecks, check, key);
76+
}
77+
78+
export function assertNeverNormalized(path: string): void {
79+
const normalized = normalizePathCase(_fs, path);
80+
assertAlwaysImpl(
81+
_assertionFlags.pathNormalizationChecks,
82+
() => normalized !== path,
83+
() => `Path should not be normalized but was: ${path}`
84+
);
85+
}
86+
87+
export function assertAlwaysNormalized(path: string): void {
88+
const normalized = normalizePathCase(_fs, path);
89+
assertAlwaysImpl(
90+
_assertionFlags.pathNormalizationChecks,
91+
() => normalized === path,
92+
() => `Path should be normalized but was not: ${path} -> ${normalized}`
93+
);
94+
}
95+
96+
export function assertSometimesNormalized(path: string, key: string): void {
97+
const normalized = normalizePathCase(_fs, path);
98+
assertSometimesImpl(
99+
_assertionFlags.pathNormalizationChecks,
100+
() => normalized === path,
101+
key
102+
);
103+
}
104+
105+
// Monoidal combination logic
106+
function combine(a: SeenCondition, b: SeenCondition): SeenCondition {
107+
if (a === b) return a;
108+
if (a === SeenCondition.Mixed || b === SeenCondition.Mixed) {
109+
return SeenCondition.Mixed;
110+
}
111+
// AlwaysTrue + AlwaysFalse = Mixed
112+
return SeenCondition.Mixed;
113+
}
114+
115+
export function checkSometimesAssertions(): Map<string, SeenCondition> {
116+
const summary = new Map<string, SeenCondition>();
117+
118+
for (const [key, ctxMap] of _sometimesResults) {
119+
let agg: SeenCondition | undefined;
120+
for (const state of ctxMap.values()) {
121+
agg = agg === undefined ? state : combine(agg, state);
122+
if (agg === SeenCondition.Mixed) break;
123+
}
124+
if (agg !== undefined) {
125+
summary.set(key, agg);
126+
}
127+
}
128+
129+
return summary;
130+
}

packages/pyright-scip/src/config.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
} from 'pyright-internal/common/pathUtils';
1818
import { PyrightFileSystem } from 'pyright-internal/pyrightFileSystem';
1919
import { ScipConfig } from './lib';
20+
import { sendStatus } from './status';
2021

2122
const configFileNames = ['scip-pyrightconfig.json', 'pyrightconfig.json'];
2223
const pyprojectTomlName = 'pyproject.toml';
@@ -26,6 +27,8 @@ export class ScipPyrightConfig {
2627
_configFilePath: string | undefined;
2728
_configOptions: ConfigOptions;
2829

30+
// Use this for debug logging only. For sending user messages, use sendStatus.
31+
// Some old code does not respect this.
2932
_console: Console = console;
3033
_typeCheckingMode = 'basic';
3134

@@ -100,7 +103,7 @@ export class ScipPyrightConfig {
100103
if (configFilePath) {
101104
projectRoot = getDirectoryPath(configFilePath);
102105
} else {
103-
this._console.log(`No configuration file found.`);
106+
sendStatus(`No configuration file found.`);
104107
configFilePath = undefined;
105108
}
106109
}
@@ -115,9 +118,9 @@ export class ScipPyrightConfig {
115118

116119
if (pyprojectFilePath) {
117120
projectRoot = getDirectoryPath(pyprojectFilePath);
118-
this._console.log(`pyproject.toml file found at ${projectRoot}.`);
121+
sendStatus(`pyproject.toml file found at ${projectRoot}.`);
119122
} else {
120-
this._console.log(`No pyproject.toml file found.`);
123+
sendStatus(`No pyproject.toml file found.`);
121124
}
122125
}
123126

@@ -180,7 +183,7 @@ export class ScipPyrightConfig {
180183
this._console.info(`Loading configuration file at ${configFilePath}`);
181184
configJsonObj = this._parseJsonConfigFile(configFilePath);
182185
} else if (pyprojectFilePath) {
183-
this._console.info(`Loading pyproject.toml file at ${pyprojectFilePath}`);
186+
sendStatus(`Loading pyproject.toml file at ${pyprojectFilePath}`);
184187
configJsonObj = this._parsePyprojectTomlFile(pyprojectFilePath);
185188
}
186189

packages/pyright-scip/src/indexer.ts

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -117,15 +117,15 @@ export class Indexer {
117117
this.projectFiles = targetFiles;
118118
}
119119

120-
console.log('Total Project Files', this.projectFiles.size);
120+
sendStatus(`Total Project Files ${this.projectFiles.size}`);
121121

122122
const host = new FullAccessHost(fs);
123123
this.importResolver = new ImportResolver(fs, this.pyrightConfig, host);
124124

125125
this.program = new Program(this.importResolver, this.pyrightConfig);
126-
// Normalize paths to ensure consistency with other code paths.
127-
const normalizedProjectFiles = [...this.projectFiles].map((path: string) => normalizePathCase(fs, path));
128-
this.program.setTrackedFiles(normalizedProjectFiles);
126+
// setTrackedFiles internally handles path normalization, so we don't normalize
127+
// paths here.
128+
this.program.setTrackedFiles([...this.projectFiles]);
129129

130130
if (scipConfig.projectNamespace) {
131131
setProjectNamespace(scipConfig.projectName, this.scipConfig.projectNamespace!);
@@ -194,7 +194,9 @@ export class Indexer {
194194
let projectSourceFiles: SourceFile[] = [];
195195
withStatus('Index workspace and track project files', () => {
196196
this.program.indexWorkspace((filepath: string) => {
197-
// Filter out filepaths not part of this project
197+
// Do not index files outside the project because SCIP doesn't support it.
198+
//
199+
// Both filepath and this.scipConfig.projectRoot are NOT normalized.
198200
if (filepath.indexOf(this.scipConfig.projectRoot) != 0) {
199201
return;
200202
}
@@ -204,6 +206,7 @@ export class Indexer {
204206

205207
let requestsImport = sourceFile.getImports();
206208
requestsImport.forEach((entry) =>
209+
// entry.resolvedPaths are all normalized.
207210
entry.resolvedPaths.forEach((value) => {
208211
this.program.addTrackedFile(value, true, false);
209212
})

packages/pyright-scip/src/lib.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -310,10 +310,10 @@ export function writeSnapshot(outputPath: string, obtained: string): void {
310310
fs.writeFileSync(outputPath, obtained, { flag: 'w' });
311311
}
312312

313-
export function diffSnapshot(outputPath: string, obtained: string): void {
313+
export function diffSnapshot(outputPath: string, obtained: string): 'equal' | 'different' {
314314
let existing = fs.readFileSync(outputPath, { encoding: 'utf8' });
315315
if (obtained === existing) {
316-
return;
316+
return 'equal';
317317
}
318318

319319
console.error(
@@ -326,7 +326,7 @@ export function diffSnapshot(outputPath: string, obtained: string): void {
326326
'(what the current code produces). Run the command "npm run update-snapshots" to accept the new behavior.'
327327
)
328328
);
329-
exit(1);
329+
return 'different';
330330
}
331331

332332
function occurrencesByLine(a: scip.Occurrence, b: scip.Occurrence): number {

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,8 @@ import { sendStatus, setQuiet, setShowProgressRateLimit } from './status';
1010
import { Indexer } from './indexer';
1111
import { exit } from 'process';
1212

13-
function indexAction(options: IndexOptions): void {
13+
14+
export function indexAction(options: IndexOptions): void {
1415
setQuiet(options.quiet);
1516
if (options.showProgressRateLimit !== undefined) {
1617
setShowProgressRateLimit(options.showProgressRateLimit);
@@ -91,6 +92,8 @@ function snapshotAction(snapshotRoot: string, options: SnapshotOptions): void {
9192

9293
const scipIndexPath = path.join(projectRoot, options.output);
9394
const scipIndex = scip.Index.deserializeBinary(fs.readFileSync(scipIndexPath));
95+
96+
let hasDiff = false;
9497
for (const doc of scipIndex.documents) {
9598
if (doc.relative_path.startsWith('..')) {
9699
continue;
@@ -103,11 +106,15 @@ function snapshotAction(snapshotRoot: string, options: SnapshotOptions): void {
103106
const outputPath = path.resolve(outputDirectory, snapshotDir, relativeToInputDirectory);
104107

105108
if (options.check) {
106-
diffSnapshot(outputPath, obtained);
109+
const diffResult = diffSnapshot(outputPath, obtained);
110+
hasDiff = hasDiff || diffResult === 'different';
107111
} else {
108112
writeSnapshot(outputPath, obtained);
109113
}
110114
}
115+
if (hasDiff) {
116+
exit(1);
117+
}
111118
}
112119
}
113120

0 commit comments

Comments
 (0)