Allow relative path for `tmpDir` config setting. #151

maks-rafalko · 2018-02-03T20:01:04Z

Allow relative path for tmpDir config setting.
Fix bug with e2e tests with checking $PHPDBG env var.
Update documentation with new relative feature (infection/site@f5462d3)

sidz · 2018-02-03T20:12:44Z

src/Filesystem/Filesystem.php

+     *
+     * @return bool
+     */
+    public function isAbsolutePath(string $file): bool


is this a time to add a new dependency? :D

lol, expected question. I still don't know.

I think it's time to, it's the 3rd method we are copy-pasting from it :) Besides it's not like filesystem is a huge dependency, it's quite slim and don't change much

@borNfreee let's do this. If you don't want to do it in the current PR let's do it as separate.

got it, I will do it in this PR

sidz · 2018-02-03T20:15:04Z

could you please check travis ^^?

sidz · 2018-02-03T20:21:26Z

src/Config/InfectionConfig.php

-        return $this->config->tmpDir ?? sys_get_temp_dir();
+        $tmpDir = $this->config->tmpDir ?? sys_get_temp_dir();
+
+        if ($this->filesystem->isAbsolutePath($tmpDir)) {


I'd say that we don't need to execute this line for sys_get_temp_dir.

so it's actually only when $this->config->tmpDir is not empty

sidz · 2018-02-03T20:22:36Z

tests/Fixtures/e2e/standard_script.bash

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash

-if [[ $PHPDBG=1 ]]
+if [ "$PHPDBG" = "1" ]


what was the issue? on linux it works fine

on macos it was always true

I couldn't reproduce it, might be a different shell (I'm using zsh), but in any case it's better to compare strings in bash

if [[ $PHPDBG=1 ]] does not work on Terminal app in MacOS (while works on zsh). See image below. Also, if [[ $PHPDBG=1 ]] is not the same as if [[ $PHPDBG = 1 ]]

maks-rafalko · 2018-02-03T20:42:45Z

could you please check travis ^^?

seems like something wrong with PHPCSFixer. New version has been downloaded on Travis and it wants to remove blank line after license, while we have

'header_comment' => [
            'commentType' => 'PHPDoc',
            'header' => $header,
            'location' => 'after_open',
            'separate' => 'bottom', <-------
        ],

Conflict between header_comment.separate style and no_blank_lines_after_phpdoc :|

sidz · 2018-02-03T20:44:43Z

src/Config/InfectionConfig.php

@@ -251,6 +258,17 @@ function ($excludeDir) use ($srcDir) {

    public function getTmpDir(): string
    {
-        return $this->config->tmpDir ?? sys_get_temp_dir();
+


remove additional new line

sidz · 2018-02-03T20:47:38Z

src/Config/InfectionConfig.php

@@ -251,6 +258,17 @@ function ($excludeDir) use ($srcDir) {

    public function getTmpDir(): string
    {
-        return $this->config->tmpDir ?? sys_get_temp_dir();
+
+        if (empty($this->config->tmpDir)) {


maybe we need to throw an exception for case when "tmpDir": "" instead of use sys_temp_dir? what do you think?

it's better to continue working if we can rather than throw an exception.

I mean even if we throw an exception here, Infection should fall back to sys_temp_dir since this is not critical mistake and does not block mutation testing. WDYT?

imho to be user friendly we need to show warning message in this case if we don't want to throw an exception. (it's not easy to do it right now)
so feel free to leave it as is

p.s. could we update documentation to include this case?

forgot about it, will do, thank you

maks-rafalko · 2018-02-03T21:18:46Z

Created an issue in PHPCSFixer repo about failures on Travis with the new downloaded 2.10.1 version: PHP-CS-Fixer/PHP-CS-Fixer#3503

theofidry · 2018-02-04T21:18:05Z

src/Filesystem/Filesystem.php

+     *
+     * @return bool
+     */
+    public function isAbsolutePath(string $file): bool


I think it's time to, it's the 3rd method we are copy-pasting from it :) Besides it's not like filesystem is a huge dependency, it's quite slim and don't change much

theofidry · 2018-02-04T21:18:40Z

tests/Fixtures/e2e/Custom_tmp_dir/composer.json

@@ -0,0 +1,20 @@
+{


we're using 4 spaces in other files aren't we?

theofidry · 2018-02-04T21:18:50Z

tests/Fixtures/e2e/Custom_tmp_dir/composer.json

+      "Namespace_\\Test\\": "tests/"
+    }
+  }
+}


missing line return

theofidry · 2018-02-04T21:19:56Z

tests/Fixtures/e2e/Custom_tmp_dir/phpunit.xml

+         backupStaticAttributes="false"
+         bootstrap="./vendor/autoload.php"
+         colors="true"
+         convertErrorsToExceptions="true"


aren't those the default? You can easily find out with phpstorm and adding xsi:noNamespaceSchemaLocation="http://schema.phpunit.de/6.4/phpunit.xsd" e.g. https://github.com/humbug/box/blob/master/phpunit.xml.dist#L3

theofidry · 2018-02-04T21:20:37Z

tests/Fixtures/e2e/Custom_tmp_dir/tests/SourceClassTest.php

+
+use Namespace_\SourceClass;
+
+class SourceClassTest extends \PHPUnit\Framework\TestCase


Not a big deal but I find better to be consistent and import this class with a use statement?

theofidry · 2018-02-04T21:22:34Z

tests/Fixtures/e2e/standard_script.bash

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash

-if [[ $PHPDBG=1 ]]
+if [ "$PHPDBG" = "1" ]


I couldn't reproduce it, might be a different shell (I'm using zsh), but in any case it's better to compare strings in bash

For infection/infection#151

maks-rafalko · 2018-02-05T20:06:20Z

All comments addressed, including documentation. Please review again 😋

I decided to keep IOException since a like named constructors implemented in our version

sidz · 2018-02-05T20:13:51Z

tests/Fixtures/e2e/Custom_tmp_dir/infection.json

@@ -0,0 +1,12 @@
+{
+  "timeout": 25,


4 spaces :P

…ts with checking $PHPDBG env var

…nore

maks-rafalko added the Enhancement label Feb 3, 2018

maks-rafalko added this to the 0.8.0 milestone Feb 3, 2018

maks-rafalko requested review from sidz and theofidry February 3, 2018 20:01

sidz reviewed Feb 3, 2018

View reviewed changes

sidz approved these changes Feb 3, 2018

View reviewed changes

sidz reviewed Feb 3, 2018

View reviewed changes

maks-rafalko force-pushed the relative-tmp-dir branch from 2c5a3ab to 43d56ab Compare February 3, 2018 20:45

sidz reviewed Feb 3, 2018

View reviewed changes

maks-rafalko mentioned this pull request Feb 4, 2018

Fix: Run 'composer cs:fix' #153

Merged

1 task

maks-rafalko force-pushed the relative-tmp-dir branch from 43d56ab to 4e0ab98 Compare February 4, 2018 08:14

theofidry approved these changes Feb 4, 2018

View reviewed changes

This was referenced Feb 5, 2018

Tests: add a phpspec & phpunit comparison test #144

Merged

Added support for Codeception test framework #143

Closed

maks-rafalko added a commit to infection/site that referenced this pull request Feb 5, 2018

Document ability to use relative folder for tmpDir setting

f5462d3

For infection/infection#151

maks-rafalko force-pushed the relative-tmp-dir branch from 4e0ab98 to 42380fc Compare February 5, 2018 20:04

sidz approved these changes Feb 5, 2018

View reviewed changes

maks-rafalko force-pushed the relative-tmp-dir branch from 42380fc to 716fb20 Compare February 5, 2018 20:15

maks-rafalko added 6 commits February 6, 2018 22:00

Allow relative path for tmpDir config setting. Fix bug with e2e tes…

c88ec0f

…ts with checking $PHPDBG env var

Simplify checks for tmpDir, add more tests

60f151b

Rebase on master. Add tests composer script

13ded1a

Use symfony/filesystem instead of custom implementation

df67fb7

Cleanup e2e tests, update phpunit.xml with the XSD schema

92eba9b

Rebase, refactor bew e2e tests from master. Add private key to git ig…

c6772f9

…nore

maks-rafalko force-pushed the relative-tmp-dir branch from 716fb20 to c6772f9 Compare February 6, 2018 19:09

maks-rafalko merged commit c0cd06c into master Feb 6, 2018

maks-rafalko deleted the relative-tmp-dir branch February 6, 2018 19:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow relative path for `tmpDir` config setting. #151

Allow relative path for `tmpDir` config setting. #151

maks-rafalko commented Feb 3, 2018 •

edited

Loading

sidz Feb 3, 2018

maks-rafalko Feb 3, 2018

theofidry Feb 4, 2018

sidz Feb 5, 2018

maks-rafalko Feb 5, 2018

sidz commented Feb 3, 2018

sidz Feb 3, 2018

maks-rafalko Feb 3, 2018

sidz Feb 3, 2018

maks-rafalko Feb 3, 2018

theofidry Feb 4, 2018

maks-rafalko Feb 5, 2018

maks-rafalko commented Feb 3, 2018 •

edited

Loading

sidz Feb 3, 2018

sidz Feb 3, 2018

maks-rafalko Feb 3, 2018

sidz Feb 3, 2018 •

edited

Loading

maks-rafalko Feb 4, 2018

maks-rafalko commented Feb 3, 2018

theofidry Feb 4, 2018

theofidry Feb 4, 2018

theofidry Feb 4, 2018

theofidry Feb 4, 2018

theofidry Feb 4, 2018

theofidry Feb 4, 2018

maks-rafalko commented Feb 5, 2018 •

edited

Loading

sidz Feb 5, 2018

maks-rafalko Feb 5, 2018


		use Namespace_\SourceClass;

		class SourceClassTest extends \PHPUnit\Framework\TestCase

Allow relative path for tmpDir config setting. #151

Allow relative path for tmpDir config setting. #151

Conversation

maks-rafalko commented Feb 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidz commented Feb 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maks-rafalko commented Feb 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidz Feb 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maks-rafalko commented Feb 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maks-rafalko commented Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Allow relative path for `tmpDir` config setting. #151

Allow relative path for `tmpDir` config setting. #151

maks-rafalko commented Feb 3, 2018 •

edited

Loading

maks-rafalko commented Feb 3, 2018 •

edited

Loading

sidz Feb 3, 2018 •

edited

Loading

maks-rafalko commented Feb 5, 2018 •

edited

Loading