Only mock methods defined in interfaces

@mondrake Hope you're not too mad that, even if this issue is assigned to you, I got curious and tried to fix a few.

Not at all :)

... and in fact it's not assigned to me ;)

I think first of all we need to discuss whether we want to do that. It's a bit limiting PHPUnit in fact, and I am not sure our codebase is really up to it yet. This will probably have to become a meta at later stage if we decide for a yes.

I think first of all we need to discuss whether we want to do that. It's a bit limiting PHPUnit in fact, and I am not sure our codebase is really up to it yet. This will probably have to become a meta at later stage if we decide for a yes.

There are some actual problems in there, but looking at the "simple" solutions for the ones I got passing, basically mocking the Interface instead of the implementing class, and one (IMHO) actual wrong mock, we might consider not suppressing this error.

But as you said: Discussion! :)

Ok, I got rid of the "easy" ones.

Personally I think we should not suppress this one, since it caught a few incorrect mocked classes and "usually" just mocking the interface instead of the implementing classes enough to get it passed.

Downside is that there will be occasions that we need to add to the PHPStan baseline, which might be hard for people to understand/do.

That's my 0.02 EUR, I'll leave it to @mondrake what status this issue needs right now.

Adding my own 0.01 cents by changing to NR. Let’s see if @longwave comments, or else we may ping committers.

As it helps us fix type errors I am generally +1 for this, unless it is going make life super difficult in some way later on. How difficult is it to fix the other baseline errors? As an example YamlTest looks like it should be mocking SerializationInterface instead?

I also think we should just refactor HelpTopicTwigLoaderTest::getHandlerMock() away, as it's only called once with each argument.

Ok, this is s fr as I can take this issue.

There's only two suppressions left, both in tests/Drupal/Tests/Core/Entity/EntityUrlTest.php.
There's too much funkiness going on in there for me to grok the code, let alone alter it.
We need people with actual brains to tackle those, or just leave them in the baseline.

All the other ones, I handled in my patented "Throw it on the wall and see if it sticks"-method.
Which lead to:

- Replace mocking with prophecy. Looks like phpstan/phpstan-phpunit isn't checking that approach. Unsurprising, since AFAIK prophecy is dropped from being standard on board in PHPUnit 10.
- Since prophecy doesn't do non-existing methods I added a few mock-classes with empty methods, just to make them usable with mockery.

I _think_ we're not loosing test-coverage, but I'll let reviewers decide that.

A few nits on the MR.

I think this is a sensible improvement in terms of tests readibility, so this is really worth doing IMHO.

Re #9

unless it is going make life super difficult in some way later on

my pragmatic approach would be that unless we try with future tests, we cannot know. So IMHO better have this in and improve what we can now, and then if it proves to be too strict a rule, we can always reintroduce the ignore.

Thanks @mondrake. Fixed yer nits.

I reviewed some of this then realised maybe we are uncovering bugs?

For example in SqlContentEntityStorageSchemaTest the reason we cannot mock EntityTypeManagerInterface is that the constructor calls:

    $this->entityType = $entity_type_manager->getActiveDefinition($entity_type->id());

But this method doesn't exist on the interface, so if something swaps in a different EntityTypeManagerInterface there is no guarantee that this method even exists.

I reviewed some of this then realised maybe we are uncovering bugs?

Somehow these PHPStan should-be-quick-n-sweet issues always dig up more.

Anyway, let's fix yer nits on the MR first.

Right, threads addressed.

Unsure what to do with #14 📌 Only mock methods defined in interfaces Fixed .

- Are changes needed?
- If so, in here or in follow-up(s)?
- Something completely different?

For #14, I would suggest to defer to a specific follow-up. Unfortunately with our BC policy changing interfaces is very difficult.

Thanks @mondrake, opened 📌 Rework SqlContentEntityStorageSchemaTest Active as a follow-up.

Merge blocked: fast-forward merge is not possible. To merge this request, first rebase locally.

Committed 19d08d3 and pushed to 10.1.x. Thanks!

mondrake → closed merge request !3764

There are two hardcoded ones left in the baseline

ag "Trying to mock an undefined method"
core/phpstan-baseline.neon
2949:			message: "#^Trying to mock an undefined method getRevisionId\\(\\) on class Drupal\\\\Tests\\\\Core\\\\Entity\\\\UrlTestEntity\\.$#"
2954:			message: "#^Trying to mock an undefined method isDefaultRevision\\(\\) on class Drupal\\\\Tests\\\\Core\\\\Entity\\\\UrlTestEntity\\.$#"

Automatically closed - issue fixed for 2 weeks with no activity.