Added --testsuite argument, allowing to filter files/directory by parent testsuite name attribute

[jpic]

Before, we could run the test suite of a component as such: UnitTest/src/runtests.php Database

But the new test runner does not allow this. For example, consider this phpunit.xml.dist: https://fisheye6.atlassian.com/browse/zetacomponents/trunk/phpunit.xml.dist?hb=true

Phpunit --filter Database will not behave as expected in two ways:

0. test cases without Database in the class name will be omited (ie. ezcQueryExpressionTest from Database/tests/sqlabstraction/expression_test.php)
1. test cases with Database in the class name from other components will be included (ezcDatabaseSchemaGenericTest from DatabaseSchema/tests/generic_test.php)

As a more alarming example: --filter MvcTools runs 124 tests, wheras --testsuite MvcTools runs 168.

Credit to Derick Rethans who participated in this little task. Long live the Components !

(this is a follow up to: #494)

[edorian]

I've tested it and it seems fine to me.

You shouldn't need to recurse the check as testsuites can't be in testsuites. If anything that check should be only done once outside of the foreaches.

if $testSuiteNode->getAttribute('name') does not match $testSuiteFilter return $suite; // the empty suite

Also I'd name the argument $testSuiteFilter and not $testSuiteName as its there for filtering.

Another point is that the --help entry is missing and maybe it would be nicer to say "contains" instead of "equals" so that you could say "all testsuites named intergation-* by passing --testsuite integration.

---

For the test hit me up on irc if you need any help

[jpic]

You shouldn't need to recurse the check as testsuites can't be in testsuites

"testsuites" tags might not be nestable, but "testsuite" tags are... In the phpunit.xml.dist that Sebastian commited (https://fisheye6.atlassian.com/changelog/zetacomponents?cs=1181653#trunkZ002fphpunit.xml.dist), there is such a structure:

<phpunit>
    <testsuites>
        <testsuite name="Zeta Components">
            <testsuite name="Archive">
                <directory [...]</directory>
            </testsuite>
            <testsuite name="Authentication">
[...]

You should see the nested "testsuite" tags here. Although one level nesting is relevant in this case (according to Sebastian's commit), I don't know if more levels of nesting would be.

Also:

if $testSuiteNode->getAttribute('name') does not match $testSuiteFilter return $suite; // the empty suite

That would work if that method was called with every testsuite tag as argument. Which is not the case.
Of course this is not obvious, I don't know if this is a bug or design decision. I did spend quite some time before i realized that the nested testsuite tags were totally omited. For this very reason, there is probably no elegant way of filtering out by nested testsuite tag.

The reason of that is in here: https://github.com/jpic/phpunit/blob/47c5f6d127fe6cffc7d82274ba425e745cb7b4bd/PHPUnit/Util/Configuration.php#L763
The xpath, "'testsuites/testsuite'", clearly omits nested testsuite tags. So your idea won't work because $testSuiteNode is <testsuite name="Zeta Components"> (or any testsuite tag that has a testsuites tag as parent). The method getTestSuite is not called with the nested testsuite elements as arguments ever.

In this loop https://github.com/jpic/phpunit/blob/47c5f6d127fe6cffc7d82274ba425e745cb7b4bd/PHPUnit/Util/Configuration.php#L814 :

foreach ($testSuiteNode->getElementsByTagName('directory') as $directoryNode)

$testSuiteNode is <testsuite name="Zeta Components">, and $directoryNode are the<directory> elements of all children, from those in<testsuite name="Archive"> to those in <testsuite name="WorkflowSignalSlotTiein">. So the only way to filter them out seems to be in the for loop.

Anyway, it's of course improvable as you mentionned a wildcard for example. But you know, YAGNI, I personnaly wouldn't decide to implement it until I actually need it - I would be more likely to do it wrong, trying to implement something I don't need. But if you make it a requirement to accept the pull request then I can implement wildcard support in the name (but why not accept a regexp rather than a wildcard for example ?). Wouldn't that be consistent with --filter ? Then maybe shouldn't we consider another --testsuite-filter (or even --testsuite-name-filter) option ?

[jpic]

Please tell me how to help you integrate this feature before it conflicts with updates :)

[sebastianbergmann]

I'll try to find the time this weekend to look into this.

[edorian]

+1 from me. I'd like to have a test or two for it (.phpt tests) but I can write those too once this is pulled.

Especially having a test for nested name testsuites would be needed as I'm quite sure this is going to break at some point in the future if we don't have one for it :)

So if you already have a sample for this @jpic please include it :=)

[grahamc]

+1 on this.

[soid]

Guys,
Why does it hang here for 3 month? If it doesn't work for all cases and nobody volunteering to fix it right for nested testsuites it means nobody needs it for nested testsuites. I don't have to tell you all these is new stuff and it shouldn't break anything.
If it's so hard to pull into framework such simple features, who will volunteer to work on more complex extensions?

[edorian]

@soid It's on the list of things i want/need to get done for the 3.7 release and getting that feature in with regression testing, documentation (which someone has to write too) and possible clean ups will take at least 4 hours.

Time i current don't have for stuff that is not a urgent bug fix but a feature for a future version that I'd much rather test with all the other stuff.

My current goal is to get 3.7 out of the door as soon as possible but between having a day job and $personalReasons I currently don't have the time.. you taking a piss / venting some sort of frustration in a rather random place doesn't really help ether

[soid]

It's not frustration, I'm just trying to understand why it takes so hard.
This is not a feature request that requires you to implement it. Its implemented feature that should be reviewed by people who know the code (13 minutes), merged (1 minute) and changelog updated (1 minute). That's it. If it doesn't pass the code review it takes even less. And of course if it's merged it should come to unstable branch anyway.
I fully understand all your reasons and I appreciate the time you spent on the project. But you shouldn't block it if it's not perfect enough. That way the project will involve more volunteers and finally you will have to spend less time on it.

PS. I'll try to contribute here and we'll write some tests for the feature. I hope it won't hang for months :-D
PPS. Okay, I cheated a bit in numbers but I think the idea is clear.

[soid]

I had a chance to look at the code from this request and I can say this is piece of shit.

1. It doesn't follow existing standards while introducing the new arguments (e.g. PHPUnit_TextUI_TestRunner isn't changed while it plays important role in arguments processing);
2. It works only for testsuites set in xml config and conceptually broken as it sorts out testsuites while parsing the xml. I would rather make it work like --filter argument;

I'd like to volunteer here and implement it right way. I wouldn't introduce a new argument and would just use existing --filter. I'd store parent testsuite for all tests and suites, and then I'd generate somewhere in PHPUnit_Util_Test::describe() strings like "ParentTestSuite::ChildSuite::TestCase". So it would be possible to use it like --filter=ParentTestSuite::ChildSuite for sorting out suites.

How do you think would it work? Or it's better to leave --testsuite option?

[jpic]

Thanks for the objective review. I don't know what this code did to you but you seem pretty mental about it, so please accept my sincere apologies about that. Please understand that I was just trying to help, not to alienate random people.

We don't need this feature anymore so I'm closing the request.

[soid]

Sorry for being straightforward. It's just my opinion, I don't really know much about the phpunit code and I can be wrong.
Anyway thanks for your contributions.

[edorian]

@soid

I had a chance to look at the code from this request and I can say this is piece of shit.

This is not acceptable behavior. Please refrain from talking like that to people sending contribution to a project you are not involved in.

This is not a feature request that requires you to implement it. Its implemented feature that should be reviewed by people who know the code (13 minutes), merged (1 minute) and changelog updated (1 minute). That's it. If it doesn't pass the code review it takes even less. And of course if it's merged it should come to unstable branch anyway.

And cleaned up, and tested, and documented and then someone needs to apologize to the contributor that got insulted but someone hickjacking this pull request.

PS. I'll try to contribute here and we'll write some tests for the feature. I hope it won't hang for months :-D

To be honest: You won't get any special (at least not positive) threatmeant for insulting people and shouting around about project managment in some random pull request you have stumbled upon without even talking to the project maintainer or asking whats up on IRC.

---

@jpic First off: I'm sorry that you got disturbed my this dicussion as it has nothing to do the with pull request:

Like stated: This can easily go into 3.7 and I can see the need for the feature (at least it would make testsuites a lot more useable again).

So if you don't mind I'd gladly merge it into master once I get around to.

P.S so zeta switched their test layout? I'd merge it anyways if thats fine

[sebastianbergmann]

Thank you, @edorian for making clear that this kind of language is not appropriate. I have to apologize that I did not step up earlier and make this clear. I, too, still want this feature but of course the implementation has to be clean. I trust your judgement of whether or not to merge this pull request.

[jpic]

The components move from having a single repo with a single phpunit.xml.dist to being each in its own repo with each it's own phpunit.xml.dist. I love this decision. This means that the components project might become an open coding standard rather than a commercially supported set of libraries.

We all have our bad days so I'm cool. Probably we all humiliated ourselves on a forum once in our lives, no big deal B)

[soid]

My apologies for being rude. I'm still learning English and definitely didn't get what I said :-) Just wanted to be straightforward. I'm glad the request has moved on and @edorian decided to merge the request. It's what I was talking about. But I think some code-tuning should be done here and I'd like to volunteer. I'll try to catch you in IRC.

[soid]

Allright, here's my approach. I believe the right way to filter out the suites is to place the logic in PHPUnit_Framework_TestSuite::run() where phpunit runs all tests. It's akin to existing --filter option but extends it and filters testsuites too.
Let me know if you have concerns about something I'll easily fix it.

I have one concern: $parent member isn't introduced in PHPUnit_Framework_Test and set dynamically in PHPUnit_Framework_TestSuite::addTest(). As PHP interfaces can't specify members it would require to introduce something like getParent() and setParent() and its implementation for all classes implemented PHPUnit_Framework_Test so the change would broke many code implemented PHPUnit_Framework_Test (outside phpunit). What do you link about it?

[edorian]

I don't like the approach taken in your pull request. It puts additional complexity in the --filter parameter and since we only have one it would not be possible to say "All tests with 'mysql' from testsuite 'db' " which would be one of the usecases I see.

That could maybe solved with allowing multiple filter arguments but it would (I'm guessing) lead to a lot more false positives.

Apart from that it's a BC issue that might not hit a view people but it's something to take into account.

For me @jpic seems a lot cleaner, better documented, less error prone and more fitting for the use cases i see.

[soid]

I don't have preferences about either to use the new --testsuite option or including the logic in the --filter. However my main concern regarding the first commits that it's working only when testsuites configuration set in XML file.

it would not be possible to say "All tests with 'mysql' from testsuite 'db' " which would be one of the usecases I see.
--filter is regex so it's possible but not beautiful (something like --filter=db.+mysql).

So what do you think if I would introduce --testsuite option again but it would match testsuites like --filter option does but only in testsuite names?

I would also leave the ability to filter hierarchical suites but it's not what I really need as I need the ability to filter testsuites. It's just nice to have feature so if you don't like that parent-child relations I introduced for suites then I can remove it.

Also I'm not sure whether the new testsuite option should match regex as the filter option does. I'd like to do it as regex because: 1. it's more flexible, 2. it's how --filter works so they would work equally.

What do you think about it?

[edorian]

Good points about the xml config. The reason i didn't care is that the feature is deprecated anyways (even so we might make that clearer) but it was removed from the docs quite some time ago. Because of that i was fine with it only working for the xml config :)

having --testsuites work like --filter would be great. I agree on that.

[edorian]

@jpic Thanks a lot for the patch and again: sorry for how the discussion went.

[grahamc]

Excellent! When are you guys expecting to release a new version? I need this pretty bad.

[edorian]

@grahamc The milestone for the next release is here: https://github.com/sebastianbergmann/phpunit/issues?milestone=8&page=1&state=open

I'll work on all outstanding issues but I, sadly, can't make any promises.

You could run this from the git checkouts or just patch your locally installed version. Apart from that there will be a 3.7-beta-phar somewhere down the line.

[grahamc]

Thank you!

Graham Christensen

On Wednesday, May 9, 2012 at 4:23 PM, Volker Dusch wrote:

@grahamc The milestone for the next release is here: https://github.com/sebastianbergmann/phpunit/issues?milestone=8&page=1&state=open

I'll work on all outstanding issues but I, sadly, can't make any promises.

You could run this from the git checkouts or just patch your locally installed version. Apart from that there will be a 3.7-beta-phar some down the line.

---

Reply to this email directly or view it on GitHub:
#495 (comment)