tests: update Events/07 test to clarify interpretation of tag end slashes#1046
Merged
fb55 merged 1 commit intofb55:masterfrom Dec 28, 2021
Merged
tests: update Events/07 test to clarify interpretation of tag end slashes#1046fb55 merged 1 commit intofb55:masterfrom
fb55 merged 1 commit intofb55:masterfrom
Conversation
1f049c5 to
0c6fcd1
Compare
vassudanagunta
added a commit
to vassudanagunta/htmlparser2
that referenced
this pull request
Dec 14, 2021
`Parser.ts` has special `onclosetag` logic for [`p`](https://github.com/fb55/htmlparser2/blob/6445c32b05dc070049eeaaf201bfc123daca3d37/src/Parser.ts#L336) and [`br`](https://github.com/fb55/htmlparser2/blob/6445c32b05dc070049eeaaf201bfc123daca3d37/src/Parser.ts#L340) tags. The updated tests in PR fb55#1046 were unable to expose any behavioral differences, despite the different code paths taken by the existing special logic. So I decided it would be best to at least to clarify with an update to the existing comment in the code, as well as by using the literal values 'p' and 'br' instead of the variable `name` (which is guaranteed to have those values).
Pull Request Test Coverage Report for Build 1594386588
💛 - Coveralls |
Owner
|
Hi @vassudanagunta, thanks for the great description, and you're completely right with your interpretation. Good catch on There is currently a linting issue ( |
…shes Enhancements to the `Events/07-self-closing.json` test to better cover and demonstrate the interpretation of tag end slashes (e.g. `/>`) `07-self-closing.json` is replaced with a set of tests 07a to 07h where the input and context are identical except for one isolated change. The outputs can thereby be diffed, showing the effect of the isolated change. Adds test coverage for the `recognizeSelfClosing` option.
0c6fcd1 to
81d3c47
Compare
Contributor
Author
|
No problem! I was evaluating htmlparser2 for a project of mine, a library that I also intend to publish for open source public consumption. I love your parse event-based test framework... If I have any question about how htmlparser2 works, all i have to do is create a simple JSON file with the input and config. Or copy an existing one and make one isolated change to see the behavior diff. Well done! |
Owner
|
Thanks @vassudanagunta |
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.
This PR contains suggested enhancements to the
Events/07-self-closing.jsontest.It also suggests a testing strategy that can be applied in other cases: a set of tests where the input and context are identical except for one isolated change. The outputs can thereby be diffed, showing the effect of the isolated change.
No worries if this isn't merged. Also happy to make any changes desired changes.
Test 07 enhancements
The original
07-self-closing.jsoncovered two cases:a tag ending in
/>but where the/is in fact part of the attribute value -- thus not a self-closing tagan HTML void element,
<hr>, in XHTML self-closing style.One flaw with this test is that
hris also a void element: While the test passes because the expectedclosetagevent occurs, one doesn't know if it occurs because of the end slash or because it is a void element.The test
07-self-closing.jsononly included a self-closing tag for a void element. This made the test ambiguous in purpose if you didn't know that htmlparser2 followed the above distinction.The new
07ato07htests cover much more:base name for all these tests is "end slash" rather than "self-closing" because what's being tested is how that end slash is interpreted in different contexts, e.g. as the mark of a self-closing tag, a part of the attribute, or ignored entirely.
each of the 8 tests represents one isolated change compared to one or more of the others:
xmlModeoff vs onrecognizeSelfClosingoff vs onThis allows one to diff the event streams to see the effect of that isolated change. For example, you can compare:
<hr/>generates aclosetagevent while<xx/>does not. The former is a void element and the latter is not.recognizeSelfClosingtest coverage and test demonstrated semanticsThis option was introduced in #74 but without any tests nor any documentation.
I confirmed there is no test coverage by:
Temporarily adding the following line to
Parser.ts's constructor:Ran the tests. No tests failed. The above change inverted the option for every test, and would have caused any test covering this option to fail.
I added coverage via case
07fThere is no clear documentation on the semantics of
recognizeSelfClosing, but based on the new07tests and diffing their outputs, I was able to surmise the following: