Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add source map spec tests #505

Merged
merged 8 commits into from
Jun 9, 2024
Merged

Add source map spec tests #505

merged 8 commits into from
Jun 9, 2024

Conversation

takikawa
Copy link
Contributor

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.

Comment on lines 21 to 53
// 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"
]
Copy link
Collaborator

@bomsy bomsy May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these skipped tests specific to the mozilla sourcemap implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Collaborator

@bomsy bomsy May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

bomsy commented May 8, 2024

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

@takikawa
Copy link
Contributor Author

takikawa commented May 9, 2024

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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (testCase.sourceMapIsValid)
if (testCase.sourceMapShouldBeValid)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
test/test-spec-tests.js Outdated Show resolved Hide resolved
test/test-spec-tests.js Outdated Show resolved Hide resolved
@bomsy
Copy link
Collaborator

bomsy commented May 30, 2024

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
Copy link
Contributor Author

takikawa commented Jun 4, 2024

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
Copy link
Contributor Author

takikawa commented Jun 6, 2024

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
Copy link

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.

@bomsy bomsy changed the title WIP: Add source map spec tests Add source map spec tests Jun 7, 2024
test/test-spec-tests.js Outdated Show resolved Hide resolved
@bomsy
Copy link
Collaborator

bomsy commented Jun 7, 2024

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

Fix the require of the fs promises module
Copy link
Collaborator

@bomsy bomsy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bomsy
Copy link
Collaborator

bomsy commented Jun 9, 2024

@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 bomsy merged commit 3cb92cc into mozilla:master Jun 9, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants