test: AutoReview - unify data provider returns #7544
Conversation
keradus
changed the title
There was a problem hiding this comment.
I am wondering if it wouldn't be possible to introduce a fixed contract for fixers' test classes (like define provideFixCases, provideFix80Cases etc) with proper return type and only override the body (actual cases) in tests 🤔. That would be better than defining the same/similar phpDoc hundred of times.
Anyway, I did not do a full review, just wanted to give some feedback.
tests/AutoReview/ProjectCodeTest.php
Outdated
| /** | ||
| * @dataProvider provideTestClassCases | ||
| */ | ||
| public function testDataPorvidersDeclaredReturnType(string $testClassName): void |
There was a problem hiding this comment.
| public function testDataPorvidersDeclaredReturnType(string $testClassName): void | |
| public function testDataProvidersDeclaredReturnType(string $testClassName): void |
Anyway, why do we need this test? Isn't PHPStan checking this?
There was a problem hiding this comment.
looking on amount of changes, PHPStan is not checking it.
There was a problem hiding this comment.
Mental shortcut - PHPStan requires more precise iterable type and IMHO it could be sufficient. For new providers we rather define good array shapes, all currently ignored missing types also could be added properly. If I understand correctly you just wanted to ensure really, really precise array shape?
There was a problem hiding this comment.
yep, I wanted to avoid array<int> when we have only one int, so array{int}.
some side wins on the go also, like return array vs list vs iterable unification.
now (With this PR), for data providers, we either have no phpdoc (and simply rely on : iterable), or we have nice iterable of tuples
| @@ -452,6 +452,56 @@ public function testExpectedInputOrder(string $testClassName): void | |||
| } | |||
There was a problem hiding this comment.
I am wondering if it wouldn't be possible to introduce a fixed contract for fixers' test classes
Overall, sounds valid.
I think the pre-step for it is unification of prototypes (eg in this PR we see config, expected, input -> expected, input, config (yet not claiming this PR unifies everything) and test method names (especially around @requires PHP xyz)
And if for some cases we could not reuse contract, the custom-type could help
https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/pull/7544/files#diff-a4ce6ffd8cb697aecebba7af3c784ed29078f645544c069ec7fecbe9417e2a04R20
inspired by #7542