Let the RWC harness iterate over files instead of building one big file#18416
Conversation
sandersn
left a comment
There was a problem hiding this comment.
Why not delete the old method in favour of multifile? Is there a reason to keep it around?
| export function getErrorBaseline(inputFiles: ReadonlyArray<TestFile>, diagnostics: ReadonlyArray<ts.Diagnostic>, pretty?: boolean) { | ||
| let outputLines = ""; | ||
| const gen = iterateErrorBaseline(inputFiles, diagnostics, pretty); | ||
| for (let {done, value} = gen.next(); !done; { done, value } = gen.next()) { |
There was a problem hiding this comment.
why can't we use for-of? Is it that we target es5, or because es6's spec doesn't support it?
There was a problem hiding this comment.
We do not compile with --downlevelIteration on (and we do not include a Symbol.iterator polyfill)
| Harness.Baseline.runBaseline(`${baseName}.output.js`, () => { | ||
| return Harness.Compiler.collateOutputs(compilerResult.files); | ||
| }, baselineOpts); | ||
| Harness.Baseline.runMultifileBaseline(baseName, "", () => { |
There was a problem hiding this comment.
technically we should clone these tests so runBaseline doesn't break, right? If we intend to stop using runBaseLine, maybe we should just delete it.
There was a problem hiding this comment.
runBaseline is still used for tons of not-rwc-tests, so it's not like it is going away.
|
|
||
| // 'merge' the lines of each input file with any errors associated with it | ||
| inputFiles.filter(f => f.content !== undefined).forEach(inputFile => { | ||
| const filtered = inputFiles.filter(f => f.content !== undefined); |
| // 'merge' the lines of each input file with any errors associated with it | ||
| inputFiles.filter(f => f.content !== undefined).forEach(inputFile => { | ||
| const filtered = inputFiles.filter(f => f.content !== undefined); | ||
| const dupeMap = ts.createMap<true>(); |
| writeComparison(comparison.expected, comparison.actual, relativeFileName, actualFileName); | ||
| } | ||
|
|
||
| export function runMultifileBaseline(relativeFileBase: string, extension: string, generateContent: () => IterableIterator<[string, string, number]> | IterableIterator<[string, string]>, opts?: BaselineOptions, referencedExtensions?: string[]): void { |
There was a problem hiding this comment.
looks like return type of generateContent would be better as IterableIterator<[string, string, number | undefined]>.
There was a problem hiding this comment.
An IterableIterator<[string, string]> is not assignable to an IterableIterator<[string, string, number | undefined]>, unfortunately. Tuple optionality would be nice.
| // Collect, test, and sort the fileNames | ||
| outputFiles.sort((a, b) => ts.compareStrings(cleanName(a.fileName), cleanName(b.fileName))); | ||
| let dupeCount = 0; | ||
| const dupeMap = ts.createMap<true>(); |
| // Yield them | ||
| for (const outputFile of outputFiles) { | ||
| let resultName = sanitizeTestFilePath(outputFile.fileName); | ||
| if (dupeMap.has(resultName)) { |
There was a problem hiding this comment.
would it improve readability to extract this code to a function?
There was a problem hiding this comment.
I saw it 3 or 4 times so I think it's a good idea.
| Harness.Baseline.runBaseline(outputFileName, () => fullBaseLine, opts); | ||
| baselinePath.replace(/\.tsx?/, "") : baselinePath; | ||
|
|
||
| if (!multifile) { |
There was a problem hiding this comment.
do we still need the non-multifile mode? why not get rid of it?
There was a problem hiding this comment.
We use the non-multifile mode for the compiler runner, still. Which is probably a good thing, since those tests are small.
| } | ||
|
|
||
| function *iterateBaseLine(typeWriterResults: ts.Map<TypeWriterResult[]>, isSymbolBaseline: boolean): IterableIterator<[string, string]> { | ||
| let typeLines = ""; |
There was a problem hiding this comment.
did you switch to string from string[] because string concatenation is fast enough in JS?
|
@sandersn I did delete all the baselines for the old method... for our RWC tests. The single-output-file baseline methods are still used for compiler, conformance, test262, and some other tests, however, so the code is going to stick around. (Though all the collation/combining methods now defer to the iteration methods, to reduce duplication!) That's probably a good thing, since I really didn't want four 3-line files for each of the module resolution tests, anyway. |
1d92f9f to
e420c9c
Compare
e420c9c to
7654f47
Compare
@mhegazy mentioned yesterday that he'd like it more if our RWC inputs and outputs were split into more files, such that they were easier to review changes to. This does that. There is a PR on our internal tracker with corresponding baseline updates.