-
Notifications
You must be signed in to change notification settings - Fork 430
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
580: Adds support for --tasks option in run command. #582
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @paslandau,
Thanks for the PR.
I've added some remarks. Can you take a look at them?
We are transforming the codebase to strict types at the moment.
Can you change the target branch to grumpy-seventies
and add the correct typehints?
I hope to get that branch released soon... :)
public function filterByTaskName($tasks) | ||
{ | ||
return $this->filter(function (TaskInterface $task) use ($tasks) { | ||
if (empty($tasks)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to iterate through the list when the tasks are empty. You can put the empty check above the filter function and return a new collection through return new TasksCollection($this->toArray());
See \GrumPHP\Collection\TasksCollection::filterByTestSuite()
* | ||
* @return TasksCollection | ||
*/ | ||
public function filterByTaskName($tasks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rename this one to filterByTaskNames
?
if (empty($tasks)) { | ||
return true; | ||
} | ||
return in_array($task->getName(), $tasks); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can set the third argument to true here.
@@ -66,9 +73,12 @@ public function execute(InputInterface $input, OutputInterface $output) | |||
$files = $this->getRegisteredFiles(); | |||
$testSuites = $this->grumPHP->getTestSuites(); | |||
|
|||
$tasks = $this->parseCommaSeparatedOption($input->getOption("tasks")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$tasks = $this->parseCommaSeparatedOption($input->getOption("tasks")); | |
$tasks = array_map('trim', explode(',', $input->getOption('tasks', ''))); |
The de-duplicating logic is a bit confusing and is not required since the filter method on the taskcollection is smart enough. I would prefer to drop it for the single line suggested above. (optionally you could add array_filter to drop the empty items, but that is also covered by the filter method)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will retain empty input values, i.e. if --tasks
is empty or omitted, $tasks
will be an array containing one empty element. So we would either put it in an if OR add and array_filter
--- both makes the result less readable, imho.
I'm fine with removing deduplication, though.
Thoughts?
fyi - failing tests after change:
/codebase/grumphp/vendor/phpunit/phpunit/phpunit --configuration /codebase/grumphp/phpunit.xml.dist GrumPHPTest\\Console\\Command\\RunCommandTest /codebase/grumphp/test/Console/Command/RunCommandTest.php
[...]
4) GrumPHPTest\Console\Command\RunCommandTest::parses_comma_separated_options with data set "null" (null, array())
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
Array (
+ 0 => ''
)
/codebase/grumphp/test/Console/Command/RunCommandTest.php:36
5) GrumPHPTest\Console\Command\RunCommandTest::parses_comma_separated_options with data set "empty" ('', array())
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
Array (
+ 0 => ''
)
/codebase/grumphp/test/Console/Command/RunCommandTest.php:36
6) GrumPHPTest\Console\Command\RunCommandTest::parses_comma_separated_options with data set "empty after trim" (' ', array())
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
Array (
+ 0 => ''
)
*/ | ||
public function __construct(ContextInterface $taskContext, TestSuiteInterface $testSuite = null) | ||
public function __construct(ContextInterface $taskContext, TestSuiteInterface $testSuite = null, $tasks = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make $tasks a required array?
I would prefer to always pass in a list of tasks.
(This is a BC break, but I don't think it will cause too much damage since it is an internal DTO.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this?
/**
* TaskRunnerContext constructor.
*
* @param ContextInterface $taskContext
* @param string[] $tasks
* @param TestSuiteInterface $testSuite
*/
public function __construct(ContextInterface $taskContext, array $tasks, TestSuiteInterface $testSuite = null)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
use GrumPHP\Locator\RegisteredFiles; | ||
use PHPUnit\Framework\TestCase; | ||
|
||
class RunCommandTest extends TestCase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test can be dropped if you change the input option logic as suggested above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'dont feel quite comfortable having "nothing" that actually checks the behavior - which kinda was the initial thought on adding a test in the first place.
E.g. consider the inlined
$tasks = array_map('trim', explode(',', $input->getOption('tasks', '')));
If somebody were to switch "," by ";" there would be "nothing" to catch that bug. Mutation testing tools like https://github.com/infection/infection usually "make those changes" to verify if everything is tested "correctly".
Are you open to introducing some sort of end2end/integration test that "acutally" calls the run command and verifies the output (see https://symfony.com/doc/4.1/console.html#testing-commands )?
It's something that we do quite frequently because it's a very good high level indicator for "some things don't work as they are supposed to" (though it comes with an increased maintenance overhead due to duplication).
Something along the lines of
public function test_command()
{
$application = new Application();
/**
* @var RunCommand $command
*/
$command = $application->find('run');
$commandTester = new CommandTester($command);
$commandTester->execute(array(
'command' => $command->getName(),
'--tasks' => 'foo',
));
$output = $commandTester->getDisplay();
$this->assertContains('...', $output);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @paslandau,
In a normal situation, those tests would work fine.
However, the behaviour of the grumphp commands are different based on which configuration file you supply. This means we would have a lot of e2e tests to cover all possible situations.
I was looking in to this with @Landerstraeten a while ago, but didn't find a good solution yet.
Feel free to suggest something in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll give it some thought but I'm not to deep into symfony (more of a laravel guy) plus since those sort of tests are usually tightly coupled to the application I guess I'd need more time digging some grumphp internals.
So I would keep it separate from this PR + keep the current test for now.
Hey @veewee would you like to review paslandau@6b16628 before I make a PR against seventies? |
Hi @paslandau, The branch looks OK but it's easier to review in a PR. You can change this PR or create a new one. |
Thanks! Continuing in #587. |
Note: Sorry for the ugly Reflection hack to test the protected method in
RunCommandTest
- can't use anonmous classes here due to PHP >= 5.6 constraint.