chore: Chore add VNU GitHub Action by nschonni · Pull Request #1206 · mdn/content

nschonni · 2021-01-12T09:01:56Z

As and alternate to mdn/yari#418
Downside to this approach is that there is no local runnable version of the testing without running the replacement that touches ~400 files

nschonni · 2021-01-12T09:08:42Z

@Elchi3 FYI, the issue it's failing on was introduced in mdn/yari#1106

Elchi3 · 2021-01-12T09:29:20Z

@nschonni not sure I follow. What did I break? :) Can you elaborate?

Elchi3 · 2021-01-12T09:57:06Z

ahh I see it now.

Error: "file:/home/runner/work/content/content/files/en-us/web/html/global_attributes/enterkeyhint/index.html":23.112-23.115: error: No “p” element in scope but a “p” end tag seen.

Yeah, it would be great if CI or any tests would have caught this for me. I'll try to see if I can add an HTML validator to my editor at least.

Elchi3 · 2021-01-12T11:40:34Z

Opened #1210 which should fix it.

peterbe · 2021-01-12T13:53:52Z

I'm not sure. This feels like a hack. And adding vnu-jar feels very heavy. Also, this kind of stuff feels more appropriate to do with Yari if anything. https://github.com/mdn/content/runs/1686915136 was cool though.
There's just something that doesn't feel great about running the content through a HTML validator. It's not really HTML. It's Kumascript-HTML if anything. Perhaps it would make more sense to do a string replace on the rawHTML and replace it with something like <ignore data-macro="..."/> and then run it.
Besides, the whole sed stuff feels weird and if we're going to do this properly, I think we should do it properly and with more care and time. Sorry for sounding a bit of a downer. I'm also hesitant to invest more into the HTML validation when we could use the limited time and energy investing into something Markdownish.

sideshowbarker · 2021-01-12T16:21:25Z

I'm also hesitant to invest more into the HTML validation when we could use the limited time and energy investing into something Markdownish.

Strongly agree. (And that coming from a developer of an HTML validator…) Feels like this might be a bit of an XY problem https://en.wikipedia.org/wiki/XY_problem.

It’d be great if a plan and proposed schedule for moving to something Markdown-ish were put together.

That said, if/when we do move to something Markdown-ish, does that means we necessarily mass-convert all the existing sources? Or instead would it be that — at least in the near term — we’d continue HTML file in the tree, mixed with some Markdown files?

peterbe · 2021-01-12T18:52:24Z

I'm also hesitant to invest more into the HTML validation when we could use the limited time and energy investing into something Markdownish.

Strongly agree. (And that coming from a developer of an HTML validator…) Feels like this might be a bit of an XY problem https://en.wikipedia.org/wiki/XY_problem.

Never heard that one before. Thanks!

It’d be great if a plan and proposed schedule for moving to something Markdown-ish were put together.

I hear ya, but that's going to be hard. Sorry. The reason is that we predict it'll be a slow progress since we first need to do a bunch of exploration, research, and prototyping. Within that work, we'd get a better handle on compromises and trade-offs that we need to counter for the fact that we have so much legacy and weird content.

That said, if/when we do move to something Markdown-ish, does that means we necessarily mass-convert all the existing sources? Or instead would it be that — at least in the near term — we’d continue HTML file in the tree, mixed with some Markdown files?

I suspect we won't. It'd probably be gentle and almost a page-by-page. Or sub-tree by sub-tree.

Ryuno-Ki · 2021-01-19T20:57:05Z

That said, if/when we do move to something Markdown-ish, does that means we necessarily mass-convert all the existing sources?

I feel like this should be moved into another issue, which could be linked to to collect all the comments that contain bits of what would be needed.

nschonni · 2021-01-24T21:41:22Z

@peterbe this continues to find issues as I rebase this occasionally. To make this more of a CI only thing, I could drop the package install and just "NPX" run it on the CI only

nschonni · 2021-04-19T21:34:31Z

How about making it part of the regular Yari runner?

I don't think the complication of trying to add it to Yari is worth it. This job will go away at the end of your markdown conversion. This just keeps the files in better shape for editors (and the markdown conversion tool) till you eventually cut over.

It could even be part of the flaw system even though it's run on the source HTML file, not the built HTML.

Think @sideshowbarker mentioned before that this works best in "batch mode" like we're calling it here, vs individual invokations that each take a Java start-up hit.

I'd rather the CI doesn't become red ❌ unless there's important things that really ought to be fix now.

HTML validation issues are important, as either they creating rendering issues, or the browser is swallowing the issue, but may stop doing after an unrelated change on the page

So you got it working?? You had said: "and it's not going to work as it can't properly report the annotations back to the PR."

It's always been working as this current separate PR job. It didn't work when I tried to move it to a workflow trigger as part of the PR Companion. It's possible that way is workable with annotations, but you're fighting the built-in integration that Actions already has with the problem matchers. The implications for the other PR companion stuff is tracked in mdn/fred#955, and probably better to discuss there

nschonni · 2021-04-20T00:32:21Z

Sorry, this has been going for awhile (started in another PR), so I forgot the "gotcha" for someone running locally, is that they'd get some "false positives" from the elements in the frontmatter on some pages.
CI modifies the local tree in place to escape that, but I didn't think it was "safe" to add that to the locally NPM script, so people don't accidentally change a large chunk of files.

nschonni · 2021-05-03T15:26:58Z

@peterbe ping

peterbe · 2021-05-03T20:13:43Z

@peterbe ping

So how do we test this? My fear is that false positives (where VNU incorrectly complains about a file the user didn't even touch) leads to a PR being failingly red when the user didn't introduce the problem. Perhaps they just touched a file that's always had an HTML validation problem.

Will yarn vnu-jar exit non-zero if there's a index.html that has a validation problem? Or will it just post a comment within the PR?

Perhaps butter me up and/or we put in some things to work around possible false positives and then we merge this and see how it goes.

peterbe · 2021-05-03T20:14:31Z

    "node": ">=12.0.0"
  },
  "scripts": {
+    "vnu-jar": "java -jar node_modules/vnu-jar/build/dist/vnu.jar --errors-only --filterfile .vnurc --no-langdetect --skip-non-html files/",


How does it know which files to test? Will it test all 11k each time?

Tests them all in a batch each time, but it's around 3 minutes in the CI. User doesn't have to run it locally unless they want to

Seems here https://github.com/mdn/content/pull/1206/checks?check_run_id=2493264692 it only took 1m 32s.
23 seconds was the time it took to run the actual yarn vnu-jar.

Will this delay when the PR Review Companion starts to run? I know that one waits for the PR Test workflow.

Nope, this runs in parallel with the other jobs AFAIK. PR companion is waiting for the "PR Test" to finish, not all other parallel jobs

nschonni · 2021-05-03T20:18:50Z

My fear is that false positives (where VNU incorrectly complains about a file the user didn't even touch) leads to a PR being failingly red when the user didn't introduce the problem.

If the error is with the file that is touched by the PR, it will show up on the regular files as inline annotations. If it's an issue with something they didn't touch, then GitHub pushes them to the bottom of the Files tab.
Either way, as we tested with the "System file changes" job, a contributor can always land even with the job failing if they don't want to bother a new contributor 😄

peterbe · 2021-05-03T20:48:33Z

If the time it takes to run this is too slow, I don't want the reviewer (with merge access) to have to sit around impatiently waiting for the "HTML Linting" to go green whilst it's checking unrelated files. If that's the case we can optimize this workflow to just test the files/**/index.html files that were actually touched.

Ryuno-Ki · 2021-05-03T20:48:36Z

If I understand that correctly, the failing test requires admin permissions to resolve?

peterbe · 2021-05-03T20:48:56Z

If I understand that correctly, the failing test requires admin permissions to resolve?

No, just write access.

peterbe · 2021-05-03T20:55:21Z

Let's try it! We'll keep an eye on it to see what things it's finding and what might not be great and we can iterate from there.

nschonni · 2021-05-03T20:56:01Z

If I understand that correctly, the failing test requires admin permissions to resolve?

PS: @Ryuno-Ki we did some testing around that in #3728

peterbe · 2021-05-03T21:00:42Z

@nschonni To be honest, I don't know how to test this.
I check out your branch and run

yarn vnu-jar

I get pages and pages of warnings and then it exits non-zero.

How come it's not failing here in this PR?

nschonni · 2021-05-03T21:07:29Z

How come it's not failing here in this PR?

Right, the CI is doing this to escape the front matter https://github.com/mdn/content/pull/1206/files#diff-c6f76479cefbaa8eea1ca9824164c77d44d8b981fbd44ae665e36cb09e6951f2R45

peterbe · 2021-05-03T21:27:38Z

How come it's not failing here in this PR?

Right, the CI is doing this to escape the front matter https://github.com/mdn/content/pull/1206/files#diff-c6f76479cefbaa8eea1ca9824164c77d44d8b981fbd44ae665e36cb09e6951f2R45

Oh so it rewrites the index.html files first. Then you'd get different results.

nschonni · 2021-05-03T21:29:13Z

It only re-writes the frontmatter lines in the CI, so the lines/colums it reports here are still correct

nschonni · 2021-05-11T02:43:48Z

@peterbe #4901 fixes the latest round of issues this flagged so this can be landed before/after that one

peterbe · 2021-05-11T12:55:28Z

@nschonni Dang! Now that I wake up from my snooze, your CI tests are failing. Sorry.

nschonni · 2021-05-11T15:09:52Z

@peterbe that PR was landed, rebased and needed to submit #4916 for the new ones since last night
I suggest just landing this and cleaning up things as they come in

nschonni · 2021-05-11T16:26:32Z

@peterbe and there were more between landing the latest and rebasing 😄
I'll submit a cleanup PR after if you land this

peterbe · 2021-05-11T16:43:34Z

@peterbe and there were more between landing the latest and rebasing 😄
I'll submit a cleanup PR after if you land this

I did see the PR you submitted recently. Great catches! That kind of stuff would have been prevented with this as part of CI. Right?

nschonni · 2021-05-11T17:18:09Z

Yup, that's what this catches, but doesn't block a PR from landing if a maintainer is OK with cleaning it up after

nschonni · 2021-05-12T20:55:11Z

@peterbe any remaining concerns?

peterbe · 2021-05-13T10:46:26Z

@peterbe any remaining concerns?

Can you merge in main one more time to assert that all errors will go away.

Also, I don't understand your point about being able to merge even if the line CI fails. Won't that just ruin CI for the next person making a PR if you merge it.

nschonni · 2021-05-13T12:38:09Z

@peterbe rebased again
Yes the next persons CI might go red, but I've said I'd submit another PR to clean it up (like the 31 others I submitted while this has been open https://github.com/mdn/content/pulls?q=is%3Apr+html+validation+is%3Aclosed+author%3Anschonni)

peterbe · 2021-05-13T14:05:36Z

I don't like...

This runs for every file and not just the ones touched.
The complicated sed command doesn't work on any OS.
That this whole thing relies on changing the file which makes it problematic to run locally

nschonni#1 solves 1 and 2 of that list.

What's so weird is that if someone, a reviewer, says "Fine. I'll let this pass" it'll poison every other PR going forward.
It also just feels strange to run something like vnu-jar on all ~12k files. It could maybe make sense in a push workflow or as a cronjob.

peterbe · 2021-05-13T14:07:25Z

nschonni#1 solves 1 and 2 of that list.

Perhaps I rushed that. The front-matter doesn't actually need to escaped. It just needs to be deleted, right?
Using that bodyBegin attribute from fm() we could turn...

---
foo: "bar"
---
<p>Here it starts</p>

...into...

<p>Here it starts</p>

the line number still make sense in the problem matcher and front-matter could never matter.

nschonni · 2021-05-13T14:07:28Z

OK, I give up. @sideshowbarker I'll leave this to you to work out internally

peterbe · 2021-05-13T14:08:12Z

OK, I give up. @sideshowbarker I'll leave this to you to work out internally

Please don't. If you agree and can incorporate my PR I think this is good to go.

nschonni requested a review from peterbe as a code owner January 12, 2021 09:01

nschonni requested a review from a team January 12, 2021 09:01

Elchi3 mentioned this pull request Jan 12, 2021

Add missing <p> to enterkeyhint page #1210

Merged

chrisdavidmills closed this in #1210 Jan 12, 2021

Elchi3 reopened this Jan 12, 2021

nschonni force-pushed the vnu-jar branch 3 times, most recently from b2ad410 to 115c117 Compare January 14, 2021 19:27

nschonni mentioned this pull request Jan 14, 2021

fix: Validation issues #1323

Merged

nschonni force-pushed the vnu-jar branch 5 times, most recently from 4c9d18e to 6add6b9 Compare January 19, 2021 17:23

nschonni force-pushed the vnu-jar branch 3 times, most recently from 87ef826 to 4ee6a51 Compare January 24, 2021 20:59

nschonni force-pushed the vnu-jar branch 4 times, most recently from 8fda94d to 4befd17 Compare February 2, 2021 05:19

peterbe reviewed May 3, 2021

View reviewed changes

chore: Add html validator CI

3519964

peterbe mentioned this pull request May 13, 2021

use get-diff-action nschonni/mdn-content#1

Closed

Uh oh!

Conversation

nschonni commented Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nschonni commented Jan 12, 2021

Uh oh!

Elchi3 commented Jan 12, 2021

Uh oh!

Elchi3 commented Jan 12, 2021

Uh oh!

Elchi3 commented Jan 12, 2021

Uh oh!

peterbe commented Jan 12, 2021

Uh oh!

sideshowbarker commented Jan 12, 2021

Uh oh!

peterbe commented Jan 12, 2021

Uh oh!

Ryuno-Ki commented Jan 19, 2021

Uh oh!

nschonni commented Jan 24, 2021

Uh oh!

nschonni commented Apr 19, 2021

Uh oh!

nschonni commented Apr 20, 2021

Uh oh!

nschonni commented May 3, 2021

Uh oh!

peterbe commented May 3, 2021

Uh oh!

peterbe May 3, 2021

Choose a reason for hiding this comment

Uh oh!

nschonni May 3, 2021

Choose a reason for hiding this comment

Uh oh!

peterbe May 3, 2021

Choose a reason for hiding this comment

Uh oh!

nschonni May 3, 2021

Choose a reason for hiding this comment

Uh oh!

nschonni commented May 3, 2021

Uh oh!

peterbe commented May 3, 2021

Uh oh!

Ryuno-Ki commented May 3, 2021

Uh oh!

peterbe commented May 3, 2021

Uh oh!

peterbe commented May 3, 2021

Uh oh!

nschonni commented May 3, 2021

Uh oh!

peterbe commented May 3, 2021

Uh oh!

nschonni commented May 3, 2021

Uh oh!

peterbe commented May 3, 2021

Uh oh!

nschonni commented May 3, 2021

Uh oh!

nschonni commented May 11, 2021

Uh oh!

peterbe commented May 11, 2021

Uh oh!

nschonni commented May 11, 2021

Uh oh!

nschonni commented May 11, 2021

Uh oh!

peterbe commented May 11, 2021

Uh oh!

nschonni commented May 11, 2021

Uh oh!

nschonni commented May 12, 2021

Uh oh!

peterbe commented May 13, 2021

Uh oh!

nschonni commented May 13, 2021

nschonni commented Jan 12, 2021 •

edited

Loading