Skip to content

Commit 7277a07

Browse files
fix: Make tests more robust & fix path bug
1 parent a29134b commit 7277a07

10 files changed

Lines changed: 564 additions & 64 deletions

File tree

.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: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
# < definition scip-python python snapshot-util 0.1 `src.long_importer`/__init__:
22

33
import foo.bar.baz.mod
4-
# ^^^^^^^^^^^^^^^ reference snapshot-util 0.1 `foo.bar.baz.mod`/__init__:
4+
# ^^^^^^^^^^^^^^^ reference local 0
55

66
print(foo.bar.baz.mod.SuchNestedMuchWow)
77
#^^^^ reference python-stdlib 3.11 builtins/print().
8-
# ^^^^^^^^^^^^^^^ reference snapshot-util 0.1 `foo.bar.baz.mod`/__init__:
8+
# ^^^ reference local 0
99
# ^^^^^^^^^^^^^^^^^ reference snapshot-util 0.1 `src.foo.bar.baz.mod`/SuchNestedMuchWow#
1010

packages/pyright-scip/src/indexer.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -123,9 +123,9 @@ export class Indexer {
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: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,13 @@ import { IndexOptions, SnapshotOptions, mainCommand } from './MainCommand';
99
import { sendStatus, setQuiet, setShowProgressRateLimit } from './status';
1010
import { Indexer } from './indexer';
1111
import { exit } from 'process';
12+
import { TestFailure, TestError, ValidationResults } from './test-runner';
1213

13-
function indexAction(options: IndexOptions): void {
14+
15+
16+
17+
18+
export function indexAction(options: IndexOptions): void {
1419
setQuiet(options.quiet);
1520
if (options.showProgressRateLimit !== undefined) {
1621
setShowProgressRateLimit(options.showProgressRateLimit);
@@ -59,21 +64,21 @@ function indexAction(options: IndexOptions): void {
5964
process.chdir(originalWorkdir);
6065
}
6166

67+
// Simplified snapshot action - processes tests without orchestration
6268
function snapshotAction(snapshotRoot: string, options: SnapshotOptions): void {
63-
const subdir: string = options.only;
6469
const inputDirectory = path.resolve(join(snapshotRoot, 'input'));
6570
const outputDirectory = path.resolve(join(snapshotRoot, 'output'));
66-
71+
6772
let snapshotDirectories = fs.readdirSync(inputDirectory);
68-
if (subdir) {
69-
console.assert(snapshotDirectories.find((val) => val === subdir) !== undefined);
70-
snapshotDirectories = [subdir];
73+
74+
if (options.only) {
75+
console.assert(snapshotDirectories.find((val) => val === options.only) !== undefined);
76+
snapshotDirectories = [options.only];
7177
}
72-
78+
7379
for (const snapshotDir of snapshotDirectories) {
7480
let projectRoot = join(inputDirectory, snapshotDir);
7581
console.assert(fs.lstatSync(projectRoot).isDirectory());
76-
console.log(`Output path = ${options.output}`);
7782

7883
indexAction({
7984
projectName: options.projectName,
@@ -91,6 +96,8 @@ function snapshotAction(snapshotRoot: string, options: SnapshotOptions): void {
9196

9297
const scipIndexPath = path.join(projectRoot, options.output);
9398
const scipIndex = scip.Index.deserializeBinary(fs.readFileSync(scipIndexPath));
99+
100+
let hasDiff = false;
94101
for (const doc of scipIndex.documents) {
95102
if (doc.relative_path.startsWith('..')) {
96103
continue;
@@ -103,11 +110,15 @@ function snapshotAction(snapshotRoot: string, options: SnapshotOptions): void {
103110
const outputPath = path.resolve(outputDirectory, snapshotDir, relativeToInputDirectory);
104111

105112
if (options.check) {
106-
diffSnapshot(outputPath, obtained);
113+
const diffResult = diffSnapshot(outputPath, obtained);
114+
hasDiff = hasDiff || diffResult === 'different';
107115
} else {
108116
writeSnapshot(outputPath, obtained);
109117
}
110118
}
119+
if (hasDiff) {
120+
exit(1);
121+
}
111122
}
112123
}
113124

Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,192 @@
1+
import * as fs from 'fs';
2+
import * as path from 'path';
3+
import { join } from 'path';
4+
5+
export interface TestFailure {
6+
testName: string;
7+
type: 'empty-scip' | 'missing-output' | 'content-mismatch' | 'orphaned-output';
8+
message: string;
9+
}
10+
11+
export interface TestError {
12+
testName: string;
13+
type: 'invalid-filter' | 'empty-scip';
14+
message: string;
15+
}
16+
17+
export interface ValidationResults {
18+
passed: string[];
19+
failed: TestFailure[];
20+
errors: TestError[];
21+
}
22+
23+
export interface TestRunnerOptions {
24+
snapshotRoot: string;
25+
filterTests?: string;
26+
failFast?: boolean;
27+
quiet?: boolean;
28+
}
29+
30+
export interface SingleTestOptions {
31+
check: boolean;
32+
quiet: boolean;
33+
}
34+
35+
function validateFilterTestNames(inputDirectory: string, filterTestNames: string[]): TestError[] {
36+
const availableTests = fs.readdirSync(inputDirectory);
37+
const missingTests = filterTestNames.filter(name => !availableTests.includes(name));
38+
39+
if (missingTests.length > 0) {
40+
return [{
41+
testName: missingTests.join(', '),
42+
type: 'invalid-filter',
43+
message: `The following test names were not found: ${missingTests.join(', ')}. Available tests: ${availableTests.join(', ')}`
44+
}];
45+
}
46+
47+
return [];
48+
}
49+
50+
function detectOrphanedOutputs(inputDirectory: string, outputDirectory: string): TestFailure[] {
51+
if (!fs.existsSync(outputDirectory)) {
52+
return [];
53+
}
54+
55+
const inputTests = new Set(fs.readdirSync(inputDirectory));
56+
const outputTests = fs.readdirSync(outputDirectory);
57+
const orphanedOutputs: TestFailure[] = [];
58+
59+
for (const outputTest of outputTests) {
60+
if (!inputTests.has(outputTest)) {
61+
orphanedOutputs.push({
62+
testName: outputTest,
63+
type: 'orphaned-output',
64+
message: `Output folder exists but no corresponding input folder found`
65+
});
66+
}
67+
}
68+
69+
return orphanedOutputs;
70+
}
71+
72+
function reportResults(results: ValidationResults, failFast: boolean): void {
73+
const totalTests = results.passed.length + results.failed.length;
74+
const errorCount = results.errors.length;
75+
76+
// Report errors first
77+
for (const error of results.errors) {
78+
console.error(`ERROR [${error.testName}]: ${error.message}`);
79+
}
80+
81+
// Report failures
82+
for (const failure of results.failed) {
83+
console.error(`FAIL [${failure.testName}]: ${failure.message}`);
84+
}
85+
86+
// Report summary
87+
if (totalTests > 0 || errorCount > 0) {
88+
console.log(`\n${results.passed.length}/${totalTests} tests passed, ${results.failed.length} failed, ${errorCount} errored`);
89+
}
90+
91+
// Exit with non-zero status if there were failures or errors
92+
if (results.failed.length > 0 || results.errors.length > 0) {
93+
process.exit(1);
94+
}
95+
}
96+
97+
export class TestRunner {
98+
constructor(private options: TestRunnerOptions) {}
99+
100+
async runTests(
101+
singleTestRunner: (testName: string, inputDir: string, outputDir: string, options: SingleTestOptions) => Promise<ValidationResults>
102+
): Promise<void> {
103+
const inputDirectory = path.resolve(join(this.options.snapshotRoot, 'input'));
104+
const outputDirectory = path.resolve(join(this.options.snapshotRoot, 'output'));
105+
const failFast = this.options.failFast ?? false;
106+
107+
const results: ValidationResults = {
108+
passed: [],
109+
failed: [],
110+
errors: []
111+
};
112+
113+
// Pre-execution validation: determine test directories to process
114+
let snapshotDirectories = fs.readdirSync(inputDirectory);
115+
let isFilterMode = false;
116+
117+
if (this.options.filterTests) {
118+
// Filter to specific tests
119+
const filterTestNames = this.options.filterTests.split(',').map(name => name.trim());
120+
isFilterMode = true;
121+
122+
// Validate filter test names exist
123+
const filterErrors = validateFilterTestNames(inputDirectory, filterTestNames);
124+
if (filterErrors.length > 0) {
125+
results.errors.push(...filterErrors);
126+
reportResults(results, failFast);
127+
return;
128+
}
129+
130+
snapshotDirectories = snapshotDirectories.filter(dir => filterTestNames.includes(dir));
131+
}
132+
133+
// In non-filtering mode, detect orphaned outputs that should be cleaned up
134+
if (!isFilterMode) {
135+
const orphanedOutputs = detectOrphanedOutputs(inputDirectory, outputDirectory);
136+
137+
// For orphaned outputs in check mode, report as failures
138+
if (orphanedOutputs.length > 0) {
139+
results.failed.push(...orphanedOutputs);
140+
141+
if (failFast) {
142+
reportResults(results, failFast);
143+
return;
144+
}
145+
}
146+
}
147+
148+
// Process each test directory
149+
for (const testName of snapshotDirectories) {
150+
if (!this.options.quiet) {
151+
console.log(`Processing test: ${testName}`);
152+
}
153+
154+
try {
155+
const testResults = await singleTestRunner(
156+
testName,
157+
inputDirectory,
158+
outputDirectory,
159+
{
160+
check: true, // We'll determine this based on the context
161+
quiet: this.options.quiet ?? false
162+
}
163+
);
164+
165+
// Merge results
166+
results.passed.push(...testResults.passed);
167+
results.failed.push(...testResults.failed);
168+
results.errors.push(...testResults.errors);
169+
170+
// Check for fail-fast condition
171+
if (failFast && (testResults.failed.length > 0 || testResults.errors.length > 0)) {
172+
reportResults(results, failFast);
173+
return;
174+
}
175+
} catch (error) {
176+
results.errors.push({
177+
testName,
178+
type: 'empty-scip',
179+
message: `Test runner failed: ${error}`
180+
});
181+
182+
if (failFast) {
183+
reportResults(results, failFast);
184+
return;
185+
}
186+
}
187+
}
188+
189+
// Report final results
190+
reportResults(results, failFast);
191+
}
192+
}

0 commit comments

Comments
 (0)