Vale test suite

[osfameron]

Workflow:

For styles.js

• edit test/adoc/Test-Couchbase-*.yml
• npm test
• view test/adoc/styles.html for details
• rinse and repeat

For expected.js

• add test/fixtures/foo.adoc
• touch test/fixtures/foo.adoc.expected
• npm test
• npm run accept # when you're happy with the new output

[sarahlwelton]

Did you mean to commit the package-lock.json and package.json?

[osfameron]

yes, they contain the dependencies for node to be able to run the tests 👍

[osfameron]

Adding @RayOffiah as he may have some ideas on how to make this nicer / exercise the more complicated cases within Asciidoc!

Requested review from @sarahlwelton - is still a sketch really, but I think worth you playing with it at this point.

[osfameron]

Did a spot of work over weekend, and backed away from specifying the tests in Asciidoc - it felt like it was adding extra complication, without too many advantages.

Also it's restructured, so runs vale just once, making it much faster.

(@RayOffiah if you have thoughts on doing it within Asciidoc, happy to discuss!)
But I think removing that dependency is sensible as a design, as it makes it potentially a useful thing to contribute back to Vale, if anyone else wants it.
You can choose what extension to use for specific sections etc. - look at e.g. the Headings rule tests.

[osfameron]

Status: npm test side of things works great, and gives slightly improved diagnostic output on error.

Now that I don't have Asciidoc builtin, I haven't yet reinstated the pretty HTML output - working on this today.

[osfameron]

Now ready for review again with the directory of YAML tests and a new HTML output.

[osfameron]

@sarahlwelton check the styles.html to look for yourself, but here's a summary of the checks that "failed".
e.g. either because something is wrong, or there's an edge case missing, or I just couldn't figure it out...

• Couchbase.CouchbaseAutoOp
  shouldn't this flag "The Operator supports rolling upgrades. We call it the Couchbase Autonomous Operator." as "Operator" is mentioned before the full name?
• Couchbase.DirectionalLanguage "Move up to the previous section." not flagged
• Couchbase.Dropdown doesn't flag "dropdowns"
• Couchbase.Italics just doesn't work? (odd, as it's more or less the same as Couchbase.Bold which is fine)
• Couchbase.LessFewer doesn't flag "less people"
• Couchbase.Link I couldn't figure out how to flag
• Couchbase.ReadingLevel (seems a bit flaky, it did work, but seems to require VERY obtuse language to do so)
• Check: Couchbase.RepeatedWord missed "Please please check the document"
• Couchbase.UnexplainedAcronym missed "The HFE is not responding"

[sarahlwelton]

@sarahlwelton check the styles.html to look for yourself, but here's a summary of the checks that "failed". e.g. either because something is wrong, or there's an edge case missing, or I just couldn't figure it out...

• Couchbase.CouchbaseAutoOp
  shouldn't this flag "The Operator supports rolling upgrades. We call it the Couchbase Autonomous Operator." as "Operator" is mentioned before the full name?
• Couchbase.DirectionalLanguage "Move up to the previous section." not flagged
• Couchbase.Dropdown doesn't flag "dropdowns"
• Couchbase.Italics just doesn't work? (odd, as it's more or less the same as Couchbase.Bold which is fine)
• Couchbase.LessFewer doesn't flag "less people"
• Couchbase.Link I couldn't figure out how to flag
• Couchbase.ReadingLevel (seems a bit flaky, it did work, but seems to require VERY obtuse language to do so)
• Check: Couchbase.RepeatedWord missed "Please please check the document"
• Couchbase.UnexplainedAcronym missed "The HFE is not responding"

In order...

• Couchbase.CouchbaseAutoOp has always been really flaky and I'm not sure if it's ever really worked. We might need to dig deeper to see if we can figure out how to get it working.
• DirectionalLanguage accepts "up to" as in, "up to version x". We'll need to modify the regex expression if we want "move up to" to get flagged.
• Dropdown wasn't pluralized, so that's probably why.
• I'd want to investigate why Italics doesn't work as we want to make sure people aren't using italics. Would need to look into the regex.
• Again, will need to tweak the Regex as LessFewer only checks for plurals, from what I remember of the regex. It looks for a word followed by s, not a plural noun like "people"
• Regex could be tweaked for Link - it does flag if written as something like "Click the Learn More link next to the ..."
• ReadingLevel wasn't written by me. It was a standard metrics rule that we adopted. I have managed to trigger it in the past.
• Vale.Repetition catches Please please - perhaps it's because of the capital that RepeatedWord doesn't catch that.
• UnexplainedAcronym is a similarly flaky test that I feel we need to tweak the regex for. Most of the conditional rules have been very flaky and I've struggled to get them to work as described in the Vale docs. Open to suggestions.

[osfameron]

@sarahlwelton thanks, that's all useful context!
As I mentioned, I got Copilot to generate the test cases, so some of the failures it found are interesting edge-cases, as I don't think I'd persuaded it to write to exactly the regexes.

So I think the next iterations are:

• update regexps to handle edge cases found
• fix the Link fixture to trigger the existing rule (and possibly add other cases. From a quick scan, it did look like there were too many possible variants for this to be a good regex rule?)
• Investigate the conditional and ReadingLevel rules further

Then

• flag any remaining failures as TODO (e.g. expected failures) so that the tests all "pass" with exceptions
• Github Action test on update to rules

[sarahlwelton]

Why did we comment out the scope setting?

[osfameron]

oh, driveby hack hack hack hack...
and it seems not to be necessary? (At least according to the minimal test cases)

Happy to revert, but we might as well make it part of the test: can you provide a counterexample that it's needed for?

[sarahlwelton]

If it works without it, don't worry about it - was just curious.

I can't find the documentation around the text scope now to figure out what it's supposed to limit.

[sarahlwelton]

Glad to see it seems to be working, here.

[osfameron]

Gah! 😱
So, this was failing on Github runner, because I installed Vale as a "Snap",
and Snap packages can't read the OS tempdir, by design... 🤦

We now use $RUNNER_TEMP instead, and everything works OK.
Re-requested review from @sarahlwelton.

[osfameron]

Updated to integrate with main and update to add new tests.
FYI @sarahlwelton @zeba-shireen @TimLFletcher perhaps I can do brief show-and-tell at next process meeting.