Add phpstan/phpstan-phpunit as a dev dependency

Me Like!!! :)

Adding a new dependency to core will require a dependency evaluation eventually and, if approved, adding to this page https://www.drupal.org/about/core/policies/core-dependency-policies/depe...

All stuff for later though.

Dependency evaluation:

Maintainership: Primary author is Ondřej Mirtes, also the author of PHPStan. The author is responsive to issues and pull requests.
Security policy: None stated - this is a development tool so it is unlikely any security issue would affect us. We could open an issue in their queue to check, although not sure it is worth it?
Release cycle: Not as regular or prolific as PHPStan itself, but still several releases this year - more is not expected given that it is a much smaller target that PHPStan itself, and PHPUnit is fairly stable so it does not need to chase a moving target.
Other dependencies: None (except PHPStan, obviously)

As a release manager I have no issue with adding this dependency; it is a development-only code quality tool and could easily be removed again if any issues arise (e.g. if it became abandoned or for some reason not forward compatible with a new version of PHP or PHPUnit).

We already know PHPStan improves code quality and so improving the quality of our tests in an automated manner will only help further.

+1 to opening a followup to explore whether we should only mock methods that exist on interfaces.

I would mark this RTBC except that version 1.3.2 of phpstan-phpunit was released today, so NW to bump that.

Ah, did not see that; in that case I don't see why this can't be committed now and we can bump both dependencies together in a future issue.

phpstan/phpstan-unit 1.3.2 requires phpstan/phpstan ^1.9.3, so we need to wait for a bump of the current dev dependencies first.

I looked at this in #3327018: Update PHPStan to 1.9.3 and make daily "updated deps" QA jobs pass

Whoops, unwanted status change

PHPStan 1.9.3 made it in as well, so we can upgrade to the latest version.

Needs a manual rebase :/

Agreed, and in fact, already on it.

Great cleanup. I guess we don't actually do anything with our @covers annotations!

Is @covers on an interface really correct? We are only actually testing a concrete implementation?

Also why is (for example) this change required, when the class has @coversDefaultClass set to that class already?

-   * @covers: ::displayAllowedTags
+   * @covers \Drupal\Core\Field\FieldFilteredMarkup::displayAllowedTags

Also: this patch is now quite large; should we consider ignoring or baselining the common errors and solving them separately? e.g assertSame(NULL) could have its own issue.

Is @covers on an interface really correct? We are only actually testing a concrete implementation?

I hate it when the right answer means more work ;)
Tried to replace all (obvious) interfaces.

And that's all I can do for now until somebody decides if and how to chop this up into pieces.

I agree with @mondrake: Yes, this MR is big, all changes are backed up by PHPStan(-PHPUnit), so we can "verify" the changes.

Also, if we split, we basically give everybody more work with several issues where we have to c/p from this issue and creating suppressions just to remove them, when we have the "solution" already.

Also why is (for example) this change required, when the class has @coversDefaultClass set to that class already?

-   * @covers: ::displayAllowedTags
+   * @covers \Drupal\Core\Field\FieldFilteredMarkup::displayAllowedTags

My bad, missed the colon after @covers and assumed a full-class-name fix was needed for the ::displayAllowedTags-bit, like most of the other issues with @covers,

Fixed now, also for other occurrences.

If at all necessary, a test-only patch with the composer json+lock changes only could help showing what’s being fixed.

Back to NR for #30.

Segway:

The test-only patch made me wonder: What (if any) is the rule-of-thumb for adding a patch-level semver number in a dependency?
For example: All symfony/* stop at minor level, some other dependencies include patch-level versions, and phpspec/prophecy-phpunit only has a major version.

Nitpick in
core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php

* @covers \Drupal\ckeditor5\HTMLRestrictions::allowsNothing::allowsNothing

Double allowsNothing. Which is a bit strange this is not flagged by phpstan-phpunit.

Reviewed the whole MR this time, a bunch more @coversDefaultClass comments and some other minor nitpicks.

Added dependency evaluation to the IS.

Otherwise this all looks good to me so marking RTBC for another core committer opinion - +1 from me because this will only improve our tests in the future.

Regarding:

None stated - this is a development tool so it is unlikely any security issue would affect us.

That is not necessarily true. We had to issue a security advisory for RCE in PHPUnit in SA-CORE-2017-001. This is why we now have an explicit warning about PHPUnit's full disclosure policy on the core dependency info page. The purpose of these dependency evaluations is partly to supply content for that page, not just to determine whether the dependency can be added or not.

Opened https://github.com/phpstan/phpstan-phpunit/issues/161 to confirm the security policy.

Are there separate rules for this that we could enable one at a time? While I was skimming the MR, I had a lot of questions about the changes to @covers especially that would be best reviewed in their own issue. The stuff with missing parent::setUp(), parent::tearDown(), assertCount(), etc. OTOH is self-explanatory.

I see that @longwave previously suggested splitting this up and others pushed back. However, the problem with the current approach is that we're mixing adding the dependency with simple, obvious 1:1 replacements and the @covers changes that essentially require running the tool locally and/or reading the entire test or method to understand what's correct -- and therefore blocking adding the dependency on all that. This is covered in the Context, contextual scope, and reviewability section of the issue scope policy.

The average human brain can't effectively catch defects with this many different changes, and there is research that supports this. That's why we have the issue scope policy. It's not enough that the tool tells us to change it. The tool cannot tell us whether test coverage that we thought was there is actually missing, for example, meaning additional followups for additional test coverage might be needed, and that's one of the outcomes that there might be from the @covers errors.

So far three of three release managers agree and independently came to the conclusion that this issue needs to be split up (@quietone had also intended to suggest it).

When we added phpcs support to core, we did not run a gigantic cleanup patch to fix every single error it detected. Instead, we first made the choice to add the dependency and did that by itself, then opted into CS rules one at a time. Fixing errors correctly required context, and it was too many changes to review at once.

When we added CSpell, we did not fix every spelling error in core at once. Instead, we generated a dictionary of the words that were flagged as misspelled, and have been gradually reducing its size.

When we added PHPStan, we similarly did not fix everything it detected in core at once, but instead generated a large baseline, which we continue to fix gradually.

This issue should take the same approach. Understanding the changes to the @covers annotation requires context that is not available from scanning the large MR. It requires reading the code of each changed test.

If this MR simply added the dependency and added the errors to the baseline with a followup meta, it probably would have been committed weeks ago.

Let's split the MR up to add the dependency plus the necessary baseline, and then we can make incremental improvements, and discuss the @covers problem in its own, dedicated policy issue.

Discussed this with @mondrake in Slack, our plan is to:

- Use #3335406: DrupalStandardsListenerTrait is broken to fix DrupalStandardsListenerTrait and all the @covers errors it detects once fixed.
- Then return to this issue and try to get phpstan/phpstan-phpunit added as a dev dependency.
- Since we then have coverage of @covers (Baduum tish, I'm here all week, try the veal...) through phpstan/phpstan-phpunit we can then deprecate/remove DrupalStandardsListenerTrait in (Yet Another) issue to be more ready for PHPUnit 10 that drops the usage of listeners.

So, for now, postponing this issue on #3335406: DrupalStandardsListenerTrait is broken.

The blocker is in.

rebase and upgraded to 1.3.11

Rebasing to be able to retest and get rid of the (alarmingly increased) random JS test failures.

Right....

Random JS test failures galore!

Not wasting any more test-cycles on that, marking RTBC.

Note: I _did_ work on this issue, but only on the resolving of the new PHPStan errors, which are now out of scope and out of the new MR.
Therefore I think I'm OK to do so. There will be plenty of eyes on this from Core Committers and Framework managers anyway.

Committed 947fd48 and pushed to 10.1.x. Thanks!

@mondrake one follow-up to clean-up baseline should be enough?

Opened #3351055: [META] Fix PHPStan baseline suppressions introduced with phpstan/phpstan-phpunit to get rid of the new suppressions in the PHPStan baseline.

INSTA-EDIT: Cross-post with @andypost. Hope the new issue works for you?

I created a CR for this https://www.drupal.org/node/3351386