Command factory tests

[SenseException]

I was reviewing the project code to check which classes can be tested easily (#44), but I'm afraid there will be bigger refactoring steps in the future before many classes of the project can be tested.

For now, I contribute small tests of the command factory, creating build in and custom commands. For custom commands, I created some dummy commands to be able to check exceptions and returned command instances.

I guess next time will be a PR with refactored code.

[eps90]

Not exactly. At first, we have no DI mechanism in Magallanes, and as @andres-montanez said in one of issue, we shouldn't use external dependencies as long as Magallanes lives also as a Pear package.
But injecting things in many DI mechanisms can be done by constructor or via proper methods. Sometimes it's even necessary to inject them like that.

[SenseException]

Can you please link to that issue? I don't understand why you're referring to pear. Do you mean Config when you write about external dependencies?

[eps90]

No, I meant some DI mechanism, like Sf2 Dependency Injection, or any other, loaded by composer dependency that we could use. We can, of course, write our own DI.
Issue I mentioned about is: #88
It's only short sentence but, I don't know why, I keep that in mind.

[SenseException]

I don't want to introduce ServiceLocator or other external libs, but the constructor is a common place to inject dependencies that are mandatory. Right now, the configuration is injected with a setter even when the Command doesn't need it. It means there is a setter which isn't needed in every Command class nor does every Command has a dependency to extend AbstractCommand.

The suggested todos are future talk, because injecting only the needed dependencies would result into changing the factory.
If there are already some rules about what this project doesn't want to introduce, a text-information like a contribution.md would be nice so that devs creating a PR can follow that guideline. Since we already introduced unit tests together, one contribution rule could be, that a PR must contain unit tests for the new code.

BTW: I'm glad the todo comments were noticed by someone.

[eps90]

Of course, that's what I was talking about. I've wanted only note that not everything in injection mechanisms should be done by constructor. In this part it can be done,even it should be done, because in the other way, get method won't work properly.
And I'm glad too that someone makes such sugestions.

[GodsBoss]

@eps90 Does the comment of @andres-montanez in #88 really mean that no external PHP packages should be used? That issue is about a feature which can only be implemented using a non-standard PHP extension like ncurses.

Also, does Magallanes really live as a PEAR package? It's neither mentioned in the documention nor can I see a PEAR package file (package.xml).

[eps90]

@GodsBoss By PHP packages @andres-montanez meant composer dependencies and I don't see any reason why should we need ncurses for progress bar (if you're talking about #88).
Of course I mistyped my comment and I meant phar package :)

[eps90]

The one thing bother me for a long time - why did you create the classes physically? Can't you just mock them?

[SenseException]

What do you suggest how we should handle this for the current and future tests of object creating factories in Magallanes?

[eps90]

Well there is a way to do this, and I almost have the solution but one PHPUnit bug stopped me... Here are some details:

• PHPUnit allows to create mocks of non-existing object.
• All we'd need to do is either create mock of some class (non-extending the AbstractCommand) to check if there is an exception
• ... and create mock of AbstractCommand and call setMockClassName on MockBuilder to simulate class that extends AbstractCommand with specified name

Where's the bug? setMockClassName doesn't handle class names with namespaces, and for me it's critical. There's an issue in PHPUnit-Mock-Objects repo (no. 134) and it's very old (since mid of 2013). I've asked there are there any news about it. So we'll see.
I see two possible solutions:

• I think (but I'm not sure) that PR Make Task Factory load tasks from external namespaces #189 can fix this. Then all tasks will be treated as they were in global namespace. So setting mockClassName to some class without namespace should do the job
• Second thing: (I've not tested that neither) - use Mockery. It has own mechanism for mocking classes (and as far I know, there is also possibility to mock final classes). But I think it's kind of inconsistency to mock classes with two different engines (Mockery has own expectation system).

[SenseException]

I know Mockery and tested it before. It offeres lot of functionalities to mock static methods and instances created inside a methods, that are not injected. When I created the merged PR for introducing unit tests to Magallanes, I decided against Mockery. Unit tests can be a good way to show bad testable code while showing what can be improved. Who needs DI when a class can be mocked inside another method. Let's make every method static because we can easily use Mockery.

I agree that finding a way for replacing Dummy classes would be nice, but I guess there are open questions.

What are we trying to accomplish with unit tests on current code and what are the plans for the current code? Do we bend our tests to the current code first? Is new introduced code and its tests build the same way like the current?

[eps90]

I'm not saying we need to use Mockery to test untestable code. I meant only we can use it because it doesn't contain the bug the PHPUnit has.
Current code has a lot of anti-patterns, like Singletons, static methods, methods in a class responsible for completely another thing. But we need to test the production code as it is, eventually with small changes that do not affect current functionallity but make testing nicer. DI mechanisms would help us with it a lot. Then we could get rid of static methods like in e.g. Config class. Well it's quite wrong example because class itself should know about it's configuration but without calling Config class (well Factory could do this). In the other hand, we can't use third-party solutions here so we'd need to create our own DI container which is pretty simple though.
The reason why I suggested creating mocks is to prevent creating those classes physically. Look, there are three PRs including tests for Task\Factory and Command\Factory. Each of them brings 2-3 classes only to test if class exists. Can you imagine what's gonna be when there would be 10 PRs? 30? So I belive there is a way to avoid it and not to create a class physically only to make one test method pass.

[eps90]

Meanwhile I've found temporary solution to workaround the bug.

• This code will fail due to PHPUnit bug, because you pass namespaced class to setMockClassName

$this->getMockBuilder('MyAbstractClass')
    ->setMockClassName('MyNamespace\MyClass')
    ->getMock();

• And this will pass:

$mock = $this->getMockBuilder('MyAbstractClass')
    ->setMockClassName('MyClass')
    ->getMock();
class_alias('MyClass', 'MyNamespace\MyClass');

$this->assertInstanceOf('MyClass', $mock); // true
$this->assertInstanceOf('MyNamespace\MyClass', $mock); // true
$this->assertInstanceOf('MyAbstractClass', $mock); // also true

[eps90]

And here's how your tests gonna look without any additional classes:

    public function testGetCustomCommand()
    {
        $this->getMockBuilder('Mage\Command\AbstractCommand')
            ->setMockClassName('MyCommand')
            ->getMock();

        class_alias('MyCommand', 'Command\MyCommand');

        $command = Factory::get('my-command', $this->config);
        $this->assertInstanceOf('Command\\MyCommand', $command);
    }

    /**
     * @expectedException \Exception
     * @expectedExceptionMessage The command MyInconsistentCommand must be an instance of Mage\Command\AbstractCommand.
     */
    public function testGetInconsistencyException()
    {
        $this->getMockBuilder('Command\MyInconsistentCommand')
            ->getMock();

        Factory::get('my-inconsistent-command', $this->config);
    }

How do you like it?

[SenseException]

I did class_alias as workaround before too, since I had to test similar code too. It was about creating instances inside a method. But it's still a workaround. Because of your argument about the possible rising amount of Dummy classes in the future, I'm with you about finding a way to remove the Dummies/substitudes.

I'm working with PHPUnit 4.5 currently and its support of Prophecy I dig into that and look if it can help to find a solution. We're at the start of adding unit tests to Magallanes and it would be easy to rise the version in composer.json if needed. Maybe we don't have to handle that bug.

[eps90]

Good point. I totally forgot that in PHPUnit 4.5 mocks can be used with Prophecy. I'll test that and will let you know ASAP.
Back to my previous solution, yes it is workaround but only for PhpUnit bug. Setting class name is ok for me since the class does not exist and we want to check code behavior for some known conditions. That's all we need. We're not dependent on anything. And that's the idea of unit testing. Preparing the whole environment outside the class it's rather integration testing which is unwanted since we want to refactor the code in the future.

[eps90]

Well saddly, prophecy behaves differently than PHPUnit mocks. When you create a mock with PHPUnit MockBuilder, PHP interpreter know that the class exists. So calling it will make the class accessible globally.
In Prophecy, you receive some proxy object and you can stub methods or make promices. It's great tool when you have DI or loosely decoupled classes. I think it won't pass here.

[SenseException]

When life gives you lemons, then $this->assertLemonade($juice)
I chose the alias variant for the tests.

[eps90]

Thanks for comment indicating that this is temporary solution. I'll test that tomorrow when I'll have access to my laptop and I'll give you proper feedback :-)

[eps90]

use @link instead of @see.
@see is for documentation links, like other methods, classes, etc.
@link is purely for hyperlinks

[eps90]

Please add PHPUnit's coverage annotations, because I can see that you unintentionally cover Mage\Command\AbstractCommand.

[SenseException]

Annotations are now added/fixed.

[eps90]

Everyhitngs seems great now, thanks for merging my PR and for your support, @SenseException. Merging! :)