Skip to content
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

Add the option '--if-not-exists' in the command 'ongr:es:index:create' #535

Closed
wants to merge 15 commits into from

Conversation

ndinh215
Copy link
Contributor

@ndinh215 ndinh215 commented Jan 5, 2016

I fixed the issue #423 . Please review again. Thanks.

->addOption('no-mapping', 'nm', InputOption::VALUE_NONE, 'Do not include mapping')
->addOption(
'if-not-exists',
'ine',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that Symfony does not support multi-letter input option shortcuts. I see that we have the same with other options

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http://www.gnu.org/software/libc/manual/html_node/Argument-Syntax.html
With this standard, it could understand wrong the options.

$input->getOption('manager')
)
);
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure 100% but probably better would be change to: return 0;.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct. As Doctrine's code about this processing, it will be

return $error ? 1 : 0;

whatever success or error.

Hence, here, we agree that 0 will be for fail and 1 for success. Is it OK? @saimaz

*
* @dataProvider getTestExecuteData
*/
public function testExecuteWithIndexExistence($managerName, $arguments, $options)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not use different test case data provider and try to behave inside test case to match expectations. To test --if-not-exists please write separate test case, the data provider is not necessary.

…'if-not-exists' in 'ongr:es:index:create' command
@ndinh215
Copy link
Contributor Author

ndinh215 commented Jan 7, 2016

Thanks for your points, @saimaz . With this patch, I removed try catch and added asserts for testing result which is returned from the execution and the output string. And also, I refactored the code a bit for the purpose of retrieving command tester (for testing output in terminal) via the method:

protected function getCommandTester($commandName)
    {
        $app = new Application();
        $app->add($this->getCreateCommand());

        $command = $app->find($commandName);
        $commandTester = new CommandTester($command);

        return $commandTester;
    }

About the data provider, I think this test case still needs it because at present the test case does not try to catch the Elasticsearch exception of 'Index already exists', it tries to assert the returns from command execution.


// Try to create index in prior
if (!$manager->indexExists()) {
$manager->createIndex();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Index is automatically created when you call $this->getManager().

@ndinh215
Copy link
Contributor Author

ndinh215 commented Jan 8, 2016

Closes #423

@ndinh215
Copy link
Contributor Author

ndinh215 commented Jan 8, 2016

I will create new PR with better form for this issue.

@ndinh215 ndinh215 closed this Jan 8, 2016
@saimaz saimaz removed the qa label Jan 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants