Refactor the data provider for NodeScopeResolverTest

[thg2k]

Refactor the data provider for NodeScopeResolverTest

Instead of yielding resolved analysis for each data file, and then have the
actual test only assert the given result, provide only the paths of the data
files and have the actual analysis and assertion inside the test case.

This brings the following benefits:

1. Listing tests and filtering by one single data file is much faster (showcase 1).
2. Data files skipped due to php version requirement now show as skipped
   instead of silently disappearing (showcase 2).
3. Repeating a test for a single data file many times is much faster while
   debugging the actual source code (showcase 3).
4. If the analysis would fail, instead of busting the whole test suite due to
   an exception inside the data provider, it shows inside the test case (showcase 4).

I believe in particular the showcase 4 happened a lot of times and it's hard
to debug because phpunit does not properly show uncaught exceptions in the
data providers.

Showcase 1 - List available tests

Setup:
None.

Command:
time ./vendor/bin/phpunit --no-coverage tests/PHPStan/Analyser/NodeScopeResolverTest.php --list-tests

Before:

Available test(s):
(omitted many many lines, confusing output)

real    0m35.993s
user    0m33.520s
sys     0m0.357s

After:

Available test(s):
(omitted some lines, nicer output)

real    0m1.636s
user    0m1.488s
sys     0m0.094s

Showcase 2 - Skipped data files due to version requirement now show up as skipped, same time

Setup:
None.

Command:
time ./vendor/bin/phpunit --no-coverage tests/PHPStan/Analyser/NodeScopeResolverTest.php --list-tests

Before:

.............................................................   61 / 9003 (  0%)
.............................................................  122 / 9003 (  1%)
(omitted dots)
............................................................. 8906 / 9003 ( 98%)
............................................................. 8967 / 9003 ( 99%)
....................................                          9003 / 9003 (100%)

Time: 00:01.579, Memory: 372.00 MB

OK (9003 tests, 9003 assertions)

real    0m36.586s
user    0m34.383s
sys     0m0.438s

After:

.........S............................S........................  63 / 996 (  6%)
..............S.................S....................S......... 126 / 996 ( 12%)
......S.................S........................S............. 189 / 996 ( 18%)
...............S......S..S...S...........SS..S................. 252 / 996 ( 25%)
...S..S........S....S.......................................... 315 / 996 ( 31%)
.....................S.............S........................... 378 / 996 ( 37%)
.................................S............................. 441 / 996 ( 44%)
..............................S....S........S.............S.... 504 / 996 ( 50%)
...........................................S................... 567 / 996 ( 56%)
............................................................... 630 / 996 ( 63%)
.......................S....................................... 693 / 996 ( 69%)
S............................S................................. 756 / 996 ( 75%)
................................S................SS............ 819 / 996 ( 82%)
........S...................S..S............................... 882 / 996 ( 88%)
.......................................................S....... 945 / 996 ( 94%)
.S......S.........................S................             996 / 996 (100%)

Time: 00:35.047, Memory: 368.00 MB

OK, but incomplete, skipped, or risky tests!
Tests: 996, Assertions: 9003, Skipped: 40.

real    0m36.719s
user    0m35.242s
sys     0m0.407s

Showcase 3 - Running a single test data file with an assertion error, much faster

Setup:
Change int<min, 2> with int<min, 42> in integer-range-types.php

Command:
time ./vendor/bin/phpunit --no-coverage tests/PHPStan/Analyser/NodeScopeResolverTest.php --filter integer-range

Before:

............F..................................................  63 / 198 ( 31%)
............................................................... 126 / 198 ( 63%)
............................................................... 189 / 198 ( 95%)
.........                                                       198 / 198 (100%)

Time: 00:00.130, Memory: 368.00 MB

There was 1 failure:

1) PHPStan\Analyser\NodeScopeResolverTest::testFileAsserts with data set "/home/web/php/phpstan/phpstan-src/tests/PHPStan/Analyser/nsrt/integer-range-types.php:9" ('type', '/home/web/php/phpstan/phpstan...es.php', 'int<min, 42>', 'int<min, 2>', 9)
Expected type int<min, 42>, got type int<min, 2> in /home/web/php/phpstan/phpstan-src/tests/PHPStan/Analyser/nsrt/integer-range-types.php on line 9.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'int<min, 42>'
+'int<min, 2>'

/home/web/php/phpstan/phpstan-src/src/Testing/TypeInferenceTestCase.php:125
/home/web/php/phpstan/phpstan-src/tests/PHPStan/Analyser/NodeScopeResolverTest.php:194

FAILURES!
Tests: 198, Assertions: 198, Failures: 1.

real    0m35.080s
user    0m33.815s
sys     0m0.369s

After:

F                                                                   1 / 1 (100%)

Time: 00:00.406, Memory: 64.00 MB

There was 1 failure:

1) PHPStan\Analyser\NodeScopeResolverTest::testFileAsserts with data set "nsrt/integer-range-types.php" ('/home/web/php/phpstan/phpstan...es.php', null)
Expected type int<min, 42>, got type int<min, 2> in /home/web/php/phpstan/phpstan-src/tests/PHPStan/Analyser/nsrt/integer-range-types.php on line 9.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'int<min, 42>'
+'int<min, 2>'

/home/web/php/phpstan/phpstan-src/src/Testing/TypeInferenceTestCase.php:126
/home/web/php/phpstan/phpstan-src/tests/PHPStan/Analyser/NodeScopeResolverTest.php:201

FAILURES!
Tests: 1, Assertions: 1, Failures: 1.

real    0m2.068s
user    0m1.895s
sys     0m0.119s

Showcase 4 - Running a single test data file with a SYNTAX error, faster and better reporting

Setup:
Change , $i); with , $i; in integer-range-types.php

Command:
time ./vendor/bin/phpunit --no-coverage tests/PHPStan/Analyser/NodeScopeResolverTest.php --filter integer-range

Before:

No tests executed!

real    0m12.093s
user    0m11.542s
sys     0m0.240s

After:

E                                                                   1 / 1 (100%)

Time: 00:00.074, Memory: 46.00 MB

There was 1 error:

1) PHPStan\Analyser\NodeScopeResolverTest::testFileAsserts with data set "nsrt/integer-range-types.php" ('/home/web/php/phpstan/phpstan...es.php', null)
PHPStan\Parser\ParserErrorsException: Syntax error, unexpected ';', expecting ')'

/home/web/php/phpstan/phpstan-src/src/Parser/RichParser.php:63
/home/web/php/phpstan/phpstan-src/src/Parser/PathRoutingParser.php:44
/home/web/php/phpstan/phpstan-src/src/Parser/CachedParser.php:46
/home/web/php/phpstan/phpstan-src/src/Testing/TypeInferenceTestCase.php:94
/home/web/php/phpstan/phpstan-src/src/Testing/TypeInferenceTestCase.php:244
/home/web/php/phpstan/phpstan-src/tests/PHPStan/Analyser/NodeScopeResolverTest.php:198

ERRORS!
Tests: 1, Assertions: 0, Errors: 1.

real    0m1.676s
user    0m1.510s
sys     0m0.122s

[ondrejmirtes]

I haven't gone into detail with the PR but please tell me if I'm right to assume that with this approach, the test will only tell us about first failure in any given file, right?

Currently we're yielding all the asserts from a file to report errors on all of them at once.

[thg2k]

I haven't gone into detail with the PR but please tell me if I'm right to assume that with this approach, the test will only tell us about first failure in any given file, right?

That is correct. But in a classic phpunit test file this is also the case:

public function testSomething(): void
{
  $this->assertSame(1, $a);
  $this->assertSame(1, $b);
}

The test bails at the first failed assertion. So you can think of the data file as a series of assertions, indeed only the first one failing is shown.

[ondrejmirtes]

Yes, this would be a huge downgrade from the current version.

Your solution is faster by numbers "on paper" but in practice this would slow down our workflows too much. It's very valuable to see all errors at once.

[thg2k]

Yes, this would be a huge downgrade from the current version.

I suppose this can be done by refactoring further inside assertFileAsserts() to build an array expected-actual and then assert the full array instead of asserting one by one. If you are interested in the overall PR I can do that.

Your solution is faster by numbers "on paper" but in practice this would slow down our workflows too much. It's very valuable to see all errors at once.

Not really "on paper", when you are working on a particular data file don't you have to repeat it several times? Spending 1 second for each run instead of 30+ seems like a great benefit to me.

[staabm]

I agree that seeing all errors per file is a big plus to get an idea of the impact of a change as a whole.

[ondrejmirtes]

Thank you for your contribution. I appreciate the time you invested in preparing this pull request. However, I have decided not to merge it.