Make interceptor resilient to file not found warnings #846
Conversation
|
No because the test doesn't test an exception is thrown, it tests that the
interceptor is still enabled after an exception occurs
…--
Nicolas Séverin
Senior Software Developer
World First, UK
MSc in Computer Sciences and Networks | 2014
Grenoble-INP ESISAR, France
MBA | 2015
IAE de Grenoble, France
Tel : +44 (0) 7756 734336
Mail : niiko3oo@gmail.com
Le dim. 24 nov. 2019 à 12:21, Gert de Pagter <notifications@github.com> a
écrit :
***@***.**** commented on this pull request.
------------------------------
In tests/phpunit/StreamWrapper/IncludeInterceptorTest.php
<#846 (comment)>:
> @@ -223,4 +224,20 @@ public function test_passthrough_dir_methods_pass(): void
IncludeInterceptor::disable();
}
+
+ public function test_it_re_enables_interceptor_after_file_not_found(): void
+ {
+ IncludeInterceptor::intercept(self::$files[1], self::$files[2]);
+ IncludeInterceptor::enable();
+
+ try {
+ fopen('file-does-not-exist.txt', 'r');
+ } catch (Warning $e) {
Would $this->expectException(Warning::class) do the trick here as well?
(I think phpunit 8.4 introduced a expectWarning, but i'm not sure what
phpunit version we run atm)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#846?email_source=notifications&email_token=ACY7HKKO7TC4RCGSEP4WCZLQVJUJRA5CNFSM4JQ6VLKKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCMYK4QI#pullrequestreview-321957441>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACY7HKNOXD47GU7OS4BQ4VLQVJUJRANCNFSM4JQ6VLKA>
.
|
| if (isset($this->context)) { | ||
| $this->fp = fopen($path, $mode, (bool) $options, $this->context); | ||
| } else { | ||
| $this->fp = fopen($path, $mode, (bool) $options); |
There was a problem hiding this comment.
Not that it is in any way the point of this PR, but why can't this if/else branch be condensed down to just the following?
$this->fp = fopen($path, $mode, (bool) $options, $this->context ?? null);There was a problem hiding this comment.
it could even be fopen($path, $mode, (bool) $options, $this->context) all the time, as $this->context defaults to null
There was a problem hiding this comment.
@BackEndTea I hope that's fine by you, I applied this suggestion throughout the file as it makes the code a lot clearer to understand.
|
It seems passing in null explicitly, instead of passing in nothing has a different result. Could you revert that change? |
|
Done |
|
Test failures are due to the interceptor not being 'cleaned up'. That means that after every test we would also need to do a I would also be okay with having this in the |
|
Okay i've done that, interestingly it wasn't failing locally for me. I've noticed one of the tests using |
|
i'd need to do a rebase to do that, there's quite a lot of commits in my branch which aren't in 0.14 as it was based on master. done i've also address the |
|
Thank you @nico-incubiq for this contribution! |
|
Released: 0.14.3 |
* Make interceptor resilient to file not found warnings * Don't add a property just to test something * Also prevent directory includes from breaking the interceptor * Small code improvements suggested on PR * Protect all functions which disable the interceptor against errors * Undo passing null instead of not passing anything * Disable interceptor after each test * Fix Code Style * Skip test not passing on Windows * Use requires to skip test on Windows
This PR:
Fixes #836
Warnings are converted to Errors while running tests under PHPUnit. But due to the fact the interceptor wrapper disables itself before calling fopen, and re-enables itself just after, if an error is thrown in between, the interceptor is never re-enabled.
This is actually the issue we experienced with @Bilge. Because we use Symfony, and one of the cache components attempts to read files in
./var/cache/test/poolsto determine hits and misses as part of the Kernel initialisation. Which means that very early on the interceptor gets disabled, and then mutations are never really applied to the code, so all mutants appear to be escaping.--I've only added the try/finally in the
stream_openfunction because I didn't have any example for the other functions, but tell me if you'd rather do the same for all functions on principle.--I have added a try/finally to all functions disabling and re-enabling the interceptor around a system call, and added specific tests for all of them.