Correct edge cases in mappings, adding property-based smoke tests to verify#103
Open
jonathonherbert wants to merge 7 commits intojsh/data-based-specs-for-position-testsfrom
Open
Conversation
to make them simpler and more comprehensible ... but although the tests pass, they are not reversible, which stretches my mental model a bit ... I need to understand better why this wasn't a problem before
…which makes the behaviour clearer I was testing an inverted mapping, making changes unintuitive D:< Having said that, I'm not quite sure why getFieldValueRanges requires a negative oldSize rather than a positive toSize for its chipValue end, which is something I arrived at via experimentation rather than a complete understanding ... dislike
cleaning up the ranges to remove the odd inversion, and expanding the property-based tests to verify they are correct
Contributor
Contributor
jonathonherbert
commented
Nov 23, 2025
| return docToCqlStr(newState.doc); | ||
| }; | ||
|
|
||
| export function* pseudoRandom(seed: number): Generator<number, number, number> { |
Collaborator
Author
There was a problem hiding this comment.
For deterministic tests.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
(Split from #99 to make the test-based changes easier to review.)
What does this change?
Corrects some inconsistencies in the mappings we generate to map between queryStr and ProseMirror documents. These do not cause problems at the moment, but reveal themselves if we rely on mappings to determine where suggestions should appear, as in #99.
Problems with mappings have historically been difficult to spot and debug. This PR adds tests that use ProseMirror's test builder classes to add markers at the beginning and end (or, in the case of empty nodes, the empty content position) of nodes, which correspond with markers added to a query string. So, for example, we expect the positions given in the angle brackets below:
... to match exactly. This should make it easy to express the structure that produces a failing case.
Because mapping problems have been caused by unexpected combinations of nodes and content, the tests include some property tests based on pseudo-random combinations of nodes and content. This should give us confidence that the mappings are accurate and we've flushed out edge cases.
Finally, we round-trip the mapping from queryStr -> doc -> queryStr, to make sure that positions are reversible.
How to test
How can we measure success?
Confidence that our mapping from queryStr to ProseMirror doc, and vice-versa, is correct.