search

validator / htmlparser

The Validator.nu HTML parser https://about.validator.nu/htmlparser/
Other
56 stars 26 forks source link

Ensure testharness makes missing doctype name null #36

Closed sideshowbarker closed 3 years ago

sideshowbarker commented 4 years ago

This change ensures that the tokenizer sets the doctype name to null when the doctype name is missing in the input source.

Otherwise, without this change, the doctype name is set to the empty string — which doesn’t conform to the requirements in the HTML spec, and which causes us to fail 9 tests in the html5lib-tests suite.

This PR branch also includes a related change for the testharness.

Relates to https://github.com/validator/htmlparser/issues/35

The tests that are failing without this change are the following:

sideshowbarker commented 4 years ago

I believe there's another layer of bug / spec change in the tree builder on this topic.

OK — I guess we should have (add) a test to expose it? This is another area of the code where it’s not clear to me how it aligns with the relevant spec algorithms. But I somewhat just trust that if we are passing the existing tests, that means the implementation is functionally the same as spec prose. But of course that can only be as reliable if we’re confidant we have coverage for the spec requirements in the test suite…

sideshowbarker commented 4 years ago

I am assuming that the approval for the changes in this PR does not mean “Let’s go ahead and merge this to master”. Right? I suppose we should document the workflow somewhere, but I have thus far understand that in order to merge to master here, there are some other steps that need to take place on the Firefox side — for example, a patch submitted against a BMO bug, and a successful try run, etc. — and then if that all looks good, then you come back here and either merge the PR to master yourself, or comment here with an explicit OK.

hsivonen commented 3 years ago

I suppose we should document the workflow somewhere, but I have thus far understand that in order to merge to master here, there are some other steps that need to take place on the Firefox side — for example, a patch submitted against a BMO bug, and a successful try run, etc. — and then if that all looks good, then you come back here and either merge the PR to master yourself, or comment here with an explicit OK.

Yeah, there needs to be a BMO bug number, a successful try run, and, typically, test cases that Firefox CI runs. Since autoland on the Firefox side has a certain risk of backouts, I wait until a changeset makes it to mozilla-central before merging the corresponding changeset here.

Thanks for the patch and patience.