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

tests: update Events/07 test to clarify interpretation of tag end slashes #1046

Merged
merged 1 commit into from Dec 28, 2021

Conversation

vassudanagunta
Copy link
Contributor

This PR contains suggested enhancements to the Events/07-self-closing.json test.

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.json covered two cases:

  • a tag ending in /> but where the / is in fact part of the attribute value -- thus not a self-closing tag

  • an HTML void element, <hr>, in XHTML self-closing style.

    One flaw with this test is that hr is also a void element: While the test passes because the expected closetag event 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.json only 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 07a to 07h tests 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:

    • void vs non-void
    • with and without end slash
    • xmlMode off vs on
    • recognizeSelfClosing off vs on
    • end slash attached to unquoted attribute value

    This allows one to diff the event streams to see the effect of that isolated change. For example, you can compare:

    • 07a with 07d to see that <hr/> generates a closetag event while <xx/> does not. The former is a void element and the latter is not.
    • 07x with 07y to see the effect of xmlMode.

recognizeSelfClosing test coverage and test demonstrated semantics

This option was introduced in #74 but without any tests nor any documentation.

I confirmed there is no test coverage by:

  1. Temporarily adding the following line to Parser.ts's constructor:

    this.options.recognizeSelfClosing = !this.options.recognizeSelfClosing
    
  2. 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 07f

There is no clear documentation on the semantics of recognizeSelfClosing, but based on the new 07 tests and diffing their outputs, I was able to surmise the following:

🌶 recognizeSelfClosing really means "recognized all tags ending with /> as self-closing in normal HTML mode (as if in xmlMode), not just void element tags"

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).
@coveralls
Copy link

coveralls commented Dec 15, 2021

Pull Request Test Coverage Report for Build 1594386588

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.46%

Totals Coverage Status
Change from base Build 1592764720: 0.0%
Covered Lines: 737
Relevant Lines: 741

💛 - Coveralls

@fb55
Copy link
Owner

fb55 commented Dec 17, 2021

Hi @vassudanagunta, thanks for the great description, and you're completely right with your interpretation. Good catch on recognizeSelfClosing not being tested.

There is currently a linting issue (npm run lint will show it) — once that's fixed, I'm happy to merge this.

…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.
@vassudanagunta
Copy link
Contributor Author

vassudanagunta commented Dec 17, 2021

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!

@fb55 fb55 merged commit 11e48f6 into fb55:master Dec 28, 2021
@fb55
Copy link
Owner

fb55 commented Dec 28, 2021

Thanks @vassudanagunta

@vassudanagunta vassudanagunta deleted the clarifying-tests branch December 29, 2021 05:21
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.

None yet

3 participants