Page MenuHomePhabricator

Ensure all AbuseFilter variables are part of at least one unit test
Open, Needs TriagePublic

Description

In dealing with https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/201086/ we realized how easy it is for someone to define a new variable for AbuseFilter but not create a pertinent unit test for it.

Rather than relying on coders to keep that in mind, it is ideal if we had a Jenkins task that would fail if there exists a variable that is not part of any unit tests.

@Legoktm suggested in the latest comment on that patch an approach:

Write a unit test that iterates over the list of variables and checks whether $this->assertTrue( file_exists( tests/parserTests/$variable ), "missing test for $variable" )? My pseudo-code is bad but you get the idea :)

However, since variables can also be added by other extensions (e.g. StructuredDiscussions currently adds some variables) this may not be a final solution and needs more thought.

Event Timeline

We definitely need something like this. A bunch of thoughts:

  1. This shouldn't be in place only for variables, but also for functions, keywords, operators... Basically everything from AbuseFilter::$builderValues (so we can also iterate this property)
  2. There's no relation between tests in the parserTests folder and builder values coverage: variables are not covered in those tests, and some of the tests don't cover a specific function/keyword. Instead, it would be great if we could copy what coverage tests do, i.e. check if a specific function is executed.
  3. We have too many external variables to test them within AF tests. Those should be tested in the extension where they are generated, and I think we can only suggest other devs to do it on their extension.