atoum / AtoumBundle

This bundle provides a simple integration of atoum into Symfony 2.
MIT License
44 stars 25 forks source link

Add support for all atoum's native arguments #28

Closed jubianchi closed 11 years ago

jubianchi commented 11 years ago

This commit adds support for quiet all atoum's native arguments. I had to disable --version and --help as they are already used by Symfony. I also disabled --loop as it produce weird errors.

To use the arguments, we need to write them using their long version :

$ app/console atoum AcmeDemoBundle --no-code-coverage --configurations=my-atoum-config.php
ludofleury commented 11 years ago

Maybe we should merge the bootstrap & co in the config runner ? it could be in conflict ?

jubianchi commented 11 years ago

@ludofleury I don't think this would conflict with anything but you are right, it needs some test to confirm that everything works as expected.

Also, the code in the command could be cleaned a bit by putting all the runner's configuration in one place.

FlorianLB commented 11 years ago

Why don't add short option notation ?

jubianchi commented 11 years ago

@FlorianLB short options are harder to handle as we have to check wether a shortcut already exists or not and I didn't find a proper way to do that.

Moreover, managing short option could be hard as we would have to maintain a map to map shortcuts (be they native atoum's shortcuts or aliased shortcuts) to correctly inject values into the atoum runner.

FlorianLB commented 11 years ago

@jubianchi I thought the $this->runner->getHelp() could return the short option name in the same way that you get the long version.

jubianchi commented 11 years ago

@FlorianLB Yes it does exactly this but before defining option in the symfony command, I have to check if the shortcut is already used by another, native symfony, option.

I could take a shortcut on this and assume that only some of them are used (which is the case) : -h for --help and -v for --version are used so I could short-circuit this but this would make this partially hardcoded.

Then, for these shortcuts, i'll have to use a map to be able to match the new shortcut (say, we replace `-hwith-e``) with the original ones.

This is not a big problem and it can be implemented quickly but what I'm asking myself is if this really makes sense ? The use of atoum's native options when being in a Symfony2 project is, IMHO, an edge case so using the long options won't really be annoying...

Perhaps I'm wrong and if so, I will do the fix with pleasure :)

FlorianLB commented 11 years ago

@jubianchi Ok, keep it simple for the moment, we will see in the future if it's a common request.

Merge ?

jubianchi commented 11 years ago

Yeah mergeable :) I think...

I'm working on a SF2+atoum bootstrap project and I'm testing everything together so if you want, I can spend some more time testing this and report/fix any failure...

Perhaps we could wait another day or two.

KuiKui commented 11 years ago

Merge ? :trollface:

ludofleury commented 11 years ago

@jubianchi By conflict I mean with my previous work with the atoumArguments. You're way is a lot cleaner, I was in a hurry for the default --bootstrap-file. I should rebase from your work.

Since now you provide every atoum argument though the sf command, I'll just have to initialize some default. Let me the week-end to do that.

jubianchi commented 11 years ago

@ludofleury no problem :)

ludofleury commented 11 years ago

We've to decide what to do about the command, is it a all-in-one proxy for bin/atoum and helper for atoum in Sf context ?

Actually we've got a tricky problem, atoum check the first CLI argument: https://github.com/atoum/atoum/blob/master/classes/script/arguments/parser.php#L145

So using

$ app/console atoum --env=test

will throw an exception if we don't pass any argument here:

<?php

$this->runner->run()

I'm not sure that the Sf command should proxy bin/atoum ? (removing the loop, dealing with cli arg/options conflict...) while anyone could use bin/atoum...

NB: with the current PR, the argument --bootstrap-file is processed twice (one pass with the @jubianchi code, one pass with my old code) and the argument test-all is always added

ludofleury commented 11 years ago

We discussed this feature with @stephpy, my opinion: the atoum command should be only a easy way to lunch tests in a Symfony context (for a bundle, for a file in a bundle, for a tag in a bundle maybe ? etc...)

We shouldn't go for a proxy for atoum, just provide the bin/atoum in the bin directory.

jubianchi commented 11 years ago

@ludofleury I agree with you that the Symfony command should be as simple as possible but, in some cases, it might be usefull to access atoum's native arguments.

I d'ont have my use cases in mind ATM, I'll check my project later and give you some feedback about it but perhaps i fell into an edge case and I should have used the atoum's native bin...