Unit test classes should be declared as either final or abstract

[andrewnicols]

Fixes #102

The rationale for this change is based on the principle of least astonishment.

• If I modify a unit test, I expect it to only impact the thing that I expect it to test. If I add a new test, it should only test the thing I expect it to test.
• If someone is extending my unit test, then that is no longer the case, and I may be testing things which I did not intend, or over-testing the same thing.

To solve this the idea is:

• If I intend a test to be consumed, then I should make it abstract and I should extend it myself for my specific use-case.
• If I do not intend a test to be extended, then it should be declared final

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (da7aa56) 96.57% compared to head (b8f9eee) 96.73%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #103      +/-   ##
============================================
+ Coverage     96.57%   96.73%   +0.16%     
- Complexity      557      586      +29     
============================================
  Files            24       26       +2     
  Lines          1547     1625      +78     
============================================
+ Hits           1494     1572      +78     
  Misses           53       53              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[stronk7]

Apart from the detail in the review, surely the major "problem" of this nice Sniff is to determine how do we apply it (only for X.Y and up, warning and error in 1-year, only for non-plugins, ...).

As far as it's not part of the coding-style neither enforced by PHPUnit incoming versions... we need to be careful about it (back to the discussion happening @ #92).

Ciao :-)

[andrewnicols]

Okay,

I've:

• split this into two sniffs:
  • one for tests which makes them final
  • one for testcases which makes them abstract
• applied it to 4.4 and up only

I think we need to add this to the epic for PHPUnit 10 as a part of that puzzle and add it to our coding style. This isn't something radical.

[stronk7]

For fixtures...

moodle/Tests/Sniffs/PHPUnit/fixtures

or

moodle/Tests/fixtures/PHPUnit

I've been doing the later lately and it seems that you've been doing the former. Of course, there are still a lot of fixtures to move so, at some time, we should decide which of the 2 fixtures dirs to use and go for it everywhere (for testing own Sniffs, other standards sniffs are another story).

[stronk7]

The final question in the review here is... do we want this to start warning now, with 4.4dev? Or want it for the next one?

[andrewnicols]

For fixtures...

moodle/Tests/Sniffs/PHPUnit/fixtures

or

moodle/Tests/fixtures/PHPUnit

I've been doing the later lately and it seems that you've been doing the former. Of course, there are still a lot of fixtures to move so, at some time, we should decide which of the 2 fixtures dirs to use and go for it everywhere (for testing own Sniffs, other standards sniffs are another story).

I've been doing the former because it keeps the fixtures closer to the thing under test, and makes them easier to find, but I'm happy to move them. Let me know your preference.

[stronk7]

I've been doing the former because it keeps the fixtures closer to the thing under test, and makes them easier to find, but I'm happy to move them. Let me know your preference.

Yeah, I think I agree with you, let's keep the fixtures closer to the the Test using them, specially now that we are creating subdirs for the tests.

Not in a hurry to move all current ones that have followed other paths, but let's go with your alternative for new ones.

[stronk7]

So, just to be 100% sure, as commented above:

• The final question in the review here is... do we want this to start warning now, with 4.4dev? Or want it for the next one?
• It will apply to all code checked (core and plugins).

I'm happy with that (4.4dev), will generate some noise for plugins, but it's a good thing, I think. And prepares us for the future, together with #106 and other details that have arrived recently (static providers, ....).

So, with your 👍 , agreement!

[stronk7]

Sold!