[PhpUnitBridge] support ClockMock and DnsMock with PHPUnit 10+
#58467
Conversation
xabbuh
commented
Oct 5, 2024
| Q | A |
|---|---|
| Branch? | 7.2 |
| Bug fix? | no |
| New feature? | yes |
| Deprecations? | no |
| Issues | Fix #49069 |
| License | MIT |
803602a to
16b6f37
Compare
There was a problem hiding this comment.
This misses the equivalent of the $mockedNamespaces constructor argument of the SymfonyTestsListener.
This constructor argument is important because the "hack" used to mock clock and dns functions requires being applied before the first use of those functions in a given namespace (because PHP caches the fact that a function should fallback to the global namespace). So registering the mock only when starting that test can be too late if some other (previous) tests execute code in the same namespace that use those functions.
And the SymfonyTestsListenerTrait also registers mocks early when starting the testsuite rather than the test, for the same reason.
src/Symfony/Bridge/PhpUnit/Extension/RegisterClockMockSubscriber.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bridge/PhpUnit/Extension/UnregisterClockMockSubscriber.php
Outdated
Show resolved
Hide resolved
If I didn't miss anything PHPUnit doesn't allow to pass array parameters to an extension which makes this harder to implement. One possible way to implement this feature that I see is supporting JSON encoded environment variables. What do you think? |
ClockMock and DnsMock with PHPUnit 10+
|
Maybe we can use a comma-separated list of namespaces for the values of parameters (simpler to write than using JSON inside an XML node) |
3dd81e9 to
08b7ecc
Compare
|
I have updated the PR to register the namespaced functions as soon as possible, merged the two extensions and made it possible to register comma-separated, custom namespaces when registering the extension |
|
|
||
| foreach ($test->metadata() as $metadata) { | ||
| if ($metadata instanceof Group && 'time-sensitive' === $metadata->groupName()) { | ||
| ClockMock::withClockMock(true); |
There was a problem hiding this comment.
SymfonyTestsListenerTrait also registers the mock on starting the test in addition to enabling it. Is is safe to drop that part ?
There was a problem hiding this comment.
this already happens in the RegisterClockMockSubscriber which is run on the test suite level, doing it here as well would just redo what already happened before
|
I just re-discovered that the phpunit-bridge also has a CoverageListener (separate from the SymfonyTestsListener). Should we port it to a separate CoverageExtension ? and is it even possible to port it ? |
|
Looking at the code I guess that it should be possible to port it. Do we have an example test on which we could try such a migrated extension? |
|
Ah I see we have tests for it. I will see if I can find some time to work on it. Will send a different PR then. |
|
|
||
| class SymfonyExtension implements Extension | ||
| { | ||
| public function bootstrap(Configuration $configuration, Facade $facade, ParameterCollection $parameters): void |
There was a problem hiding this comment.
this also needs to enable the DebugClassLoader if available, as done in SymfonyTestsListenerTrait.
There was a problem hiding this comment.
I did not enable the DebugClassLoader here on purpose. PHPUnit defers the execution of bootstrapping extensions until the test suite to run was actually loaded. Thus, enabling the class loader here is too late to trigger deprecations for classes that have been loaded before. Right now, the only place to enable it early enough is a custom bootstrap file (like done in #58370).
There was a problem hiding this comment.
But for projects that don't use a custom bootstrap file, I don't see which custom code can run before the loading of the testsuite. So the autoloading of custom classes will still probably happen after that point.
So the bootstrapping of extensions looks like a good place to register it for a better DX for the simple case (DebugClassLoader::enable is already handling the case where autoloaders have already been wrapped by a DebugClassLoader, so it is fine if both the bootstrap file and the extension bootstrapper call it)
There was a problem hiding this comment.
fair enough, this would still fail if a class (e.g. a fixture) is defined in the same file as a test, but it will probably still cover 99% of the other use cases
90ff906 to
c9d3d5e
Compare
|
@stof I added tests to ensure that the mocked functions from both the |
e217507 to
f750beb
Compare
ee2e5aa to
869c6d4
Compare