Skip to content
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

Enhancement: Implement ArgumentRemovalArrayReplace mutator #565

Conversation

localheinz
Copy link
Member

This PR

Related to #537.

@localheinz
Copy link
Member Author

localheinz commented Nov 10, 2018

When I add the following test case

diff --git a/tests/Mutator/ArgumentRemoval/ArgumentRemovalArrayReplaceTest.php b/tests/Mutator/ArgumentRemoval/ArgumentRemovalArrayReplaceTest.php
index 69f96d2..b542f13 100644
--- a/tests/Mutator/ArgumentRemoval/ArgumentRemovalArrayReplaceTest.php
+++ b/tests/Mutator/ArgumentRemoval/ArgumentRemovalArrayReplaceTest.php
@@ -141,6 +141,29 @@ PHP
             ],
         ];

+        yield 'It mutates correctly when there is a backslash in front of array_replace' => [
+            <<<'PHP'
+<?php
+
+$a = \array_replace(['A', 1, 'C'], ['D']);
+PHP
+            ,
+            [
+                <<<'PHP'
+<?php
+
+$a = \array_replace(['D']);
+PHP
+                ,
+                <<<'PHP'
+<?php
+
+$a = \array_replace(['A', 1, 'C']);
+PHP
+                ,
+            ],
+        ];
+
         yield 'It mutates correctly when array_replace uses constants as input' => [
             <<<'PHP'
 <?php

and run

$  ./vendor/bin/phpunit tests/Mutator/ArgumentRemoval/ArgumentRemovalArrayReplaceTest.php

then the tests pass, but warnings are emitted:

PHPUnit 6.5.13 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.1.22
Configuration: /Users/am/Sites/infection/infection/phpunit.xml.dist

......PHP Warning:  Unexpected character in input:  '' (ASCII=7) state=0 in - on line 3
PHP Warning:  Unexpected character in input:  '' (ASCII=7) state=0 in - on line 3
......                                                      12 / 12 (100%)

Time: 910 ms, Memory: 12.00MB

OK (12 tests, 46 assertions)

Does anyone have an idea how to sort this out?

@sanmai
Copy link
Member

sanmai commented Nov 11, 2018

ASCII 7 aka Bell sneaked through somewhere?

@localheinz
Copy link
Member Author

@sanmai

Yes, right here:

exec(sprintf('echo %s | php -l', escapeshellarg($realMutatedCode)), $output, $returnCode);

@sanmai
Copy link
Member

sanmai commented Nov 11, 2018

Well, that bit just checks if a test contains a valid PHP code. It is somewhere in ArgumentRemovalArrayReplaceTest.php or ArgumentRemovalArrayReplace.php

@sanmai sanmai added the Mutator label Nov 11, 2018
@sanmai sanmai mentioned this pull request Nov 12, 2018
4 tasks
@localheinz localheinz force-pushed the feature/argument-removal-array-replace branch 2 times, most recently from d54e37e to 0d22be3 Compare November 14, 2018 16:49
@localheinz
Copy link
Member Author

localheinz commented Nov 14, 2018

@sanmai

What do you think about 05639f3 627096f?

Copied and adjusted from the examples at http://php.net/proc_open.

@localheinz localheinz force-pushed the feature/argument-removal-array-replace branch from 05639f3 to 627096f Compare November 15, 2018 07:20
@sanmai
Copy link
Member

sanmai commented Nov 15, 2018

It should be more like this: #301 (comment)

Though I'm not entirely sure everything is in order there (proc_open is a bit tricky about closed-read pipes). So I'd make a separate function, and a test for it with a known invalid code.

static function isCodeValid($code) { ... }
function test_is_code_valid_sees_invalid_code() {  $this->assertFalse(self::isCodeValid('<?php invalid=1')); }
function assertCodeValid($code) { $this->assertTrue(self::isCodeValid($code));  }

Original argument in #301 against proc_open was that it may not be supported on all platforms. But we use Symfony's Process which does require proc_open to be present, so that's not a very valid argument: if there's not a proc_open present, you can't even run Infection itself.

@localheinz localheinz force-pushed the feature/argument-removal-array-replace branch from 627096f to f57a729 Compare December 8, 2018 08:49
@localheinz localheinz force-pushed the feature/argument-removal-array-replace branch from f57a729 to e37f0f6 Compare December 8, 2018 12:01
@localheinz localheinz closed this Feb 22, 2020
@localheinz localheinz deleted the feature/argument-removal-array-replace branch February 22, 2020 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants