Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add support for multiple Include/Exclude categories for a Suite #142

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants

Not all Categories can be described naturally(or unnaturally) in a class hierarchy. Categories runner should support filtering on multiple Include and Exclude categories for the same suite.

@includecategory({CatA.class, CatB.class})
Will run the methods that contain ALL of the specified categories.

@excludecategory({CatC.class, CatD.class})
Will not run the methods that contain ANY of the specified categories.

Add support for multiple Include/Exclude categories
@IncludeCategory({CatA.class, CatB.class})
Will run the methods that contain ALL of the specified categories.

@ExcludeCategory({CatC.class, CatD.class})
Will not run the methods that contain ANY of the specified categories.
Owner

dsaff commented Feb 6, 2012

Would pull request #340 be sufficient to this?

The #142 impl of multi Include is slightly different then the one in #340. As far as I can tell, #340 accepts ANY match on Include, while #142 requires ALL to match.

@Category(EJB.class, Security.class) 
@Test public void A...()

@Category(JMS.class, Security.class) 
@Test public void B...()

In #340, a suite with @includecategory(JMS.class, Security.class) would run both A and B. (category security is on both)
in #142, a suite with @includecategory(JMS.class, Security.class) would only run B.

Both are valid use cases..

We currently have the #142 behavior++ supported via Maven Surefire, https://jira.codehaus.org/browse/SUREFIRE-809. Personally I'm more likely to configure include/excludes as part of the build system and not in the Test suite it self.

Contributor

Tibor17 commented Feb 6, 2012

Hi All,

We have all very similar ideas around Categories.

I have same feature in #336 which must go together with #354 -they share the implementation.

The #142 uses arrays of classes, but the implementation in #354 uses Set.

I have to agree with Salak that the categories should be configurable out of JUnit, see #354 -and that's the reason why i provided this solution.
The implementation in #354 provides configurable categories, which means that you can configure the categories via system properties.

Regarding Salak's idea of ALL over ANY, we discussed such issue last year with David and he opened #338.
For me it is just another aspect of view, and no problem to provide descriptive method in @includecategory for operation ANY/ALL.

Tibor

Owner

dsaff commented Mar 14, 2012

@aslakknutsen, @tibor17, can you work with the author of #340 and come up with a unified proposal? If there's no common ground, I can serve as a tie-breaker, but since this is not a feature I actively use at the moment, I'd love to see a spec that starts life with broad community support. Thanks.

Contributor

Tibor17 commented Mar 15, 2012

yes, David.
IHMO we all three should participate in this thread. I am glad that we continue again.

What is missing and what we only have to find out is the way to declare ANY/ALL.

Currently we have this kind of declaration...
@includecategory({CatA.class, CatB.class})
@excludecategory({CatC.class, CatD.class})

And what about to introduce plural additionally IncludeCategories, ExcludeCategories?

It is a conflict to use IncludeCategory and IncludeCategories both on same class.

This means singular is ALL (intersection), and plural means ANY (of these assigned from).

What's your opinion guys?

Additionally i want to say that i would prefer Set of classes instead of arrays, because the duplicates have no sense.
That's the reason why i did this way.

ajmath commented Mar 15, 2012

Hey folks,

I'm in complete agreement that both ANY and ALL category matching are valid use cases. Though, I don't think that having Categories and Category for ALL and ANY is really intuitive.

What if the singular version was kept as is (only one category), and we add @includeallcategories, and @includeanycategory for each respective match method (likewise for exclude).

Sets make sense as well as I don't see any reason for duplicates.

Contributor

stefanbirkner commented Mar 16, 2012

I prefer Andrew's proposal because it makes it easier to distinguish between the different variants.

IncludeCategory and IncludeCategories are very similar. Therefore it is easy to mix them up.

Contributor

Tibor17 commented Mar 16, 2012

@Stefan i am glad that you participate

Even that i came with these two similar annotations in first thought, i don't like such concepts because we have two annotations and common risk that someone uses both -then Categories runner has to fail.

Therefore would I prefer the most simple solution as follows...

Keep the Included/ExcludedCategory with descriptive method
Class<?>[] value, and add an extra descriptive method:

Selection assignableTo() default ALL

, where

public static enum Selection {
ANY, ALL
}

finally the example would be like this:
@Categories.IncludeCategory(value = {A.class, B.class} assignableTo = ANY)

Similar with excluded ones.

The categories in suit are parents, categories in test class or method altogether are children.
So here we try to assign any of or all of parents from some child when filtering test method.

Tibor17 added a commit to Tibor17/junit that referenced this pull request Mar 17, 2012

Contributor

Tibor17 commented Mar 17, 2012

Hello guys,

As a change in my repo i added a descriptive method
public Selection assignableTo()
to the annotations

Do you agree that this solves our problem with good usability in tests?
Would you vote for this approach or should we change something?

I took Aslak's test which use ALL, and mine which use ANY. Both tests passed. I will next days verify also Andrew's test if work for us.

Pls see also the top of Javadoc in class Categories which i extending by behavior on parent and children in this sense.
This is necessary to satisfy #338 as well.

And hopefully we may conclude this long standing issue if you accept, and continue with other ones.
The question of back-integrations would open...

Contributor

Tibor17 commented Mar 20, 2012

ok, i updated my repo by using Andrew's test.
The descriptive method "assignableTo()" in annotations works in these tests and for me it is a candidate.
What you think @stefanbirkner @matheeeny @aslakknutsen ?
May we continue in this discussion?

Keeping it in the same annotation makes sense to me.

+1

ajmath commented Mar 22, 2012

+1

Thanks @tibor17 for taking the lead on this. It will be good to get back to using the upstream JUnit build in our codebase once this feature is brought in.

Contributor

Tibor17 commented Mar 22, 2012

Well the reason that i made such a little pressure is that I am in hurry because i want this feature to get into our internal test development and use it with categories

  • and it is now already late because we wanted to reuse this feature from junit core sometime in January.

So I have an internal patch built on the top of junit lib, but patches are not always the right solution :(

ajmath commented Mar 23, 2012

Hey @dsaff, I believe we've come to a consensus. Can you move forward with this?

Contributor

Tibor17 commented Mar 29, 2012

@stefanbirkner @matheeeny @aslakknutsen
yes we have come to consensus on using the declarative method assignableTo() in annotation.
But normally in my development the code is not enough, and therefore i additionally extend Javadoc to make sure that all users of the class understand the purpose and usage, limitations, etc.

Would you pls check out the Javadoc on Categories in my repo, because you did not update it in your repo?
There might be typos, and license is missing which i am aware of as well.

The implementation should follow the expected behavior and description in Javadoc -but not opposite.

Contributor

Tibor17 commented Apr 3, 2012

@stefanbirkner Hey Stefan would you pls join us again?
As previous few comments, we made a progress in an agreement of keeping assignableTo() in same annotation.

To all of us:
IMHO we should take steps from the simplest to complex.
As a proposal, we may have a review of

thx, Tibor

Contributor

stefanbirkner commented Apr 7, 2012

I'm a little bit busy, but I review the things next week.

langles commented Apr 19, 2012

Can you comment on what the behavior should be when a test class has a class-level @category annotation that matches the @includecategory specified for a test suite AND the test class has a method-level @category annotation that matches the @excludecategory for that same test suite?

Contributor

Tibor17 commented Apr 19, 2012

In such case the method is launched, because it inherited class category which is declared as included by the suite, even if method's category is signed as excluded.

In other words test method populates own categories + test class category.
The algorithm first of all excluded categories from those populated. If a category remains assignable to included, the method is launched.

Isn't it obvious from the javadoc? Should i improve it?

The annotation has additional alternative "assignableTo".

langles commented Apr 19, 2012

Thanks for that. Yes, I think the javadoc would benefit from a clarification on this point.

Contributor

stefanbirkner commented Apr 20, 2012

@tibor17 I had a look at your changes, but can't add comments. I think you need to create a pull request in order to let me add comments.

Contributor

Tibor17 commented Apr 24, 2012

@dsaff
David, how can i enable Stefan adding comments to #354 ?

Owner

dsaff commented Apr 24, 2012

I actually don't know. @stefanbirkner, can you go to https://github.com/KentBeck/junit/pull/354/files, hover over a line of code, and take a screenshot of what you see there? I thought that anyone could comment on any pull request. Sorry for the confusion.

Contributor

Tibor17 commented Apr 24, 2012

@stefanbirkner pls try to modify and take screenshot as David has mentioned.
If not succeeded , then i will ask the admin what might be wrong with that.

Contributor

Tibor17 commented May 9, 2012

@langles
Scott, thx for your remark, i fixes some issues.

I updated a few lines in Categories.java
Tibor17/junit@256e4c6

I would like you to review this comment according your remark.
(class and method-level is merged when evaluating a test method -thus the level does not matter)

There is one important notice.
A changed behavior in CategoryTest#ifNoTestsToRunUseErrorRunner().
Test case category not matching a category defined in suite behaved same as if no tests specified at all in the source code of test case.
This means Categories threw InitializationError. This was uncomfortable issue.

Nowadays the InitializationError is thrown if and only if no test specified in source code, see:

  • emptyCategoriesSuite()
  • noTestsCategoriesSuite()

Answering your question, see these two tests.
There the inclusions == exclusions

  • nothingRunnable1()
  • nothingRunnable2() (and an extra unknown category)

See oneRunnableOthersAvoided(), what happens when IncludeCategory has all in ExcludeCategory, and an extra known category e.g. Object.class

Contributor

Tibor17 commented May 18, 2012

@stefanbirkner creating a pull does not work for me now.
Your comment is important anyway. So please try to place your comments to whatever of my pulls/issues (#354 or #336 or #307), or even here. -i know it's not nice but we have a slow movement.

Contributor

Tibor17 commented May 24, 2012

Hello guys, any further objections to moving forward with pull #336 and #354?
If you want to start a private discussion, use Skype and find user tibor.digana

Contributor

Tibor17 commented Jul 2, 2012

Hi @dsaff after month nobody has any objections. Can you have a look? We may proceed ahead, i think.
thx, T

Owner

dsaff commented Jul 11, 2012

@tibor17, sorry, I've been away, and lost my context. Are you recommending moving ahead with this pull, or with #336 and #354?

Contributor

Tibor17 commented Jul 11, 2012

I am recommending moving ahead with #336 and #354.
T

Owner

dsaff commented Aug 2, 2012

Okay, proceeding with nailing down #354. Let me know if that's a disputed action.

@dsaff dsaff closed this Aug 2, 2012

@Tibor17 Tibor17 referenced this pull request Sep 12, 2012

Merged

Configurable Categories #503

Contributor

Tibor17 commented Sep 19, 2012

@aslakknutsen
@matheeeny
@stefanbirkner
@langles
You participated in our feature 'support for multiple Include/Exclude categories'.
I forked that feature to #503.
David @dsaff thinks that the javadoc is incomplete.
It's our common feature, thus can you please read this javadoc in Categories class, and briefly write in a comment the javadoc that you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment