CategoriesExcludeFilter and CategoriesIncludeFilter created. #635
Conversation
| return fqn.replace(".", "/") + ".class"; | ||
| } | ||
|
|
||
| public Class<?> forName(String className) throws ClassNotFoundException { |
|
Can we tackle the command-line issue first? I think what could work would be a general-purpose --filter command-line arg. For example, maybe: java org.junit.runner.JUnitCore --filter=com.yap.noel.CategoryFilter=Foo* Where there would need to be a public constructor CategoryFilter(String arg). Thoughts? |
|
Just to clarify, with the --filter option applied to CategoriesIncludeFilter and CategoriesExcludeFilter, would it look something like: JUnitCore --filter='com.yap.noel.CategoriesIncludeFilter=/Category0.class,/Category1.class;com.yap.noel.CategoriesExcludeFilter=/Category2.class,/Category3.class' |
|
@dsaff IMHO we should now postpone non-trivial pulls and release 4.12. Then we should with @stephenc create multi-module maven project, and independently release junit:junit-experiments:4.13-SNAPSHOT for pulls like this one. Source code will be in @stephenc Did you think of multi module project? Tibor |
|
Multi-module projects should not be split across git repositories. Seems you are suggesting the opposite. Wrt cutting a release, I think it would be good to get a few maven based releases out (for practice) and the only thing I would like to fix is the project site now that junit.org is hosted on GitHub Sent from my iPhone On 16 Feb 2013, at 13:29, Tibor Digana [email protected] wrote:
|
|
@stephenc
So the people can check out the preliminary features module, refactor them, and integrate back to junit:junit after they are really handy. |
|
Differing release cycle => separate git repos. On Saturday, 16 February 2013, Tibor Digana wrote:
|
|
I have no issues with not pulling in this change until after 4.12 is released.
|
| public boolean shouldRun(final Description description) { | ||
| if (description.getMethodName() == null) { | ||
| if (description.getChildren().isEmpty()) { | ||
| final Category classCategory = description.getAnnotation(Category.class); |
There was a problem hiding this comment.
On the surface, it looks like this code only considers the Category annotations on the test methods, not the test classes.
Can we somehow reuse Categories.CategoriesFilter?
There was a problem hiding this comment.
I'm working on getting the other changes into this changeset that filter
the classes.
I'll take a look at CategoriesFilter again, but the first time I looked at
it, I didn't see a way to make it so what I wanted.
On Feb 16, 2013 8:42 AM, "Kevin Cooney" [email protected] wrote:
In src/main/java/org/junit/filters/CategoriesFilter.java:
@@ -0,0 +1,28 @@
+package org.junit.filters;
+
+import org.junit.experimental.categories.Category;
+import org.junit.runner.Description;
+import org.junit.runner.manipulation.Filter;
+
+abstract class CategoriesFilter extends Filter {
- public static final String DEFAULT = "";
- @OverRide
- public boolean shouldRun(final Description description) {
if (description.getMethodName() == null) {if (description.getChildren().isEmpty()) {final Category classCategory = description.getAnnotation(Category.class);On the surface, it looks like this code only considers the Categoryannotations on the test methods, not the test classes.
Can we somehow reuse Categories.CategoriesFilter?
—
Reply to this email directly or view it on GitHubhttps://github.com//pull/635/files#r3039689.
There was a problem hiding this comment.
One possible way to reuse the code
- Create the FilterFactory interface I suggested earlier
- Have your FilterFactory implementation go through the Description tree to find all categories used by any of the test classes or methods that match your globs. It would then use that information to create an instance of Categories.FilterFactory and return that
That might require additional creational methods for Categories.FilterFactory. If so, it would be great to get that in soon.
My hope is we can get in the changes so that developers have the hooks they need to do command-line based filtering before the next major version, assuming the changes are small. Implementations of FilterFactory could be added in a later release or to junit.contrib
There was a problem hiding this comment.
@kcooney , I just browsed through Categories.CategoryFilter and think it's way too complicated for what it needs to do -- it breaks the SRP. For example, there's no need to implement both categories include and exclude filtering in the same class. Rather, have one filter implement categories inclusion, another one implement categories exclusion, then use Filter.intersect() to combine both. Such a design makes it more amenable to combining other filters, too (eg an IgnoreFilter can be created that would obsolete specific @Ignore processing in JUnit (IIRC, such processing violates DRY since it occurs in at least two different places).
IMO (and I can surely be biased towards my own implementation :-), Categories.CategoryFilter ought to be deprecated in favor of CategoriesIncludeFilter and CategoriesExcludeFilter. I'm not opposed to making the latter two inner classes of Categories.
I'm already working on a 'FilterFactory' in order to support the --filter option (and possibly XML-based configurations if it's decided to move in that direction in the future). Such a factory was actually needed for our Ant task (especially since we do allow custom-filters to be used). (FWIW, I'd like to move as much of this functionality into JUnit proper since we're moving towards using Gradle for our builds and it looks like it's easier to (finally) contribute to JUnit rather than reimplementing something hacky in Gradle).
There was a problem hiding this comment.
@noel-yap, agree in abstract principle that splitting responsibilities when possible is a good thing. That said, CategoryFilter is published, and deprecation has its costs. I don't see an argument above that convinces me that deprecating CategoryFilter would serve users in good stead--anyone who wants to implement inclusion in one object and exclusion in another can create two instances of CategoryFilter, and intersect them or do whatever else they wish.
|
@noel-yap Good to see you again! The main thing that would help for me would be to have some integration tests. One way to do that:
Edit: As a bonus, the integration tests would make it easier for us to come up with content for the wiki There are probably examples in the code base that do something similar. I'm particularly interested in:
If we all agree on how one will tell |
|
Good working with you again, too :-) I'll work on these tests as part of the --filter processing.
|
|
@noel-yap It might be nice to know what changes we might need to @dsaff The problem with requiring the filter to have a constructor that takes in a String is it might require the developer to do a lot of "work" in the constructor (we commonly see this problem at my job when we write custom Runners). I suggest a factory interface: The reason why I would like to have the top level description object passed in is there are times when you cannot define the filter without knowing what the tree looks like. If you are worried about developers wanting more parameters in the future, we could pass in a |
|
I'd like to say I'll have something next week, but I'm planning to be OOO
|
|
@noel-yap Regarding the command line. I do not want to make you disapointed, but command line staff was discussed. |
|
@Tibor17 , perhaps I misunderstand, but it looks to me as though my pull request is complementary to what's discussed in #503. Adding --filter would allow any number of perhaps-custom filters to be used in JUnit. Command line support doesn't mean XML-based configuration ought not be considered, either. I'm not opposed to leaving out the '.class' in the filter and categories specifications. The only reason it's there is to maintain consistency with how our original build system filtered in and out classes (ie by using Ant globbing). I'm even open to removing the Ant globbing altogether and having the user specify FQCNs (but that could be a pain if ever such classes were refactored into other packages). The important parts for me is to have some easy way to specify filter configuration on the command line and for tests that have been filtered out to call |
|
@noel-yap I tried to explain to you that the JUnit has the tendency to lead the style of programmatic API. |
|
@Tibor17 , sorry, I'm new to github and am having trouble finding the specifics about the properties you mentioned. |
|
@noel-yap |
|
Catching up here. @kcooney, FilterFactory is a strictly better suggestion than my magic-constructor proposal. Thank you. @noel-yap, we run the danger that by describing this pull request "better to support Categories", and starting off with code that doesn't actually intersect with the currently-running code, we've created a bit of a mirror into which each of us can project our own desires about what the obvious next thing to do with Categories is. At this point, I'm personally most interested in the possibility of creating a standard command-line filter parameter. This has several advantages:
@Tibor17, see above for why I am interested in a general filter parameter, but remain uninterested (as I was in #503) in introducing special-case properties or args just for categories. The conversation thus far has not convinced me that there's anything fundamentally wrong with CategoryFilter that justifies replacing, vs. fixing it. Once we have a general command-line filtering mechanism in place, things like ant-globbing can become specific to a particular filter implementation, and can be discussed in a smaller scope. |
|
@dsaff , how about something like:
JUnitCore.setFilter() can also be defined in the API for those not wanting to go through the command line. While direct support for properties won't be implemented, custom filters can use properties if they want. |
|
@noel-yap, I like setFilter(). I was hoping we could combine filters by including multiple --filter args on the command line, but perhaps that raises the spectre of whether multiple filters are intersected or unioned. My gut feeling is that single class name + string arg will be perfectly sufficient, and I'd like to draw the line long before we risk maintaining a complicated DSL, with all of the parsing and escaping issues that raises. |
|
@dsaff , multiple --filter has the advantage of being easier to assemble than using infix '|' syntax. Let's go with that. It would mean renaming I'm not sure how useful filter union would be. Even if someone wanted to do that, they could just call JUnit multiple times. Single class name with one |
|
@noel-yap, how about we get the factory class case built, as a minimum viable feature, and then consider the static method proposal as an extension? |
|
@noel-yap To be clear, are these command line parameters or system Note that if we use system properties, you could choose to use system -Dorg.junit.filter=
|
|
@kcooney, I'd like to use command-line parameters first: system properties are inherently global in ways that I'd like to avoid, but we could revisit that once we have a syntax we like. |
|
@dsaff , since we're talking about accepting only constructors that take in only one (or no) I expect to have something by EOD tomorrow or early next week. |
JUnitCore.addFilter() created. Filtered out methods and classes treated as if they were @ignored.
|
I think the messiest part of the latest changeset is the FilteredClassRunner decorator. The functionality in its runChild() could be moved directly into BlockJUnit4ClassRunner and its existing subclasses that override runChild() but I wasn't sure if that would have other consequences. |
… the Ignore annotation handling into a more general filter handling; the specific Ignore annotation handling is replaced with an IgnoreFilter instance.
|
It turns out that moving the FilteredClassRunner.runChild() logic into BlockJUnit4ClassRunner wasn't so bad. |
|
Looks like in github when you comment on an earlier commit, it's really hard to do a reply, so I'll summarize
|
| system.out().println("JUnit version " + Version.id()); | ||
| List<Class<?>> classes = new ArrayList<Class<?>>(); | ||
| List<Failure> missingClasses = new ArrayList<Failure>(); | ||
| List<Failure> failures = new ArrayList<Failure>(); | ||
| FilterFactory filterFactory = new FilterFactory(); | ||
| for (String each : args) { |
There was a problem hiding this comment.
Could we extract a class to do the command line parsing?
CommandLineParser parser = new CommandLineParser(args);
parser.parse();
List<Class<?>> classes = parser.getClasses();
List<Failure> failures = parser.getFailures();
List<FilterFactory> filterFactories = parser.getFilterFactories();
|
@kcooney , what is the advantage of keeping @ignore and filtering separate such that filtering receives no notifications? The advantage I see of combining the two is simplification; there'll be fewer concepts floating around; @ignore handling will simply be just another Filter. FWIW, my group has created a custom Ant task to use ignore notifications for filtered tests so that we know which tests are being skipped. These tests are currently being skipped in CI because they've been Categorized as Manual or DevMachine. This allows the tests to be run locally while being skipped on CI. @ignore doesn't allow this capability. We're going to be switching over to Gradle and was hoping I could move the above functionality into JUnit itself rather than hacking yet another build system to do what we want. I'm thinking the gains in putting it into JUnit would be greater than the gains in putting it into Gradle. Perhaps the proper solution would be to introduce something new, something like a NotifiedFilter? Or maybe there ought to be some option (somewhere) that indicates whether Filters perform notification when a test is skipped? I've started a separate thread for this topic. |
This is the first of at least a few changes that aim better to support Categories (eg creating the Request with the Filters is needed).
One open issue with this specific change is how best to specify from the command line the included and excluded categories. My current code uses the properties test.categories.include and test.categories.exclude. Are there other examples of JUnit using properties? Is there some naming convention to follow?
I'm sure there'll be lots of other questions, comments, etc (eg does JUnit already have support of Ant-style globbing that this change can just reuse?).