Import tests from unofficial testing repo #4
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This goes in the mochitests for devtools/client/shared/source-map-loader
(use the upstream command and not the shortcut on my setup)
Tests are described in a single JSON file. Using a single file instead of one file per test makes it easier to load all the test descriptions in the harness.
In case test harnesses wish to print the test case name or define a function for each test case, and so on.
A version field like "3" is is accepted in some tools but not technically valid. For the purposes of spec tests, we specify it's invalid.
The invalid form is accepted by some implementations, so it would be worth putting that in as an additional test. Also add a missing comment in basic-mapping.js
Improves Firefox harness to check these new conditions as well, TODO to add it for WebKit
These can be used by test harnesses if desired, but mainly they are present to make it easier to understand what the intention of tests are.
The spec gives a limit of 32-bits for the values of VLQ fields. However, it's not very precise on how this limit is implemented. For example, it's not clear if the sign bit is included or not. For these tests, we have assumed for now that values exceeding 32-bits are invalid and makes the mapping invalid. In addition, we have assumed the sign bit should be included in the limit.
* Test when the mappings key does not map to a string * Test a large VLQ that should be valid and decode to within 32-bits
Reading the spec strictly, a mapping like ",," is not legal as an empty segment with zero fields is not legal. However, in practice implementations don't parse this very strictly.
Also remove extra commas
It's not explicitly spelled out in the spec (FIXME) but it doesn't make sense to have a null source and source content for a particular source entry, so it might make sense to make that invalid. It should likely be valid to have a null source entry as long as a source content exists.
Fixes some unintentional errors in formatting
It looks like this approach of having the same source appear in multiple sections doesn't really match with how tools process source maps. It would be better to have a multi-section test with concatenated sources. The spec should probably also be clear on whether, e.g., the sources lists should be mutually exclusive.
Unlike the previous test that had two sections, there is one section for each of the two original sources.
Adds a new test action "checkMappingTransitive" that takes mapping data and also a list of intermediate maps that should be queried. The intention is that a tool harness would, if it supports this action, look up a position starting at the generated source's map and resolve it iteratively through the intermediate maps until the original source is found. The particular test that is added is a minified JS generated source that has a source map for the minification, and then a source map for the TS to JS compilation step.
This test is similar to the previously added transitive mapping test but has an additional intermediate source map.
While it initially seems like this case should be invalid, I think there is a good argument to be made that it's valid. First, the spec doesn't explicitly rule it out and does allow the possibility of both being null. Second, in practice it does have a meaning for browser engines. For example, if you have a source map with null as a source, and also have a file named "null" present then the browser may load that as the reference source file.
With a non-null sources
Adds a sourcesContent field for an empty file
jkup
approved these changes
May 30, 2024
nicolo-ribaudo
approved these changes
May 30, 2024
There was a problem hiding this comment.
This is great! Given that the readme is clearly marking the tests as "work-in-progress" I think we can merge this, so that devtools can start pulling them in as part of their CI, and then we can keep iterating.
@abelkius mentioned some bugs in some .map files, if they are not intentional lets try to fix them as soon as possible to not block integration in Chrome/Firefox.
If we want to move something out of the readme we could introduce a file for the coverage info, but I don't think there is any harm in just leaving it the way it is.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
So far the source map tests have been developed in an unofficial repo at https://github.com/takikawa/source-map-tests. This PR imports the tests that have been developed there (still WIP) over to this repo, as we work to make the tests more official.
I've updated the README so that the testing info is in a sub-section. Would be happy to adjust the README if there's any feedback (maybe some of it should go in a separate markdown document?).