Changing this file is also not triggering the test itself, which is a bug, but I'm just going to ignore that for the moment.

This should maybe be a new method (such as PhabricatorInfrastructureTestCase::testMethodsDoNotChangeVisibility).

Also, this test won't be run in rARC or rPHU... Maybe there should be a base PhutilLibraryTestCase class to avoid copy-paste.

Seems worth writing to work for rARC and rPHU... otherwise LGTM...!

Haha, no problem. This looks good once we sort out T6822.

Interesting... one issue that I noticed with this is that it fails if any of the loaded libraries violate the method visibility rule. For example, I just hit this in rP:

> arc unit -- src/
   FAIL  PhabricatorLibraryTestCase::testMethodVisibility
Assertion failed, expected values to be equal (at PhutilLibraryTestCase.php:88): Class "ArcanistPhpcsLinter" implements method "getMandatoryFlags" with the wrong visibility. The method has visibility "public", but it is defined in parent "ArcanistExternalLinter" with visibility "protected". In Phabricator, a method which overrides another must always have the same visibility.
Expected: 'protected'
  Actual: 'public'

I think testing the depends-on libraries implicitly is fine, the testEverythingImplemented() test effectively does this too. Getting multiple test failures from one root cause isn't great in either case, but we could have the subclasses coordinate if we ever care to fix that. Doesn't seem like a big deal.

In D11052#107219, @epriestley wrote:

I think testing the depends-on libraries implicitly is fine, the testEverythingImplemented() test effectively does this too. Getting multiple test failures from one root cause isn't great in either case, but we could have the subclasses coordinate if we ever care to fix that. Doesn't seem like a big deal.

Oh, it's easy to fix... should I bother?

The immediate fixes I can come up (some static "this has run" flag) with would incorrectly break if something tried to run the test twice, which seems sort-of worse than just reporting the failure twice.

I guess you could clear the flag in willRunTests() and then set it in the first test to run? If that's not too much of a mess, maybe worthwhile, but I haven't looked at that code too recently. Either way seems fine to me.