refactor: Improve code coverage #517
Conversation
| @@ -17,9 +17,6 @@ export function parseChunked<T extends TreeAdapterTypeMap>( | |||
| parserStream.parser.tokenizer.preprocessor.bufferWaterline = 8; | |||
|
|
|||
| for (let i = 0; i < chunks.length - 1; i++) { | |||
| if (typeof chunks[i] !== 'string') { | |||
There was a problem hiding this comment.
TypeScript validates this, we don't need this check.
| @@ -317,6 +317,8 @@ generateTestsForEachTreeAdapter('open-element-stack', (treeAdapter) => { | |||
| test('Has numbered header in scope', () => { | |||
| const stack = new OpenElementStack(treeAdapter.createDocument(), treeAdapter, stackHandler); | |||
|
|
|||
| assert.ok(stack.hasNumberedHeaderInScope()); | |||
There was a problem hiding this comment.
The empty states won't be used in the parser, but we can test them here.
There was a problem hiding this comment.
🤔 Why does hasNumberedHeaderInScope exist if parse5 doesn’t use it? Should we deprecate it and remove it in a major then?
There was a problem hiding this comment.
If these functions are used, can we instead add unit tests (input/output) of parsing behavior that depends on the result of these functions?
There was a problem hiding this comment.
hasNumberedHeaderInScope is used by the parser. The only thing that the parser cannot reach is the default return value, eg. for an empty stack.
There was a problem hiding this comment.
You mean this early return
causing to never run?Could you change that early return to a break in that case, to fix the coverage?
Not sure about the algorithm: but it seems off that the default for that function i true? I’d expect, with such a function name, to exit early with true, and otherwise return false?
There was a problem hiding this comment.
The relevant part of the spec is https://html.spec.whatwg.org/multipage/parsing.html#has-an-element-in-scope
The algorithm explicitly doesn't consider the possibility of an empty stack:
This will never fail, since the loop will always terminate in the previous step if the top of the stack — an html element — is reached.
There was a problem hiding this comment.
Then I recommend using a c8 ignore (or similar), with this as an explanation:
/* Note: the algorithm <link> never fails */
/* c8 ignore next 123 */
|
|
||
| await finished(parser); | ||
|
|
||
| assert.ok(foundText); |
There was a problem hiding this comment.
probably a good idea to use an assert.equal instead, to check that the actual result makes sense and not, say, 'null' for example?
| @@ -317,6 +317,8 @@ generateTestsForEachTreeAdapter('open-element-stack', (treeAdapter) => { | |||
| test('Has numbered header in scope', () => { | |||
| const stack = new OpenElementStack(treeAdapter.createDocument(), treeAdapter, stackHandler); | |||
|
|
|||
| assert.ok(stack.hasNumberedHeaderInScope()); | |||
There was a problem hiding this comment.
🤔 Why does hasNumberedHeaderInScope exist if parse5 doesn’t use it? Should we deprecate it and remove it in a major then?
| @@ -317,6 +317,8 @@ generateTestsForEachTreeAdapter('open-element-stack', (treeAdapter) => { | |||
| test('Has numbered header in scope', () => { | |||
| const stack = new OpenElementStack(treeAdapter.createDocument(), treeAdapter, stackHandler); | |||
|
|
|||
| assert.ok(stack.hasNumberedHeaderInScope()); | |||
There was a problem hiding this comment.
If these functions are used, can we instead add unit tests (input/output) of parsing behavior that depends on the result of these functions?
Removes all low-hanging fruit for #515