Page MenuHomePhabricator

Test XHPAST linter rules in isolation
ClosedPublic

Authored by joshuaspence on Aug 30 2015, 3:13 AM.

Details

Summary

Separate XHPAST linter rules in isolation from other rules and formalize the concept of a "linter rule". As the number of XHPAST linter rules grows (we currently have around 80), it becomes increasingly difficult to manage all of the test cases because ArcanistXHPASTLinterTestCase currently tests the entire linter (i.e. all linter rules) rather than testing individual rules in isolation. See D13534 for a situation in which this is painful. This is particularly bad for third party development because unit tests could break at any time depending on upstream changes.

Basically, in order to facilitate the unit testing of XHPAST linter rules in isolation, I have made the following changes:

  1. Added a setRules() method to ArcanistXHPASTLinter. Currently, ArcanistXHPASTLinter loads all ArcanistXHPASTLinterRule subclasses unconditionally. The setRules() method provides a way to override the configured linter rules.
  2. Formalize the concept of a "linter rule". The ArcanistXHPASTLinterRule class was introduced in D10541. I feel that the modularization of ArcanistXHPASTLinter has made the linter much more maintainable and easily testable and I intend to extend this concept to other linters, such as ArcanistTextLinter.
Test Plan

Ran unit tests.

Diff Detail

Repository
rARC Arcanist
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

joshuaspence retitled this revision from to Test XHPAST linter rules in isolation.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
joshuaspence edited edge metadata.

Almost working...

@epriestley, apologies for the large diff. 95% of this diff is either:

  • Moving test data from src/lint/linter/__tests__/xhpast/ to src/lint/linter/xhpast/rules/__tests/.
  • Boilerplate code for implementing ArcanistXHPASTLinterRule.

The only substantial code changes are the following classes/files:

  • .editorconfig
  • ArcanistXHPASTLinter
  • ArcanistLinterTestCase
  • ArcanistXHPASTLinterRule
epriestley edited edge metadata.

It might be vaguely nice to put these in some directory like:

.../__tests__/xhpast/data/XHP123/test1.lint-test

...and then use the directory (XHP123) as a code to select which lint messages to allow, then run all the directories as one test case. Then you can add a new rule test by just adding a directory instead of also having to add a class for it, and don't need to do the "guess the rule from the class name" stuff.

You may not be able to map rules cleanly to codes, but you could just run all the rules, then throw away the messages with a different code (instead of running only relevant rules).

Alternatively, rule stuff could be in some standard rules.json in the test directories as a sort of middle ground (more work than just directories but less work than creating classes).

I don't think any of these approaches are hugely better than the others, just proving that I'm reviewing. :)

This revision is now accepted and ready to land.Nov 18 2015, 9:36 PM

It might be vaguely nice to put these in some directory like:

.../__tests__/xhpast/data/XHP123/test1.lint-test

...and then use the directory (XHP123) as a code to select which lint messages to allow, then run all the directories as one test case. Then you can add a new rule test by just adding a directory instead of also having to add a class for it, and don't need to do the "guess the rule from the class name" stuff.

You may not be able to map rules cleanly to codes, but you could just run all the rules, then throw away the messages with a different code (instead of running only relevant rules).

Alternatively, rule stuff could be in some standard rules.json in the test directories as a sort of middle ground (more work than just directories but less work than creating classes).

I don't think any of these approaches are hugely better than the others, just proving that I'm reviewing. :)

That's vaguely nice but I'm going to hold off for now because longer term I'm not sure that IDs are the best fit for linter rules. That is, who really has any idea what XHP123 is?

This revision was automatically updated to reflect the committed changes.