@sideshowbarker, does this reflect the new expected process for tests? I noticed that https://github.com/validator/tests is archived even though it’s still mentioned in .gitsubtrees.yaml.

@sideshowbarker, does this reflect the new expected process for tests?

Yeah — in that the tests should be added in the tests subdirectory.

I noticed that validator/tests is archived even though it’s still mentioned in .gitsubtrees.yaml.

Yeah, that’s all intentional. The original upstream repo which the tests came from is now read-only. And the inclusion in .gitsubtrees.yaml is intentional — but only for the purpose of keeping a record of where the tests originally came from.

One thing I had already been meaning to mention to you: Please see https://github.com/validator/validator/blob/main/CONTRIBUTING.md#commit-hooks for some instructions on how you can set up your local environment so that some git “pre-commit” checks in your local environment every time you commit.

If you got through those steps, then among the checks that will do is, it’ll check to make sure you’ve also updated the tests/messages.json file. Which you didn’t do yet in the patch for this PR — but you can do it by running ./checker.py make-messages, after you’ve added any tests.

And in the case of this particular PR, what that will specifically do is, it’ll add a message there for the tests/html/elements/h1/multiple-h1-haswarn.html test.

One thing I had already been meaning to mention to you: Please see https://github.com/validator/validator/blob/main/CONTRIBUTING.md#commit-hooks for some instructions on how you can set up your local environment so that some git “pre-commit” checks in your local environment every time you commit.

Set up.

If you got through those steps, then among the checks that will do is, it’ll check to make sure you’ve also updated the tests/messages.json file. Which you didn’t do yet in the patch for this PR — but you can do it by running ./checker.py make-messages, after you’ve added any tests.

[…] Seems to have an effect now—yes, set up!

This seems to have broken some existing tests:

"file:/Users/mike/workspace/validator/tests/html/attributes/headingoffset-headingreset-isvalid.html":3:4: warning: Consider using only one “h1” element per document (or, if using “h1” elements multiple times is required, consider using the “headingoffset” attribute to indicate that these “h1” elements are not all top-level headings).
"file:/Users/mike/workspace/validator/tests/html/attributes/headingoffset-headingreset-isvalid.html":13:19: warning: Consider using only one “h1” element per document (or, if using “h1” elements multiple times is required, consider using the “headingoffset” attribute to indicate that these “h1” elements are not all top-level headings).
"file:/Users/mike/workspace/validator/tests/html/attributes/popover-isvalid.html":5:4: warning: Consider using only one “h1” element per document (or, if using “h1” elements multiple times is required, consider using the “headingoffset” attribute to indicate that these “h1” elements are not all top-level headings).
"file:/Users/mike/workspace/validator/tests/html/attributes/popover-isvalid.html":21:21: warning: Consider using only one “h1” element per document (or, if using “h1” elements multiple times is required, consider using the “headingoffset” attribute to indicate that these “h1” elements are not all top-level headings).
"file:/Users/mike/workspace/validator/tests/html/elements/h1/model-isvalid.html":15:6: warning: Consider using only one “h1” element per document (or, if using “h1” elements multiple times is required, consider using the “headingoffset” attribute to indicate that these “h1” elements are not all top-level headings).
"file:/Users/mike/workspace/validator/tests/html/elements/h1/model-isvalid.html":12:6: warning: Consider using only one “h1” element per document (or, if using “h1” elements multiple times is required, consider using the “headingoffset” attribute to indicate that these “h1” elements are not all top-level headings).
"file:/Users/mike/workspace/validator/tests/html/elements/h1/model-isvalid.html":9:38: warning: Consider using only one “h1” element per document (or, if using “h1” elements multiple times is required, consider using the “headingoffset” attribute to indicate that these “h1” elements are not all top-level headings).

So, either those existing tests need to be renamed to -haswarn.html or else they need to fixed in some way, or else the change in this PR needs to be further refined.

Anyway, in the meantime I’m reverting this change — until we get the problems figured out and fixed.

@sideshowbarker, what happened, why did local tests and CI pass?

Your call on that but is there or should there be a ticket to make sure these tests cannot be overlooked?

And, should we reopen #813?

@sideshowbarker, what happened, why did local tests and CI pass?

What happened is that this change was causing new warnings to be emitted for -valid.html — but that didn’t cause CI to fail. And that’s because the test runner, as currently written, doesn’t treat warnings for -valid.html files as as failure. The test runner still exits zero in that case. So that’s why CI didn’t catch it, and we didn’t catch it.

Your call on that but is there or should there be a ticket to make sure these tests cannot be overlooked?

I need to update the test runner to make it fail for “new warnings for -valid.html” cases. But no issue needed.

And, should we reopen #813?

We can’t re-open that. Instead, we need a new PR, but this time either with -haswarn renames made to the “problem” tests sources, or else with changes made to the Java code to prevent the warnings for those cases. I mean, if they actually are valid tests and should not be causing warnings.

Your call on that but is there or should there be a ticket to make sure these tests cannot be overlooked?

I need to update the test runner to make it fail for “new warnings for -valid.html” cases. But no issue needed.

Fixed that part now.

We can’t re-open that. Instead, we need a new PR, but this time either with -haswarn renames made to the “problem” tests sources, or else with changes made to the Java code to prevent the warnings for those cases. I mean, if they actually are valid tests and should not be causing warnings.

Do you like to take care of that? Would otherwise raise a hand but I get the idea that with this situation, that’s the better call? (I can probably pick up a separate issue again shortly.)