Add FakeRunner which will be emulate running command #21

Bukharovsi · 2017-01-30T09:22:47Z

I suggest add FakeRunner which will be emulate running the command.
It must be used in unit and integration tests.
Also i suggest add EchoFakeRunner which will be not execute command but prints the command. It will be used when application runs with --dry-run argument

I suggest add FakeRunner which will be emulate running the command. It must be used in unit and integration tests. Also i suggest add EchoFakeRunner which will be not execute command but prints the command. It will be used when application runs with --dry-run argument

fix EchoFakeRunner namespace. add tests

adambrett · 2017-01-31T09:11:51Z

@Bukharovsi thanks a lot for this; I think it will be a really useful addition to the library so I'm keen to get it merged.

At first glance, there's a couple of things I'd like to tidy up before it's merged. When I get some time later I'll go through and add some comments!

Thanks

Bukharovsi · 2017-01-31T13:53:09Z

@adambrett thanks for response! Could you please explain what do you want to improve? i will fix it.

adambrett

@Bukharovsi I've added a couple of comments, it's mostly ok.

The only other thing I would like to see is the removal of the docblocks and phpstorm generated file blocks. I don't use them anywhere else in the library and think the code should be self-explanitory enough.

The last thing is that the functionality needs documenting in the relevant places in the README and then we're good to go.

Thanks

adambrett · 2017-01-31T14:26:58Z

tests/unit-tests/Runners/EchoFakeRunnerTest.php

+    public function testRun()
+    {
+        $runner = new EchoFakeRunner();
+        $runner->run(new Command('ls'));


could you wrap this in ob_start and ob_get_clean to test that the output matches the expected value?

adambrett · 2017-01-31T14:28:02Z

src/Runners/EchoFakeRunner.php

+use AdamBrett\ShellWrapper\Command\CommandInterface;
+use AdamBrett\ShellWrapper\Runners\FakeRunner;
+
+class EchoFakeRunner extends FakeRunner


I think this should just be called DryRunner. I don't think that it matters that it extends/is Fake

Bukharovsi · 2017-01-31T19:52:17Z

Thank you for feedback. I will fix it soon!

Dockblocks has been removed Auto-generated dockblocks has been removed test for command printing in DryRunnerTest has been added add \r\n for DryRunner

Bukharovsi · 2017-01-31T21:25:25Z

I have fixed problems. Please pay attention with README. I could make some mistakes.

adambrett

Looking good, just a couple of minor fixes I missed the first time around and we can get this merged!

adambrett · 2017-02-02T09:24:35Z

src/Runners/DryRunner.php

+{
+    public function run(CommandInterface $command)
+    {
+        echo $command."\r\n";


I didn't spot this first time around, could you replace this with the line ending constant PHP_EOL and put spaces around the dot?

adambrett · 2017-02-02T09:25:14Z

tests/unit-tests/Runners/DryRunnerTest.php

+        $runner = new DryRunner();
+        ob_start();
+        $runner->run(new Command('ls'));
+        static::assertEquals("ls\r\n", ob_get_clean(), 'This runner must print command');


should probably be PHP_EOL here too

adambrett · 2017-02-02T09:25:33Z

tests/unit-tests/Runners/FakeRunnerTest.php

@@ -0,0 +1,41 @@
+<?php
+/**


Missed this docblock

adambrett · 2017-02-02T09:26:35Z

README.md

+Dry*      | Exit code             |       | x         | x
+Fake*     | Exit code             |       | x         | x
+
+Use `FakeRunner` into unit tests for emulate command running. 


How about:

"You can use FakeRunner in your unit tests to emulate running a command."

adambrett · 2017-02-02T09:27:34Z

README.md

+Fake*     | Exit code             |       | x         | x
+
+Use `FakeRunner` into unit tests for emulate command running. 
+Use `DryRunner` when your application gets --dry-run argument and there is need to only print the command 


And:

"You can use DryRunner for debugging purposes, or when your application uses a --dry-run type argument and you want to echo the command rather than run it."

Bukharovsi · 2017-02-03T12:27:56Z

Thanks for response! I have fixed this!

adambrett

Awesome, thanks. I think you missed a couple of comments though. I just commented again to show what I mean. Once those two bits are sorted I'll get this merged!

Thanks

adambrett · 2017-02-03T18:08:28Z

tests/unit-tests/Runners/DryRunnerTest.php

+        $runner = new DryRunner();
+        ob_start();
+        $runner->run(new Command('ls'));
+        static::assertEquals("ls".PHP_EOL, ob_get_clean(), 'This runner must print command');


this should be single quotes and there should be a space around the .

static::assertEquals('ls' . PHP_EOL, ob_get_clean(), 'This runner must print command');

adambrett · 2017-02-03T18:09:01Z

src/Runners/DryRunner.php

+{
+    public function run(CommandInterface $command)
+    {
+        echo $command.PHP_EOL;


There should be a space around the . here

echo $command . PHP_EOL;

Bukharovsi · 2017-02-03T19:55:05Z

done!

thanks!

Bukharovsi · 2017-02-04T11:10:43Z

Excellent! thanks!

Bukharov added 3 commits January 30, 2017 12:21

Issue #20 Add FakeRunner for testing application

aea5d10

fix EchoFakeRunner namespace. add tests

php-cs fixing errors

c68bd2a

Bukharovsi changed the title ~~issue #20 https://github.com/adambrett/php-shell-wrapper/issues/20~~ Add FakeRunner whic will be emulate running command Jan 30, 2017

Bukharovsi changed the title ~~Add FakeRunner whic will be emulate running command~~ Add FakeRunner which will be emulate running command Jan 30, 2017

adambrett requested changes Jan 31, 2017

View reviewed changes

Bukharov added 2 commits February 1, 2017 00:06

Echo fake runner renamed to DryRunner

1395cac

Dockblocks has been removed Auto-generated dockblocks has been removed test for command printing in DryRunnerTest has been added add \r\n for DryRunner

it adds some information in README about DryRunner and FakeRunner

b41f459

adambrett requested changes Feb 2, 2017

View reviewed changes

fix some things after CR

15ae9e2

adambrett requested changes Feb 3, 2017

View reviewed changes

Bukharov added 2 commits February 3, 2017 22:44

add spaces around string glue

293ecc8

" -> '

42d7b31

adambrett merged commit 16aced8 into adambrett:master Feb 4, 2017

Bukharovsi deleted the fake_wrapper branch February 4, 2017 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FakeRunner which will be emulate running command #21

Add FakeRunner which will be emulate running command #21

Bukharovsi commented Jan 30, 2017

adambrett commented Jan 31, 2017

Bukharovsi commented Jan 31, 2017

adambrett left a comment

adambrett Jan 31, 2017

adambrett Jan 31, 2017

Bukharovsi commented Jan 31, 2017

Bukharovsi commented Jan 31, 2017

adambrett left a comment

adambrett Feb 2, 2017

adambrett Feb 2, 2017

adambrett Feb 2, 2017

adambrett Feb 2, 2017

adambrett Feb 2, 2017

Bukharovsi commented Feb 3, 2017

adambrett left a comment

adambrett Feb 3, 2017

adambrett Feb 3, 2017

Bukharovsi commented Feb 3, 2017

Bukharovsi commented Feb 4, 2017

		@@ -0,0 +1,41 @@
		<?php
		/**

Add FakeRunner which will be emulate running command #21

Add FakeRunner which will be emulate running command #21

Conversation

Bukharovsi commented Jan 30, 2017

adambrett commented Jan 31, 2017

Bukharovsi commented Jan 31, 2017

adambrett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bukharovsi commented Jan 31, 2017

Bukharovsi commented Jan 31, 2017

adambrett left a 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

Choose a reason for hiding this comment

Bukharovsi commented Feb 3, 2017

adambrett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bukharovsi commented Feb 3, 2017

Bukharovsi commented Feb 4, 2017