New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Using Infection library fails because of another stream wrapper #9
Comments
Maybe chaining like |
@milo |
It is theorethical propose only. |
TL;DRSame problem here MoreCustom PHPUnit hook raises an issue with a lot of escaped mutants created by Infection. Before implementation of the hook (and without call of 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
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 The simplest one that I came up with is the following: bypass-finals/src/BypassFinals.php Line 147 in 495f5bc
update this line of code and ask Infection for already overridden file, fetching 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 |
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. |
I like the idea, however it doesn't work on my machine. How I tested it:
How to reproduce and play with it: git clone git@github.com: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 I see the following error:
Also, when I dump 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 |
@maks-rafalko It looks like you can't register another wrapper inside stream_open... I tried a different approach aa88248 |
@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 aa88248 combined with the patch below In details: in that e2e test I've created, there is a class - 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 aa88248#diff-b0de2d102039b11e3840a14e052c230cfa1377774ce14d292adca84bcbccfbd9R169-R179 So, when However, then it check 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 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 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 Also, if you don't have enough time, does it make sense to add more collaborators to this project? Thank you. |
This package conflicts with infection/infection which will be required in a further commit. See also: dg/bypass-finals#9
@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. |
@dkarlovi I still reproduce the issue explained with details in my comment here: #9 (comment) So,
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 git@github.com: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 - 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?
|
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 |
fixed interoperability with Infection library
@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
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. |
fixed interoperability with Infection library
Good to hear that it works, I've released a new version. |
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 🙂 ?
The text was updated successfully, but these errors were encountered: