1.0 Tidy Up

[davedevelopment]

Looking for thoughts on this. The internals are a bit messy now, which leads to some strange behaviour and difficult to track down bugs, unless you're very familiar with how things work.

I've started experimenting with generating mocks using https://github.com/nikic/PHP-Parser, with a flow something like this:

• Create a new AST for new Mock class
• Load the AST for Mockery\Mock
• Use a visitor to traverse the Mockery\Mock AST copying nodes to the new Mock's AST
• Use reflection to extract method signatures from the class/interfaces we're mocking, adding the signatures to the new Mock's AST
• Manipulate the new Mock's AST as necessary for other things (instanceMocks, __call typehint) etc

I feel this is a better approach than manipulating strings, but I'm wondering if people (particularly @padraic) have the same thoughts. Here's an example, where I use PHP-Parser to remove the typehint on the __call method (one of the things the Generator currently does):

class RemoveMagicCallTypeHintVisitor extends \PHPParser_NodeVisitorAbstract
{
    public function leaveNode(PHPParser_Node $node) {
        if ($node instanceof PHPParser_Node_Stmt_ClassMethod && $node->name == "__call") {
            $params = $node->params;                                                      
            $params[1]->type = null;
        }

    }
}

$parser = new PHPParser_Parser(new PHPParser_Lexer);                      
$base = $parser->parse(file_get_contents(__DIR__ . '/library/Mockery/Mock.php'));

$traverser = new PHPParser_NodeTraverser;
$traverser->addVisitor(new MockStmtInjectorVisitor($mock));
$traverser->traverse($base);

I'm happy to pursue this further, if people think I'm heading in the right direction.

[davedevelopment]

Hmmm, it seems a nicer solution, but it's not going to work for built in classes and interfaces... back to the drawing board.

[davedevelopment]

No it will still work, as I have to reflect those and build up the AST myself, so it will work. Ignore me, just thinking out loud :)

[padraic]

I'm all for improving our plumbing. I'm going to do some work tomorrow to get us close to a 0.8.0 release but we should definitely see if this can be introduced for the next 0.9 release. Manipulating strings is, by its very nature, more of a dirty workaround than anything else. Do you have a branch set up somewhere for this? You can add one locally now that you have access ;).

[davedevelopment]

Nothing setup, just bits of code all over the place. I'll see if I can tidy it up tomorrow and push a POC up to a branch.

[padraic]

No problem - currently just reviewing issues, seeing what can be fixed, and will hopefully roll out a 0.8 point release for the 3.5 people who are still using PEAR (and a new tag for Composer) in another week.

Let's not leave 0.9 until 2014 though :P.

[davedevelopment]

So the php-parser branch is now passing all tests, including a modified version of the test from #98.

Performance is a real problem. I obviously expected a regression, but it's orders of magnitude and I wouldn't want to take this approach unless significant improvements can be made. That said, I haven't made any effort with regards to performance throughout the development, so there may be some quick wins, something like caching the AST for Mockery\Mock so we don't have to parse it everytime etc. Even so, the node visitors need to iterate over the full tree each time, so we should look to change those. For example, we could simply extend Mockery\Mock to make Mockery\InstanceMock, and change the default value of _mockery_ignoreVerification and add the constructor.

Mockery\Generator\MockConfiguration would also benefit from better use of reflection, it probably reflects the same class or interface more than once here and there. Maybe the methods should be getTargetClass and getTargetClassName, especially as getMethodsToMock returns wrapped ReflectionMethod instances.

Also, this is running against master of nikic/php-parser, but I think there'd only be one change necessary to make it compatible with the latest tag.

[padraic]

Have you had a go at profiling yet? Always a chance that the performance is getting hit by only a few factors (if it's the parser, and potentially solvable, could look into helping on that side). How big is the performance difference at present?

I'm sticking with 0.8 fixes (only 6 bugs left!) but I'll give a hand once I cut the issue list down a bit more ;).

[davedevelopment]

On the mockery test suite, barely noticeable, but that's because we're usually mocking classes with one or two methods.

The main project I work on has around 600 unit tests, that take 2-3 seconds with mocker/master, the best I've got with this branch is around 1 min :(

Once I started caching the parsing, the bulk of the time is split between the rest of the generation and pretty printing. I'm pretty sure there'll be lots of reflection I can cut down on, then I'm going to start looking at further caching. I may look into a way of hashing a mock config, in such a way that we can re-use classes within a test run. Will see how much time I get this week.

[davedevelopment]

Right, I've got that 1 minute test suite down to 10 seconds, but that's still a 5x slow down over the master branch. I'm caching the AST for the base mock and I'm also caching mock definitions (which we could also do in master if we wanted), so that already generated/defined classes are re-used.

The code for this is again a bit rought, but we can stabilise it if/when we see this as the way forward.

Will break out XHProf again soon and see what else I can squeeze out.

[davedevelopment]

Further improvements, but at some costs.

I found that traversing the AST was quite costly (as you would expect), so I extended PHPParser_NodeTraverser to allow visitors to mark themselves as finished, meaning traversal can end early.

I also removed some code that went beyond what was necessary to ensure we don't try to mock the same method twice.

Current dev-master:

Time: 1 second, Memory: 207.50Mb

OK (682 tests, 2053 assertions)
phpunit --colors  2.71s user 0.17s system 96% cpu 2.974 total

php-parser branch:

Time: 3 seconds, Memory: 63.50Mb

OK (682 tests, 2053 assertions)
phpunit --colors  4.58s user 0.14s system 99% cpu 4.751 total

Still a bit to go. At this point I'm slightly concerned about how much work I'm having to do to squeeze performance out of it, also at the cost of complicating the code.

Also, there are some improvements (like the caching) that could be ported to dev-master without the PHPParser generation, which might give it some reasonable performance improvements, leaving this branch even further behind.

[davedevelopment]

Failing tests on that branch now, can't quite get my head around how the traverser treats nodes when you want to change them, will try and take a look tomorrow night.

[davedevelopment]

Closing this for now, but I'm not totally giving up on a php-parser based generator. If I can get @padraic to give me the go ahead on #159, I'll then start pursuing a php-parser based generator to work alongside the string based one for better comparison.