Lazy assert types gathering in TypeInferenceTestCase

[herndlm]

Just an idea I had in mind I want to propose. This is more about the concept and not the actual implementation. I adapted NodeScopeResolverTest to make use of it, looks like there would be 14 more.

This PR adds new methods to TypeInferenceTestCase that make it possible to write tests where the assert type gathering is not happening in the dataprovider, but in the actual test method. This is done by returning callbacks in the dataprovider.

This has various upsides

• tests start immediately, feel faster and likely fail faster. previously it took a while until e.g. NodeScopeResolver printed output
• code coverage can be collected because the code parts are not executed in the dataprovider anymore CC @staabm @szepeviktor
• the phpunit configuration here prints risky test files that don't assert anything (there are 2 in the NodeScopeResolver right now) which makes the tests safer
• using $file in the yield makes PHPUnit print the test case file instead of the test case number, e.g.

There were 2 riskys:

1) PHPStan\Analyser\NodeScopeResolverTest::testFileAssertsCallbacks with data set "/Users/herndlm/Development/source/git-forks/phpstan-src/tests/PHPStan/Analyser/data/DateTimeModifyReturnTypes.php" (Closure Object (...))
This test did not perform any assertions

/Users/herndlm/Development/source/git-forks/phpstan-src/tests/PHPStan/Analyser/NodeScopeResolverTest.php:1136

2) PHPStan\Analyser\NodeScopeResolverTest::testFileAssertsCallbacks with data set "/Users/herndlm/Development/source/git-forks/phpstan-src/tests/PHPStan/Analyser/data/bug-7031.php" (Closure Object (...))
This test did not perform any assertions

I don't like that these are new methods and users have to manually migrate. Not sure if this can be made "smoother". I also don't like their names. If this idea is accepted, we could also come up with completely different names and deprecate the old methods even at some point maybe.

What do people think about this?

[ondrejmirtes]

I think something like this has been proposed before but the main blocker for me is that only the first failure from a file is reported, not all of them. That's why it's currently done with generators like that.

[herndlm]

oh, you're right, yes. I also noticed that when fixing the test files where assertType was not working. that is kind of a deal-breaker I guess hmm :/
I'll think about this, maybe I can simply adapt the way things are currently asserted. Basically a new assertFileAsserts() variant within assertFileAssertsCallbacks().

[herndlm]

I re-implemented the assertion part, in the simplest way I could currently imagine, to only fail once per file basically => output all errors in a file at once. I like the output even more than before tbh, because it's more compact, e.g.:

There were 2 failures:

1) PHPStan\Analyser\NodeScopeResolverTest::testAssertions with data set "/home/martin/PhpstormProjects/phpstan-src/tests/PHPStan/Analyser/data/implode.php" (Closure Object (...))
Expected type 'wrong', got type '12345' in /home/martin/PhpstormProjects/phpstan-src/tests/PHPStan/Analyser/data/implode.php on line 15.
Expected type 'wrong', got type '12345' in /home/martin/PhpstormProjects/phpstan-src/tests/PHPStan/Analyser/data/implode.php on line 16.
Failed asserting that an array is empty.

/home/martin/PhpstormProjects/phpstan-src/src/Testing/TypeInferenceTestCase.php:108
/home/martin/PhpstormProjects/phpstan-src/tests/PHPStan/Analyser/NodeScopeResolverTest.php:1138

2) PHPStan\Analyser\NodeScopeResolverTest::testAssertions with data set "/home/martin/PhpstormProjects/phpstan-src/tests/PHPStan/Analyser/data/bug-1945.php" (Closure Object (...))
Expected Maybe, actual certainty of variable $foo is Yes in /Users/herndlm/Development/source/git-forks/phpstan-src/tests/PHPStan/Analyser/data/bug-1945.php on line 23.
Expected No, actual certainty of variable $foo is Yes in /Users/herndlm/Development/source/git-forks/phpstan-src/tests/PHPStan/Analyser/data/bug-1945.php on line 47.
Failed asserting that an array is empty.

/home/martin/PhpstormProjects/phpstan-src/src/Testing/TypeInferenceTestCase.php:108
/home/martin/PhpstormProjects/phpstan-src/tests/PHPStan/Analyser/NodeScopeResolverTest.php:1138

it could be made even more compact, e.g. the file info is redundant now because one assertion failure is for one file and the file is already used as dataprovider array key.

differences

• now multiple failures per file are only counted as one failure, which is kind of what we want though I think, at least we get it now without any weird dataprovider behaviour.
• the "Failed asserting that an array is empty." message is a maybe a bit weird? With a custom constraint we could get rid of that message easily I think.
• time shown in the phpunit summary is now correct, this was way too short before

[staabm]

I am wondering whether we can adjust this PR in a way that already exisiting TypeInferenceTestCase subclasses immediately benefit from the base-class changes without modifications..?

(Do we need this new mechanism in parallel to the pre-exisiting one?)

[herndlm]

didn't figure that out yet, I think it's not possible, but maybe I'm missing some magic to make it possible :) the problems are different return and argument types for the 2 methods

[staabm]

wondering still about the same thing.

couldn't you rename the gatherAssertTypes method which existed before this PR to e.g. gatherAssertTypesFromFile and implement the PRs gatherAssertions as gatherAssertTypes (same for assertFileAsserts) and then we can take this changes as a drop-in replacement without needs for change in NodeScopeResolver test..
maybe I am missing something obvious?

[staabm]

I just realized that the thing which is not compatibel is https://github.com/phpstan/phpstan-src/pull/1991/files#diff-1e9b47d3e7051caa11f9d76ff7bd6ff2afb430014dd93dad916cbb8c2148f545R1139

[phpstan-bot]

This pull request has been marked as ready for review.

[herndlm]

should I fix conflicts here again or do something else? Otherwise I would just close it for now I guess, no worries