-
Notifications
You must be signed in to change notification settings - Fork 440
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,21 @@ public function filterByTestSuite(TestSuiteInterface $testSuite = null) | |
}); | ||
} | ||
|
||
/** | ||
* @param string[] $tasks | ||
* | ||
* @return TasksCollection | ||
*/ | ||
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 commentThe 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 true; | ||
} | ||
return in_array($task->getName(), $tasks); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can set the third argument to true here. |
||
}); | ||
} | ||
|
||
/** | ||
* This method sorts the tasks by highest priority first. | ||
* | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -53,6 +53,13 @@ protected function configure() | |||||
'Specify which testsuite you want to run.', | ||||||
null | ||||||
); | ||||||
$this->addOption( | ||||||
'tasks', | ||||||
null, | ||||||
InputOption::VALUE_REQUIRED, | ||||||
'Specify which tasks you want to run (comma separated). Example --tasks=task1,task2', | ||||||
null | ||||||
); | ||||||
} | ||||||
|
||||||
/** | ||||||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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 commentThe reason will be displayed to describe this comment to others. Learn more. This will retain empty input values, i.e. if I'm fine with removing deduplication, though. Thoughts? fyi - failing tests after change:
|
||||||
|
||||||
$context = new TaskRunnerContext( | ||||||
new RunContext($files), | ||||||
(bool) $input->getOption('testsuite') ? $testSuites->getRequired($input->getOption('testsuite')) : null | ||||||
(bool) $input->getOption('testsuite') ? $testSuites->getRequired($input->getOption('testsuite')) : null, | ||||||
$tasks | ||||||
); | ||||||
|
||||||
return $this->taskRunner()->run($output, $context); | ||||||
|
@@ -97,4 +107,25 @@ protected function paths() | |||||
{ | ||||||
return $this->getHelper(PathsHelper::HELPER_NAME); | ||||||
} | ||||||
|
||||||
/** | ||||||
* Split $value on ",", trim the individual parts and | ||||||
* de-deduplicate the remaining values | ||||||
* | ||||||
* @param string $value | ||||||
* @return string[] | ||||||
*/ | ||||||
protected function parseCommaSeparatedOption($value) | ||||||
{ | ||||||
$stringValues = explode(",", $value); | ||||||
$parsedValues = []; | ||||||
foreach ($stringValues as $k => $v) { | ||||||
$v = trim($v); | ||||||
if (empty($v)) { | ||||||
continue; | ||||||
} | ||||||
$parsedValues[$v] = $v; | ||||||
} | ||||||
return $parsedValues; | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,16 +22,26 @@ class TaskRunnerContext | |
*/ | ||
private $testSuite = null; | ||
|
||
/** | ||
* @var string[] | ||
*/ | ||
private $tasks = null; | ||
|
||
/** | ||
* TaskRunnerContext constructor. | ||
* | ||
* @param ContextInterface $taskContext | ||
* @param ContextInterface $taskContext | ||
* @param TestSuiteInterface $testSuite | ||
* @param string[]|null $tasks | ||
*/ | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Can you make $tasks a required array? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. yes |
||
{ | ||
$this->taskContext = $taskContext; | ||
$this->testSuite = $testSuite; | ||
if ($tasks === null) { | ||
$tasks = []; | ||
} | ||
$this->tasks = $tasks; | ||
} | ||
|
||
/** | ||
|
@@ -81,4 +91,20 @@ public function setTestSuite(TestSuiteInterface $testSuite) | |
{ | ||
$this->testSuite = $testSuite; | ||
} | ||
|
||
/** | ||
* @return string[] | ||
*/ | ||
public function getTasks() | ||
{ | ||
return $this->tasks; | ||
} | ||
|
||
/** | ||
* @return bool | ||
*/ | ||
public function hasTasks() | ||
{ | ||
return !empty($this->tasks); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
<?php | ||
|
||
namespace GrumPHPTest\Console\Command; | ||
|
||
use GrumPHP\Configuration\GrumPHP; | ||
use GrumPHP\Console\Command\RunCommand; | ||
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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Hi @paslandau, In a normal situation, those tests would work fine. Feel free to suggest something in this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
{ | ||
/** | ||
* @test | ||
* @param null $valueString | ||
* @param null $expected | ||
* @dataProvider parses_comma_separated_options_dataProvider | ||
* @throws \ReflectionException | ||
*/ | ||
function parses_comma_separated_options($valueString = null, $expected = null) | ||
{ | ||
/** | ||
* @var GrumPHP $grumPhp | ||
*/ | ||
$grumPhp = $this->createMock(GrumPHP::class); | ||
/** | ||
* @var RegisteredFiles $registeredFiles | ||
*/ | ||
$registeredFiles = $this->createMock(RegisteredFiles::class); | ||
|
||
$command = new RunCommand($grumPhp, $registeredFiles); | ||
$method = new \ReflectionMethod($command, "parseCommaSeparatedOption"); | ||
$method->setAccessible(true); | ||
|
||
$actual = $method->invoke($command, $valueString); | ||
|
||
$this->assertEquals($expected, $actual); | ||
} | ||
|
||
|
||
public function parses_comma_separated_options_dataProvider() | ||
{ | ||
return [ | ||
"default" => [ | ||
"valueString" => "foo,bar", | ||
"expected" => [ | ||
"foo" => "foo", | ||
"bar" => "bar" | ||
], | ||
], | ||
"trims values" => [ | ||
"valueString" => "foo , bar", | ||
"expected" => [ | ||
"foo" => "foo", | ||
"bar" => "bar" | ||
], | ||
], | ||
"deduplicates values" => [ | ||
"valueString" => "foo,bar,bar", | ||
"expected" => [ | ||
"foo" => "foo", | ||
"bar" => "bar" | ||
], | ||
], | ||
"null" => [ | ||
"valueString" => null, | ||
"expected" => [], | ||
], | ||
"empty" => [ | ||
"valueString" => "", | ||
"expected" => [], | ||
], | ||
"empty after trim" => [ | ||
"valueString" => " ", | ||
"expected" => [], | ||
], | ||
]; | ||
} | ||
} |
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
?