Feature optimize node scope resolver test #3440
Conversation
|
Interesting. Reminds me a bit of what I tried to do in #1991 once but unfortunately never finished |
|
@schlndh Can you please explain what this PR changes and why is it faster?
Why does it run twice? |
|
@ondrejmirtes The important change is moving The reason it helps parastan is as follows (I'm not familiar with internals of either phpunit or parastan. I'm basing this on a mixture of debugging, reading the source code and guessing):
EDIT: Based on |
|
Alright, that makes sense. And for the 2nd part - how are you achieving that all failures from a single file are reported at once? |
|
@ondrejmirtes I copied the |
|
On my computer The output now looks like this: Previously it looked like this: Could we somehow get closer to the original output that looks like a diff? Maybe even somehow reuse PHPUnit utilities for diffing values. Thanks! |
|
btw: I find it interessting that running |
|
@staabm That's expected and described in this PR. Only Paratest ran it twice. |
@ondrejmirtes let me phrase it differently: maybe it would make sense to run I have no idea this would be possible though. |
|
We won't need to worry about that when this PR gets merged. |
|
@ondrejmirtes Thanks for your feedback. I can adjust the error message. But IMO there is not much of a reason to try to preserve the diff output format. The types are always single-line strings, so the diff is just putting the two types below each other, instead of on one line. So continuing with your example, I could do something like this: IMO this is better than my current version, because the types are below each other. And it's also better than the 1.12.x version, because there's less noise from phpunit. If you still want a diff. Then I suppose I could generate an "expected string" and "actual string" and then compare them via Let me know what you think. Or if you want to avoid the back-and-forth (I should be able to get back to this on Thursday/Friday), you can merge it as is and adjust the output to your liking. |
I'd say that's expected. Paratest does extra stuff compared to phpunit. If you want to run a single test, just run it with phpunit. If you run many tests across multiple cores, than paratest's overhead should be worth it due to the speed up from running the tests in parallel. Example: |
|
Yeah, this would be sufficient, thank you! |
|
Done. I changed |
|
Love it, thank you! BTW if you wanted to do some more tests-related improvements, I have ideas how to modernize RuleTestCase which badly needs it. 2nd arguments to [
[
'Missing parameter $bar (string) in call to method Bug4800\HelloWorld2::a().',
36,
],
],But it could look like this instead: $this->analyse([__DIR__ . '/data/bug-4800.php'], [
RuleErrorBuilder::message('Missing parameter $bar (string) in call to method Bug4800\HelloWorld2::a().')
->line(36)
->identifier('argument.missing'),
]);Basically the See RuleErrorTransformer what all |
This is a different take on #3163. I fixed the main issue you mentioned: You can see all the issues from one file at once (instead of just the first one).
My motivation for this change is to speed up
make tests. On my laptop with 8C/16T the results oftime make testsare as follows:1.12.x:
My branch:
I measured the
NodeScopeResolverTest.phpseparately and it takes roughly 25s for me (almost all of that is spent ingatherAssertTypes). You can see that the different in the user time is also roughly 25s. I guess that's because in 1.12.x the test effectively runs twice. All the work is done in the data provider, which first has to run in paratest's main process, before being run again in a worker process.Sadly, the individual test cases are still not run in parallel.