WIP: Add source map spec tests #505
Conversation
This commit adds a submodule for the draft source map spec test repo and adds a new test file that runs each of the test cases. Some test cases that have known failures are skipped, with a comment explaining why.
In an indexed map, the offset line & column are stored 1-based. However, the lookup for originalPositionFor was not incrementing the 0-based column from the API argument.
| // Known failures due to intentional implementation choices or due to bugs. | ||
| const skippedTests = [ | ||
| // Versions are explicitly checked a bit loosely. | ||
| "versionNumericString", | ||
| // Stricter sources array checking isn't implemented. | ||
| "sourcesNotStringOrNull", | ||
| "sourcesAndSourcesContentBothNull", | ||
| // Stricter names array checking isn't implemented. | ||
| "namesMissing", | ||
| "namesNotString", | ||
| // This check isn't as strict in this library. | ||
| "invalidMappingNotAString1", | ||
| // A mapping segment with no fields is technically invalid in the spec. | ||
| "invalidMappingSegmentWithZeroFields", | ||
| // These tests fail due to imprecision in the spec about the 32-bit limit. | ||
| "invalidMappingSegmentWithColumnExceeding32Bits", | ||
| "invalidMappingSegmentWithOriginalLineExceeding32Bits", | ||
| "invalidMappingSegmentWithOriginalColumnExceeding32Bits", | ||
| // A large VLQ that should parse, but currently does not. | ||
| "validMappingLargeVLQ", | ||
| // The library currently doesn't check the types of offset lines/columns. | ||
| "indexMapOffsetLineWrongType", | ||
| "indexMapOffsetColumnWrongType", | ||
| // The spec is not totally clear about this case. | ||
| "indexMapInvalidBaseMappings", | ||
| // The spec's definition of overlap can be refined | ||
| "indexMapInvalidOverlap", | ||
| // Not clear if this test makes sense, but spec isn't clear on behavior | ||
| "validMappingNullSources" | ||
| ] |
There was a problem hiding this comment.
Are these skipped tests specific to the mozilla sourcemap implementation?
There was a problem hiding this comment.
Yes, for example the "versionNumericString" test is skipped because this library specifically accepts the case where the version field is a number in a string. Other implementations have different behavior (e.g., the source map validator uses this library but layers more strict checking on top).
There was a problem hiding this comment.
Ok thanks. that makes sense. Fixing these will help align to the spec.
| exports[`test from source map spec tests, name: ${testCase.name}`] = | ||
| async function (assert) { | ||
| const json = await readJSON(`./source-map-tests/resources/${testCase.sourceMapFile}`); | ||
| let sourceMapFailed = false; |
There was a problem hiding this comment.
This does not seem to be used.
There was a problem hiding this comment.
Thanks for catching that, I think it was a vestige of an earlier attempt. I'll remove it next time I update the patch.
|
Thanks for this patch. this is looking great. I just had a quick look. I'll be testing it in a little bit |
|
Note: I updated the patch with support for checking generated->original mapping (before it just checked original->generated). Now it depends on PR #507 as well. I also updated it with checking for transitive mapping. Both of these are pretty localized changes. |
| map.eachMapping(() => {}); | ||
| map.destroy(); | ||
| } catch (exn) { | ||
| if (testCase.sourceMapIsValid) |
There was a problem hiding this comment.
| if (testCase.sourceMapIsValid) | |
| if (testCase.sourceMapShouldBeValid) |
There was a problem hiding this comment.
This is not related directly to this patch but it just a tiny suggestion while trying the understanding the code.
sourceMapShouldBeValid might make it clearer what the expectation is.
| if (!testCase.sourceMapIsValid) | ||
| assert.fail("Expected invalid source map but loaded successfully"); | ||
| if (testCase.testActions) { | ||
| for (let testAction of testCase.testActions) { |
There was a problem hiding this comment.
| for (let testAction of testCase.testActions) { | |
| for (const testAction of testCase.testActions) { |
There was a problem hiding this comment.
Maybe we could use const since testAction is not reassigned.
| column: action.generatedColumn, | ||
| }); | ||
|
|
||
| for (let intermediateMapPath of action.intermediateMaps) { |
There was a problem hiding this comment.
| for (let intermediateMapPath of action.intermediateMaps) { | |
| for (const intermediateMapPath of action.intermediateMaps) { |
There was a problem hiding this comment.
Since intermediateMapPath is not reassigned
| return SourceMapConsumer.with(rawSourceMap, null, (consumer) => { | ||
| let mappedPosition = consumer.generatedPositionFor({ | ||
| source: action.originalSource, | ||
| line: action.originalLine + 1, |
There was a problem hiding this comment.
| line: action.originalLine + 1, | |
| line: mapLine(action.originalLine), |
function mapLine(line) {
return line + 1;
}
There was a problem hiding this comment.
So we seem to be adding 1 to all the lines in most places here, we can have a util function which adds the line.
|
Thanks @takikawa. I tested out the patch and it works nicely. I've added a few inline comments. Once the tests have been moved to the main repo we can updated and land this patch. Do you know the timeline for this? Also could we automate the update of the git sub-module by adding the command to the npm task for running tests in the This is a basic inital solution, it can be improved with followups later |
This is a draft PR for issue #504.
The PR adds integration to run the in-development source map specification tests. The tests are accessed via a git submodule, though the current location of the tests is likely not their final official location. The tests are still under development as well.