bypass-finals
bypass-finals copied to clipboard
Using Infection library fails because of another stream wrapper
The library Infection uses its own stream wrapper, see \Infection\StreamWrapper\IncludeInterceptor
.
Because of that, the mutants created by the library are not used and that makes the whole library unusable if the tests use bypass-finals
, because the stream wrapper is overriden.
I tried to guess how both stream wrappers could be used alongside each other but as I have no expertise at all in this topic it got hard very fast.
Do you think this could be possible? How could it be done? Can I help 🙂 ?
Maybe chaining like BypassFinals::enable($inner = IncludeInterceptor::class)
.
@milo enable
doesn't take any parameter? Is this correct?
It is theorethical propose only.
TL;DR
Same problem here
More
Custom PHPUnit hook raises an issue with a lot of escaped mutants created by Infection. Before implementation of the hook (and without call of BypassFinals::enable();
) all mutants were killed.
Details below 👇
declare(strict_types=1);
namespace App\PHPUnit\Hook;
use DG\BypassFinals;
use PHPUnit\Runner\BeforeTestHook;
final class BypassFinalsHook implements BeforeTestHook
{
public function executeBeforeTest(string $test): void
{
BypassFinals::enable();
}
}
<?xml version="1.0" encoding="UTF-8"?>
<!-- https://phpunit.readthedocs.io/en/latest/configuration.html -->
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="vendor/phpunit/phpunit/phpunit.xsd"
backupGlobals="false"
colors="true"
bootstrap="tests/bootstrap.php"
convertDeprecationsToExceptions="false"
executionOrder="random"
>
<!-- ... -->
<extensions>
<extension class="App\PHPUnit\Hook\BypassFinalsHook"/>
</extensions>
</phpunit>
For reference, see
- https://github.com/infection/infection/issues/1275#issuecomment-647138464
- https://github.com/infection/infection/issues/1275#issuecomment-647180895
- and not merged PR: https://github.com/infection/include-interceptor/pull/13
I didn't have a chance to finish it or came up with another idea, but this is a good start for further thinking
After spending hours trying to fix it on Infection side, I think it will be easier to fix it on this library, making another class that extends BypassFinals
or just patching BypassFinals
itself. Yeah, I understand that from SRP and just logically - BypassFinals should not be aware of Infection, same way as Infection should not be aware of BypassFinals, but since both libs are working with Stream Wrappers and stream wrappers are very easy being overridden - I see no other solution.
The simplest one that I came up with is the following:
https://github.com/dg/bypass-finals/blob/495f5bc762e7bf30a13ed8253f44bb3a701767bb/src/BypassFinals.php#L147
update this line of code and ask Infection for already overridden file, fetching $content
not from the original source code, but from already mutated code by Infection.
Pseudo code would be the following:
public function stream_open(string $path, string $mode, int $options, ?string &$openedPath): bool
{
// ...
if (getenv('INFECTION') === 1 && InfectionIncludeInterceptor::hasPath($path)) {
$content = InfectionIncludeInterceptor::getOverridden($path);
} else {
// existing BypassFinals code ...
$content = $this->native('file_get_contents', $path, $usePath, $this->context);
}
// ...
}
if @dg is interested in something like this, I can update infection/include-interceptor
to support this new API methods and make PoC for BypassFinals.
Hi @maks-rafalko. What do you think, would something like this work? That would just be BypassFinal run as a second one and it tries to call the previously defined wrapper.
https://github.com/dg/bypass-finals/commit/7c08b98712099224a53bdeb36a58f66e5cf73e61
I like the idea, however it doesn't work on my machine. How I tested it:
- created a branch with e2e test on Infection repository
- installed https://github.com/dg/bypass-finals/commit/7c08b98712099224a53bdeb36a58f66e5cf73e61
- ran some tests
How to reproduce and play with it:
git clone [email protected]:infection/infection.git
cd infection
git checkout bugfix/include-interceptor
# install Infection's dependencies
composer install
# go to e2e test directory
cd tests/e2e/Stream_Wrapper_Execution
# install e2e test dependencies
composer install
XDEBUG_MODE=coverage ../../../bin/infection --debug --log-verbosity=all
after that, if you open text.log
file to see what is the output of PHPUnit executed under Infection with BypassFinals enabled:
I see the following error:
PHPUnit 9.5.9 by Sebastian Bergmann and contributors.
Class "PHPUnit\Util\ErrorHandler" not found
Also, when I dump $meta
, there is no wrapper_data
key:
array(9) {
["timed_out"]=>
bool(false)
["blocked"]=>
bool(true)
["eof"]=>
bool(false)
["wrapper_type"]=>
string(9) "plainfile"
["stream_type"]=>
string(5) "STDIO"
["mode"]=>
string(1) "r"
["unread_bytes"]=>
int(0)
["seekable"]=>
bool(true)
["uri"]=>
string(112) "/path/to/infection/tests/e2e/Stream_Wrapper_Execution/vendor/dg/bypass-finals/src/BypassFinals.php"
}
It is PHP 8.0.10
@maks-rafalko It looks like you can't register another wrapper inside stream_open...
I tried a different approach https://github.com/dg/bypass-finals/commit/aa882485188494ae831da14d1139ed3bdd161e41
@dg this is much better!
I tested it for the repository/branch/e2e test I mentioned above and found 1 bug that 1 mutant from 5 still escaped. I was able to fix it and let me try to explain what happens.
TL;DR: everything works with your patch https://github.com/dg/bypass-finals/commit/aa882485188494ae831da14d1139ed3bdd161e41 combined with the patch below
In details:
in that e2e test I've created, there is a class SourceClass
that is not final (it's important) and for this file Infection does the following mutation
- public function getOne(): int
+ protected function getOne(): int
however it escapes, because original file is being used instead of mutated one. Why it happens? Let's closer look to your patched BypassFinal
, here
https://github.com/dg/bypass-finals/commit/aa882485188494ae831da14d1139ed3bdd161e41#diff-b0de2d102039b11e3840a14e052c230cfa1377774ce14d292adca84bcbccfbd9R169-R179
So, when self::$prevWrapper
is used and not null, it loads mutated version of SourceClass
thanks to these lines.
However, then it check $modified !== $content
which false
, because SourceClass
does not have final
keyword and BypassFinal
doesn't created tmp file, going forward to these lines where origin $path
is used and overrides all the $content
of Infection's interceptor.
So my patch is quite simple:
- if ($modified !== $content) {
+ if ($modified !== $content || (self::$prevWrapper && $content !== $this->native('file_get_contents', $path, $usePath, $this->context))) {
what it does basically is it stores in a temp file mutated (replaced) content of Infection's interceptor when original file does not contain final
keyword, and replaced (intercepted from Infection) file is used instead of original one, making it properly working.
Result: with you latest patch updated with the diff above - everything works great! This is a very exciting result.
What do you think about this patch? Do you think we can have it on BypassFinal source code?
By the way, we have a Discord channel, so if you think it will be quicker to discuss it online - feel free to join and ping me https://discord.gg/ZUmyHTJ
ping @dg, in case you missed it
@dg is there anything I can help with?
we are getting complaints from users that do not understand why Infection does not work when it should, and then it boils down to bypass-final
usage, so we decided to add a conflict
section with bypass-final
in our composer.json
: https://github.com/infection/infection/pull/1605
However, personally I don't like it, as it makes user to choose either to use infection but not this lib, or vice-versa.
At the same time, I don't want to spend time making a PR if you are not going to accept it with a solution above, so if you find a minute, could you please leave your feedback here?
Alternatively, we (Infection) can add another package that will be a wrapper around bypass-final
and do there the job from above comments, so users will have to install infection/bypass-final
instead of this lib. Again, it will solve the issue, but I would like to avoid it and rather complete the job here to support bypass-final
and Infection together natively.
Also, if you don't have enough time, does it make sense to add more collaborators to this project?
Thank you.
@maks-rafalko I've tried the patch and it doesn't seem to work for me anymore, will likely debug this further and create a PR.
Note: after opening #33, I've noticed that master incorporates the patch made by @dg here and it actually works with Infection 0.26.13 for me. I think this can be closed as soon as a bugfix release is made.
@dkarlovi I still reproduce the issue explained with details in my comment here: https://github.com/dg/bypass-finals/issues/9#issuecomment-939357946
So,
- yes, I confirm that most of the needed code is already in
master
branch of this repo - no, I can't confirm that it works
- my patch from that comment is still needed, thus your PR (https://github.com/dg/bypass-finals/pull/33) should be reopened
How you can test it yourself?
Look at this branch https://github.com/infection/infection/compare/bugfix/include-interceptor#diff-5447adae4da9395e026519944f362f9f7cd6986f5e5745677c9da1cbb4a3e672R5 and added e2e test.
git clone [email protected]:infection/infection.git
cd infection
git checkout bugfix/include-interceptor
# install Infection's dependencies
composer install
# go to e2e test directory
cd tests/e2e/Stream_Wrapper_Execution
# install e2e test dependencies
composer install
../../../bin/infection --debug --log-verbosity=all
1 Mutant is escaped (which is wrong). If you manually apply the patch in /tests/e2e/Stream_Wrapper_Execution/vendor/dg/bypass-finals/src/BypassFinals.php
:
- if ($modified !== $content) {
+ if ($modified !== $content || (self::$prevWrapper && $content !== $this->native('file_get_contents', $path, $usePath, $this->context))) {
then it works as expected - all the mutants are killed.
What are the next steps?
- let's reopen https://github.com/dg/bypass-finals/pull/33 and get it merged
- then do a release of this package
- then we will update
conflicts
section ininfection/infection
to allow new versions ofdg/bypass-finals
https://github.com/infection/infection/blob/d35e0795d64c2a1e030e63c51fbf6b37991b08d2/composer.json#L71
PR reopened.
Sorry I didn't have time to do this sooner, I maintain a lot of open source projects and I have hundreds of open issues, so everything takes annoyingly long time.
I tried to create a new solution and it seems that test tests/e2e/Stream_Wrapper_Execution
passes.
@dg I can confirm this commit fixes the issue and e2e test above is green!
could you make a release please? The last one was on September 2020. Then we can remove conflict
for dg/bypass-finals
on infection/infection
side.
Sorry I didn't have time to do this sooner, I maintain a lot of open source projects and I have hundreds of open issues, so everything takes annoyingly long time.
No need to apologize. The only one recommendation - does it make sense to add more collaborators you trust? So other developers can help with reviewing/merging/releasing.
Thanks you.
Good to hear that it works, I've released a new version.