More test case suggestions from cdjot regressions - #137
Merged
Merged
Conversation
Contributor
|
Excellent work! Outstanding :) That said: |
Owner
|
According to the documentation, "Text in square brackets that is not a link or image and is followed immediately by an attribute is treated as a generic span," so I suppose it's a djot.js bug. I don't think there's an issue up for this, though. |
jgm
reviewed
Jul 19, 2026
jgm
left a comment
jgm
left a comment
Owner
There was a problem hiding this comment.
Some excellent cases here! I don't know if I want to adopt all of them, and some of them should be relocated if present at all; see the comments.
| ``` | ||
|
|
||
| Inline attributes after the closing delimiter attach to the | ||
| emphasis or strong element rather than wrapping it in a span: |
Owner
There was a problem hiding this comment.
Why would one think it wraps in a span? Span syntax is clearly defined as requiring square brackets...I'd be inclined to omit this. Indeed, I think I'd want to emit both cases involving attributes. If these are needed, they should go in the attributes.test file.
Comment on lines
+30
to
+48
|
|
||
| Inline attributes attach to the ins/del/mark element: | ||
|
|
||
| ``` | ||
| {+ins+}{.a} | ||
| . | ||
| <p><ins class="a">ins</ins></p> | ||
| ``` | ||
|
|
||
| ``` | ||
| {-del-}{.a} | ||
| . | ||
| <p><del class="a">del</del></p> | ||
| ``` | ||
|
|
||
| ``` | ||
| {=mark=}{.a} | ||
| . | ||
| <p><mark class="a">mark</mark></p> |
Owner
There was a problem hiding this comment.
Again, I think these go in the attributes section if they are needed anywhere. They simply illustrate a general behavior of attributes, nothing special about mark, insert, or delete.
Comment on lines
+149
to
+162
| Inline attributes on inline links go on the `<a>` element: | ||
|
|
||
| ``` | ||
| [a](b){rel=me} | ||
| . | ||
| <p><a href="b" rel="me">a</a></p> | ||
| ``` | ||
|
|
||
| ``` | ||
| [a](b){rel="me"} | ||
| . | ||
| <p><a href="b" rel="me">a</a></p> | ||
| ``` | ||
|
|
Comment on lines
+309
to
+311
| A literal `<` or `>` in a link destination must be HTML-escaped | ||
| in the emitted href/src so it can't break out of the attribute | ||
| context. The same applies to backslash-escaped `<`: |
Owner
There was a problem hiding this comment.
This is more a general point about HTML rendering than about parsing. I'd like these cases to center on parsing.
| <p><a href="x<y">a</a></p> | ||
| ``` | ||
|
|
||
| Inline attributes attach to the autolink's `<a>` element: |
| <p>H<sub>2 </sub>O</p> | ||
| ``` | ||
|
|
||
| Inline attributes attach to the sub/sup element: |
| c</code></p> | ||
| ``` | ||
|
|
||
| Inline attributes attach to the code element: |
The existing loose-list test confirms that a blank line inside a list makes every item in that list loose, but doesn't probe the boundary: that the rule applies only within the current list. A naive implementation may scan the whole document for blank lines, retroactively rendering an earlier tight list as loose because of a separate later loose list. This test pins the boundary behavior down.
Existing tests use the no-leading-space form `|---|---|`. There was no positive/negative pair locking down what happens with `| --- | --- |` (leading space after the opening bar). The two forms produce very different output — one promotes the previous row to a header, the other keeps it as a data row and emits the dashes as cell content — so this is a useful boundary to pin.
attributes.test exercises block attributes on most other block types (block quote, heading, hr, code block, ordered list) but no test covered <dl>. A naive implementation might leak the attribute spec into the first definition's content rather than attaching it to the wrapping <dl>; this pins the documented behavior.
The existing block-comment test demonstrates that continuation
lines must be indented but doesn't cover the negative case (what
happens without indentation). This is exactly the boundary an
implementation has to get right; without a test it's easy to
silently consume an unindented `{% ... %}` span as a comment, or
conversely to fail to recognize a properly indented one.
The alt-text flattening for references inside an image alt block isn't covered by existing tests — only inline emphasis and bare strings appear in alt-text examples. Both `[foo][]` and the `[foo][foo]` form should reduce to their label text inside the alt attribute rather than being emitted literally.
The spec disallows internal whitespace in a reference definition's URL but no test exercised that rule. The two cases users actually encounter — a CommonMark-style \"title\" suffix and stray text after the URL — both invalidate the refdef and leave any later [foo][] reference unresolved. Without this test, an implementation could silently truncate at the first space and accept the prefix as the URL.
Djot requires a blank line between a list item and a nested list. Without that blank line, an indented `-` is literal text on the parent item's line, not a sub-list marker. There was no existing test exercising this rule combined with a following blank-line-separated paragraph; an implementation that doesn't distinguish carefully can absorb that paragraph as lazy list continuation, producing a wildly wrong tree.
The existing hard-break tests always have a follow-up character after the escaped newline. A trailing backslash at end of input (no following line) is a different code path: the parser must emit the <br> on the way out without expecting more content. A naive implementation that defers the <br> emission until it sees the next character will silently drop it here.
Djot inherits CommonMark's rule that a line containing only whitespace is a blank line. para.test had no test where the "blank" line between paragraphs is actually a single space; the two visible-paragraph case is the most important scenario and catches an implementation that requires a literal empty line to end a block.
A `:` followed by end of input is the minimal definition list: both term and definition are empty. This was found via a fuzz crash in cdjot, where the parser read past the end of the input buffer trying to find content. Locking down the expected empty <dl>/<dt>/<dd> structure prevents that whole class of out-of-bounds reads from going undetected.
The minimal-content case for a fenced div wasn't covered. A blank line is valid block content but contributes nothing, so the result should be an empty `<div></div>`. This was originally found via a fuzz crash in cdjot where the parser read past the buffer end looking for content; pinning the expected empty-div output catches that whole shape of out-of-bounds-read bug.
A heading-marker line with no inline content contributes nothing to the heading body. There were tests for `##` introducing an empty heading and for multi-line heading continuation, but not for a bare `#` *between* two content lines. A naive implementation could either insert an empty line in the heading content or incorrectly close the heading at the empty marker.
Existing attribute tests cover quoted values containing arbitrary
characters but not the constraint on bare identifier/class names.
Without a test pinning this down, an implementation could accept
characters like `<` or `\"` in `{#id}` or `{.cls}` and emit them
verbatim into HTML attribute contexts — a clear injection vector.
The spec gives the same character set as unquoted key=value values
(ASCII alphanumerics, `_`, `:`, `-`); anything else must invalidate
the whole spec and render as literal text.
The bare-word case `hi{}` is tested, but spans and links go
through different attribute-consumption paths (the closing
`]` or `)` is the explicit anchor for the attribute). A naive
implementation might emit a stray `{}` as literal text after
the element when no attributes are present, or — worse —
fail to consume the braces at all. Pinning these silent
no-ops is cheap and forecloses both bugs.
Contributor
Author
|
Since this PR is rather big, I've trimmed it down to the undisputed tests. This way, it can get merged more quickly and I can open separate PRs for less clear cases after I've taken a closer look again. |
Owner
|
thanks! |
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
3 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.
While bringing cdjot into its current state, I added a test case for each problem I found that was not caught by djot.js's current test suite. The tests are LLM generated, but I consider them generally useful due to being real regression tests (although for a different implementation). Additional details in the commit descriptions.
Are you interested in having these kinds of tests or do you rather prefer a less comprehensive, more focused test suite?