-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Allow integration tests to test commands that have confirmations #3823
Conversation
@@ -133,7 +133,7 @@ protected function buildCommandLine($command, $args, $options) | |||
} | |||
// insert drush command options | |||
foreach ($options as $key => $value) { | |||
if (!isset($value)) { | |||
if (!isset($value) || $value === true) { |
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.
Usually, 'yes' => true
gives --yes
, and 'yes' => '1'
gives --yes=1
. I've always found the Unish-style 'yes' => null
to be confusing. Would it be okay to switch to true
everywhere? We should also support null
for external unish clients.
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 feel free to change that.
…ts (pass --yes to confirm).
b6cbfc0
to
a12175a
Compare
Just one trivial question before merging. We can stick with |
@@ -102,7 +102,7 @@ public function drush($command, array $args = [], array $options = [], $expected | |||
protected function buildCommandLine($command, $args, $options) | |||
{ | |||
$global_option_list = ['simulate', 'root', 'uri', 'include', 'config', 'alias-path', 'ssh-options', 'backend', 'cd']; | |||
$options += ['root' => $this->webroot(), 'uri' => self::INTEGRATION_TEST_ENV]; // Default value. | |||
$options += ['root' => $this->webroot(), 'uri' => self::INTEGRATION_TEST_ENV, 'strict' => 0]; // Default value. |
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.
Why disable strict mode?
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.
Disabling strict mode bypasses argument validation. We can try without this.
tests/integration/WatchdogTest.php
Outdated
{ | ||
|
||
public function testWatchdog() | ||
{ | ||
$this->setUpDrupal(1, true); | ||
$this->drush('pm-enable', ['dblog']); | ||
$this->drush('pm-enable', ['dblog'], [], self::IGNORE_EXIT_CODE); |
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.
Why ignore exit code here?
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.
We might enable modules again and again. I guess that enabling an already-enabled module is not an error, so we can remove this.
…geTest to an integration test.
It worked to remove strict mode; it was also fine to remove the ignore errors flag. |
Pass --yes to confirm.