Add source map spec tests #505

takikawa · 2024-04-27T01:02:36Z

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.

bomsy · 2024-05-08T13:50:41Z

test/test-spec-tests.js

+// 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"
+]


Are these skipped tests specific to the mozilla sourcemap implementation?

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).

Ok thanks. that makes sense. Fixing these will help align to the spec.

bomsy · 2024-05-08T13:52:34Z

test/test-spec-tests.js

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


This does not seem to be used.

Thanks for catching that, I think it was a vestige of an earlier attempt. I'll remove it next time I update the patch.

bomsy · 2024-05-08T15:40:27Z

Thanks for this patch. this is looking great. I just had a quick look. I'll be testing it in a little bit

takikawa · 2024-05-09T18:30:51Z

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.

bomsy · 2024-05-28T11:13:10Z

test/test-spec-tests.js

+        map.eachMapping(() => {});
+        map.destroy();
+      } catch (exn) {
+        if (testCase.sourceMapIsValid)


Suggested change

if (testCase.sourceMapIsValid)

if (testCase.sourceMapShouldBeValid)

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.

test/test-spec-tests.js

bomsy · 2024-05-30T11:00:51Z

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 package.json
This simple task seems to work for me.

"test": "git submodule update --init --recursive; node test/run-tests.js",

This is a basic inital solution, it can be improved with followups later

takikawa · 2024-06-04T18:55:40Z

Once the tests have been moved to the main repo we can updated and land this patch. Do you know the timeline for this?

Thanks for the comments! We've now imported the tests into a more official repo (https://github.com/tc39/source-map-tests). I'll work on updating this PR to address your comments and update for the new location soon (hopefully this week).

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.

* Don't do reverse lookup test if source is null * Allow "null" in cases where null is expected

These aren't supported yet because the library doesn't support ignoreList yet and doesn't do any checking for it.

takikawa · 2024-06-06T08:05:10Z

I've update the PR and pointed it at the new repo, and addressed your comments (except one, commented above). Happy to address any further feedback.

Also makes the linter ignore the submodule

codecov-commenter · 2024-06-06T08:15:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.11%. Comparing base (60adcb0) to head (64050e6).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #505      +/-   ##
==========================================
+ Coverage   93.90%   94.11%   +0.20%     
==========================================
  Files          13       13              
  Lines        2988     2991       +3     
==========================================
+ Hits         2806     2815       +9     
+ Misses        182      176       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

test/test-spec-tests.js

bomsy · 2024-06-07T17:03:55Z

@takikawa The patch LGTM. Added a comment to resolve the failure in the jobs.

Fix the require of the fs promises module

bomsy

LGTM! Lets land this Thanks @takikawa for the patch.

bomsy · 2024-06-09T14:37:31Z

@takikawa Applying the fix to see if the

@takikawa The patch LGTM. Added a comment to resolve the failure in the jobs.

Fixed the failure in the jobs, so we can land the patch.

bomsy reviewed May 8, 2024

View reviewed changes

takikawa force-pushed the add-spec-tests branch from 3db03cf to 4c10b94 Compare May 9, 2024 18:28

bomsy reviewed May 28, 2024

View reviewed changes

test/test-spec-tests.js Outdated Show resolved Hide resolved

bomsy reviewed May 28, 2024

View reviewed changes

test/test-spec-tests.js Outdated Show resolved Hide resolved

bomsy reviewed May 29, 2024

View reviewed changes

test/test-spec-tests.js Outdated Show resolved Hide resolved

takikawa added 6 commits June 6, 2024 09:25

Add draft source map spec tests

6fed3b2

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.

Add transitive mapping support in spec test harness

d5cb02c

Refine handling of null in the test harness

6396a24

* Don't do reverse lookup test if source is null * Allow "null" in cases where null is expected

Update source map tests repo to official one

2dea47e

Update skipped tests for ignoreList tests

3a33cd7

These aren't supported yet because the library doesn't support ignoreList yet and doesn't do any checking for it.

Address PR feedback

8afce96

takikawa force-pushed the add-spec-tests branch from c74f9c0 to 8afce96 Compare June 6, 2024 08:03

Updates to satisfy linter

64050e6

Also makes the linter ignore the submodule

bomsy changed the title ~~WIP: Add source map spec tests~~ Add source map spec tests Jun 7, 2024

bomsy reviewed Jun 7, 2024

View reviewed changes

test/test-spec-tests.js Outdated Show resolved Hide resolved

Update test/test-spec-tests.js

d243a04

Fix the require of the fs promises module

bomsy approved these changes Jun 9, 2024

View reviewed changes

bomsy merged commit 3cb92cc into mozilla:master Jun 9, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add source map spec tests #505

Add source map spec tests #505

takikawa commented Apr 27, 2024

bomsy May 8, 2024 •

edited

Loading

takikawa May 9, 2024

bomsy May 28, 2024

bomsy May 8, 2024 •

edited

Loading

takikawa May 9, 2024

bomsy commented May 8, 2024

takikawa commented May 9, 2024

bomsy May 28, 2024

bomsy May 28, 2024

bomsy commented May 30, 2024

takikawa commented Jun 4, 2024

takikawa commented Jun 6, 2024

codecov-commenter commented Jun 6, 2024

bomsy commented Jun 7, 2024

bomsy left a comment

bomsy commented Jun 9, 2024

	if (testCase.sourceMapIsValid)
	if (testCase.sourceMapShouldBeValid)

Add source map spec tests #505

Add source map spec tests #505

Conversation

takikawa commented Apr 27, 2024

bomsy May 8, 2024 • edited Loading

Choose a reason for hiding this comment

takikawa May 9, 2024

Choose a reason for hiding this comment

bomsy May 28, 2024

Choose a reason for hiding this comment

bomsy May 8, 2024 • edited Loading

Choose a reason for hiding this comment

takikawa May 9, 2024

Choose a reason for hiding this comment

bomsy commented May 8, 2024

takikawa commented May 9, 2024

bomsy May 28, 2024

Choose a reason for hiding this comment

bomsy May 28, 2024

Choose a reason for hiding this comment

bomsy commented May 30, 2024

takikawa commented Jun 4, 2024

takikawa commented Jun 6, 2024

codecov-commenter commented Jun 6, 2024

Codecov Report

bomsy commented Jun 7, 2024

bomsy left a comment

Choose a reason for hiding this comment

bomsy commented Jun 9, 2024

bomsy May 8, 2024 •

edited

Loading

bomsy May 8, 2024 •

edited

Loading