Merge pull request #77 from shlomohass/main

Had issues on windows - fixed them + added custom descriptors support to Shell
adhocore · Feb 11, 2023 · 4c2bab4 · 4c2bab4
2 parents a91a04c + 3acd661
commit 4c2bab4
Show file tree

Hide file tree

Showing 10 changed files with 111 additions and 37 deletions.
diff --git a/.gitignore b/.gitignore
@@ -10,3 +10,4 @@ composer.lock
 clover.xml
 coverage.xml
 phpcs.xml
+.phpunit.result.cache
diff --git a/phpunit.xml.dist b/phpunit.xml.dist
@@ -6,7 +6,7 @@
  convertErrorsToExceptions="true"
  convertNoticesToExceptions="true"
  convertWarningsToExceptions="true"
- processIsolation="false"
+ processIsolation="true"
  stopOnFailure="false"
  bootstrap="tests/bootstrap.php"
  xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.3/phpunit.xsd">

diff --git a/src/Helper/Shell.php b/src/Helper/Shell.php
@@ -46,6 +46,15 @@ class Shell
     const STATE_CLOSED     = 'closed';
     const STATE_TERMINATED = 'terminated';
 
+    const DEFAULT_STDIN_WIN = ['pipe', 'r'];
+    const DEFAULT_STDIN_NIX = ['pipe', 'r'];
+
+    const DEFAULT_STDOUT_WIN = ['pipe', 'w'];
+    const DEFAULT_STDOUT_NIX = ['pipe', 'w'];
+
+    const DEFAULT_STDERR_WIN = ['pipe', 'w'];
+    const DEFAULT_STDERR_NIX = ['pipe', 'w'];
+
     /** @var bool Whether to wait for the process to finish or return instantly */
     protected bool $async = false;
 
@@ -98,25 +107,40 @@ public function __construct(protected string $command, protected ?string $input
         $this->input   = $input;
     }
 
-    protected function getDescriptors(): array
+    protected function prepareDescriptors(?array $stdin = null, ?array $stdout = null, ?array $stderr = null): array
     {
-        $out = $this->isWindows() ? ['file', 'NUL', 'w'] : ['pipe', 'w'];
-
+        $win = $this->isWindows();
+        if (!$stdin) {
+            $stdin = $win ? self::DEFAULT_STDIN_WIN : self::DEFAULT_STDIN_NIX;
+        }
+        if (!$stdout) {
+            $stdout = $win ? self::DEFAULT_STDOUT_WIN : self::DEFAULT_STDOUT_NIX;
+        }
+        if (!$stderr) {
+            $stderr = $win ? self::DEFAULT_STDERR_WIN : self::DEFAULT_STDERR_NIX;
+        }
         return [
-            self::STDIN_DESCRIPTOR_KEY  => ['pipe', 'r'],
-            self::STDOUT_DESCRIPTOR_KEY => $out,
-            self::STDERR_DESCRIPTOR_KEY => $out,
+            self::STDIN_DESCRIPTOR_KEY  => $stdin,
+            self::STDOUT_DESCRIPTOR_KEY => $stdout,
+            self::STDERR_DESCRIPTOR_KEY => $stderr,
         ];
     }
 
     protected function isWindows(): bool
     {
-        return '\\' === DIRECTORY_SEPARATOR;
+        // If PHP_OS is defined, use it - More reliable:
+        if (defined('PHP_OS')) {
+            return 'WIN' === strtoupper(substr(PHP_OS, 0, 3)); // May be 'WINNT' or 'WIN32' or 'Windows'
+        }
+        return '\\' === DIRECTORY_SEPARATOR; // Fallback - Less reliable (Windows 7...)
     }
 
     protected function setInput(): void
     {
-        fwrite($this->pipes[self::STDIN_DESCRIPTOR_KEY], $this->input ?? '');
+        //Make sure the pipe is a stream resource before writing to it to avoid a warning
+        if (is_resource($this->pipes[self::STDIN_DESCRIPTOR_KEY])) {
+            fwrite($this->pipes[self::STDIN_DESCRIPTOR_KEY], $this->input ?? '');
+        }
     }
 
     protected function updateProcessStatus(): void
@@ -132,9 +156,16 @@ protected function updateProcessStatus(): void
 
     protected function closePipes(): void
     {
-        fclose($this->pipes[self::STDIN_DESCRIPTOR_KEY]);
-        fclose($this->pipes[self::STDOUT_DESCRIPTOR_KEY]);
-        fclose($this->pipes[self::STDERR_DESCRIPTOR_KEY]);
+        //Make sure the pipe are a stream resource before closing them to avoid a warning
+        if (is_resource($this->pipes[self::STDIN_DESCRIPTOR_KEY])) {
+            fclose($this->pipes[self::STDIN_DESCRIPTOR_KEY]);
+        }
+        if (is_resource($this->pipes[self::STDOUT_DESCRIPTOR_KEY])) {
+            fclose($this->pipes[self::STDOUT_DESCRIPTOR_KEY]);
+        }
+        if (is_resource($this->pipes[self::STDERR_DESCRIPTOR_KEY])) {
+            fclose($this->pipes[self::STDERR_DESCRIPTOR_KEY]);
+        }
     }
 
     protected function wait(): ?int
@@ -177,14 +208,25 @@ public function setOptions(
 
         return $this;
     }
-
-    public function execute(bool $async = false): self
+
+    /**
+     * execute
+     * Execute the command with optional stdin, stdout and stderr which override the defaults
+     * If async is set to true, the process will be executed in the background
+     * 
+     * @param  bool $async - default false
+     * @param  ?array $stdin - default null (loads default descriptor)
+     * @param  ?array $stdout - default null (loads default descriptor)
+     * @param  ?array $stderr - default null (loads default descriptor)
+     * @return self
+     */
+    public function execute(bool $async = false, ?array $stdin = null, ?array $stdout = null, ?array $stderr = null): self
     {
         if ($this->isRunning()) {
             throw new RuntimeException('Process is already running.');
         }
 
-        $this->descriptors      = $this->getDescriptors();
+        $this->descriptors      = $this->prepareDescriptors($stdin, $stdout, $stderr);
         $this->processStartTime = microtime(true);
 
         $this->process = proc_open(
@@ -218,6 +260,10 @@ public function execute(bool $async = false): self
 
     private function setOutputStreamNonBlocking(): bool
     {
+        // Make sure the pipe is a stream resource before setting it to non-blocking to avoid a warning
+        if (!is_resource($this->pipes[self::STDOUT_DESCRIPTOR_KEY])) {
+            return false;
+        }
         return stream_set_blocking($this->pipes[self::STDOUT_DESCRIPTOR_KEY], false);
     }
 
@@ -228,11 +274,18 @@ public function getState(): string
 
     public function getOutput(): string
     {
+        // Make sure the pipe is a stream resource before reading it to avoid a warning
+        if (!is_resource($this->pipes[self::STDOUT_DESCRIPTOR_KEY])) {
+            return '';
+        }
         return stream_get_contents($this->pipes[self::STDOUT_DESCRIPTOR_KEY]);
     }
 
     public function getErrorOutput(): string
     {
+        if (!is_resource($this->pipes[self::STDERR_DESCRIPTOR_KEY])) {
+            return '';
+        }
         return stream_get_contents($this->pipes[self::STDERR_DESCRIPTOR_KEY]);
     }
 

diff --git a/tests/ApplicationTest.php b/tests/ApplicationTest.php
@@ -24,14 +24,19 @@ class ApplicationTest extends TestCase
 
     public function setUp(): void
     {
-        file_put_contents(static::$in, '');
-        file_put_contents(static::$ou, '');
+        file_put_contents(static::$in, '', LOCK_EX);
+        file_put_contents(static::$ou, '', LOCK_EX);
     }
 
     public function tearDown(): void
     {
-        unlink(static::$in);
-        unlink(static::$ou);
+        // Make sure we clean up after ourselves:
+        if (file_exists(static::$in)) {
+            unlink(static::$in);
+        }
+        if (file_exists(static::$ou)) {
+            unlink(static::$ou);
+        }
     }
 
     public function test_new()

diff --git a/tests/CliTestCase.php b/tests/CliTestCase.php
@@ -36,7 +36,7 @@ public function setUp(): void
     {
         ob_start();
         StreamInterceptor::$buffer = '';
-        file_put_contents(static::$ou, '');
+        file_put_contents(static::$ou, '', LOCK_EX);
     }
 
     public function tearDown(): void
@@ -46,7 +46,10 @@ public function tearDown(): void
 
     public static function tearDownAfterClass(): void
     {
-        unlink(static::$ou);
+        // Make sure we clean up after ourselves:
+        if (file_exists(static::$ou)) {
+            unlink(static::$ou);
+        }
     }
 
     public function buffer()

diff --git a/tests/Helper/OutputHelperTest.php b/tests/Helper/OutputHelperTest.php
@@ -29,12 +29,15 @@ class OutputHelperTest extends TestCase
 
     public function setUp(): void
     {
-        file_put_contents(static::$ou, '');
+        file_put_contents(static::$ou, '', LOCK_EX);
     }
 
     public static function tearDownAfterClass(): void
     {
-        unlink(static::$ou);
+        // Make sure we clean up after ourselves:
+        if (file_exists(static::$ou)) {
+            unlink(static::$ou);
+        }
     }
 
     public function test_show_arguments()

diff --git a/tests/Helper/ShellTest.php b/tests/Helper/ShellTest.php
@@ -24,7 +24,7 @@ public function test_get_output()
 
         $shell->execute();
 
-        $this->assertSame("hello\n", $shell->getOutput());
+        $this->assertSame("hello", trim($shell->getOutput())); // trim to remove trailing newline which is OS dependent
         $this->assertSame(0, $shell->getExitCode());
     }
 
@@ -35,7 +35,7 @@ public function test_get_process_id()
         $shell->execute(true);
 
         $this->assertIsInt($pid = $shell->getProcessId());
-        $this->assertGreaterThan(getmypid(), $pid);
+        // $this->assertGreaterThan(getmypid(), $pid); // this is not always true especially on windows
     }
 
     public function test_async_stop()

diff --git a/tests/IO/InteractorTest.php b/tests/IO/InteractorTest.php
@@ -24,14 +24,19 @@ class InteractorTest extends TestCase
 
     public function setUp(): void
     {
-        file_put_contents(static::$in, '');
-        file_put_contents(static::$ou, '');
+        file_put_contents(static::$in, '', LOCK_EX);
+        file_put_contents(static::$ou, '', LOCK_EX);
     }
 
     public function tearDown(): void
     {
-        unlink(static::$in);
-        unlink(static::$ou);
+        // Make sure we clean up after ourselves:
+        if (file_exists(static::$in)) {
+            unlink(static::$in);
+        }
+        if (file_exists(static::$ou)) {
+            unlink(static::$ou);
+        }
     }
 
     public function test_components()
@@ -152,7 +157,7 @@ public function test_call()
 
     protected function newInteractor(string $in = '')
     {
-        file_put_contents(static::$in, $in);
+        file_put_contents(static::$in, $in, LOCK_EX);
 
         return new Interactor(static::$in, static::$ou);
     }

diff --git a/tests/Input/ReaderTest.php b/tests/Input/ReaderTest.php
@@ -21,12 +21,15 @@ class ReaderTest extends TestCase
 
     public function setUp(): void
     {
-        file_put_contents(static::$in, '');
+        file_put_contents(static::$in, '', LOCK_EX);
     }
 
     public static function tearDownAfterClass(): void
     {
-        unlink(static::$in);
+        // Make sure we clean up after ourselves:
+        if (file_exists(static::$in)) {
+            unlink(static::$in);
+        }
     }
 
     public function test_default()
@@ -38,7 +41,7 @@ public function test_default()
 
     public function test_callback()
     {
-        file_put_contents(static::$in, 'the value');
+        file_put_contents(static::$in, 'the value', LOCK_EX);
 
         $r = new Reader(static::$in);
 

diff --git a/tests/Output/ColorTest.php b/tests/Output/ColorTest.php
@@ -52,12 +52,13 @@ public function test_colors()
     {
         $c = new Color;
 
-        $this->assertSame("\nabc\n", $c->colors('<eol>abc</eol>'));
+        // We use PHP_EOL here because it is platform dependent and eol tag will be replaced by it.
+        $this->assertSame(PHP_EOL."abc".PHP_EOL, $c->colors('<eol>abc</eol>'));
         $this->assertSame("\033[0;31mRed\033[0m", $c->colors('<red>Red</end>'));
-        $this->assertSame("\033[1;31mBoldRed\n\033[0m", $c->colors('<boldRed>BoldRed<eol/></end>'));
-        $this->assertSame("\033[0;36;42mBgGreenCyan\033[0m\n", $c->colors('<bgGreenCyan>BgGreenCyan</end><eol>'));
+        $this->assertSame("\033[1;31mBoldRed".PHP_EOL."\033[0m", $c->colors('<boldRed>BoldRed<eol/></end>'));
+        $this->assertSame("\033[0;36;42mBgGreenCyan\033[0m".PHP_EOL, $c->colors('<bgGreenCyan>BgGreenCyan</end><eol>'));
         $this->assertSame(
-            "\033[0;31mRed\033[0m\nNormal\n\033[1;37mBOLD\033[0m",
+            "\033[0;31mRed\033[0m".PHP_EOL."Normal".PHP_EOL."\033[1;37mBOLD\033[0m",
             $c->colors("<red>Red</end>\r\nNormal\n<bold>BOLD</end>")
         );
     }