Added compatibility with PHPUnit 10 #3813
Conversation
|
This is a breaking change. Are we allowed to do these on 3.x? Do we have something like an UPGRADE.md that I can add this to? I did not modify Twig's own tests as we can do that when/if we upgrade to PHPUnit 10. However, this is needed so that Twig's users can upgrade themselves to PHPUnit 10, when they are using |
SiebelsTim
force-pushed
the
3810-compat-phpunit10
branch
2 times, most recently
from
18e8edd
to
2e99384
Compare
|
I've pushed a different concept. I dislike that it is quite complex. I'm open to suggestions, please tell me what you think. |
SiebelsTim
force-pushed
the
3810-compat-phpunit10
branch
from
2e99384
to
2c6c59e
Compare
|
I think I resolved most of the comments. Naming things is hard though. Furthermore, I noticed that there is also But even then, upgrading to it is much harder, as we use |
SiebelsTim
force-pushed
the
3810-compat-phpunit10
branch
from
2c6c59e
to
6fca676
Compare
The replace test case must be refactored in a way where the mock is created in the test, not in the data provider. |
SiebelsTim
force-pushed
the
3810-compat-phpunit10
branch
from
d6017c7
to
e9a7f34
Compare
src/Test/ASTNodeTestCase.php
Outdated
| static::assertNodeCompilation($source, $node, $environment, $isPattern); | ||
| } | ||
|
|
||
| public static function assertNodeCompilation($source, Node $node, Environment $environment = null, $isPattern = false) |
There was a problem hiding this comment.
Could you check why this method needs to be static, same for getCompiler and getEnvironment?
There was a problem hiding this comment.
keeping getEnvironment non-static would be more flexible (test cases could then use mocks in it if they need it). so I would keep all these non-static
There was a problem hiding this comment.
When doing so, you cannot use them in data providers anymore. I thought it would be more valueable to allow them to be used in data providers.
We use getCompiler in one test internally.
I don't have a strong opinion on this though.
I've pushed a commit turning this non-static again.
There was a problem hiding this comment.
Can you explain de bit more how you use getCompiler in the dataprovider ?
There was a problem hiding this comment.
\Twig\Tests\Node\DeprecatedTest uses Compiler::getVarName() to build the expected php source code that is compiled from a Node.
I suppose we should rather hardcode __internal_compile_0 into the expected string. What do you think?
I pushed a commit doing that on top of this branch in case you agree.
SiebelsTim
requested review from
GromNaN and
stof
and removed request for
stof and
GromNaN
fixes #3810