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

--filter option implemented. #647

Merged
merged 23 commits into from Jun 5, 2013

Conversation

Projects
None yet
6 participants
Contributor

noel-yap commented Mar 1, 2013

Currently SuiteTest.suiteShouldBeOKwithNonDefaultConstructor() fails but I don't know what ought to be done about that. Under what circumstances does it make sense to have a Suite with no SuiteClasses? Suggestions?

I'll also be refactoring out the command line processing as per @kcooney and will writing more tests for the code that has changed.

@kcooney kcooney and 1 other commented on an outdated diff Mar 1, 2013

...ava/org/junit/experimental/categories/Categories.java
@@ -304,6 +308,63 @@ private static boolean hasNull(Class<?>... classes) {
}
return false;
}
+
+ private static class CategoryFilterWrapper extends Filter {
@kcooney

kcooney Mar 1, 2013

Member

Why is this needed? CategoryFilter extends Filter

@noel-yap

noel-yap Mar 1, 2013

Contributor

CategoryFilterWrapper removed. Code changed to use CategoryFilter.

@kcooney kcooney and 1 other commented on an outdated diff Mar 1, 2013

...ava/org/junit/experimental/categories/Categories.java
+ protected abstract Filter createFilter(Class<?>[] categories) throws Exception;
+
+ public Class<?>[] parseCategories(String categories) throws ClassNotFoundException {
+ List<Class<?>> categoryClasses = new ArrayList<Class<?>>();
+
+ for (String category : categories.split(",")) {
+ Class<?> categoryClass = Class.forName(category);
+
+ categoryClasses.add(categoryClass);
+ }
+
+ return categoryClasses.toArray(new Class[]{});
+ }
+ }
+
+ public static class IncludesAnyFilterFactory extends CategoriesFilterFactory {
@kcooney

kcooney Mar 1, 2013

Member

The name on the command line will be quite long (--filter=org.junit.experimental.categories.Categories.IncludesAnyFilterFactory)

I suggest making this a top-level class and naming it IncludeCategories

Alternatively, you could create one top-level class that handles category includes and excludes (prefixing excluded categories with a -)

@kcooney

kcooney Mar 1, 2013

Member

Please make the build-in FilterFactory implementations final.

@noel-yap

noel-yap Mar 1, 2013

Contributor

Done.

IIRC, @dsaff wanted to stay away from special syntax and DSLs.

@kcooney kcooney and 1 other commented on an outdated diff Mar 1, 2013

src/main/java/org/junit/filters/PassThroughFilter.java
@@ -0,0 +1,21 @@
+package org.junit.filters;
+
+import org.junit.runner.Description;
+import org.junit.runner.manipulation.Filter;
+
+public class PassThroughFilter extends Filter {
@kcooney

kcooney Mar 1, 2013

Member

Please remove; you can use Filter.ALL

@noel-yap

noel-yap Mar 1, 2013

Contributor

Done.

@kcooney kcooney and 1 other commented on an outdated diff Mar 1, 2013

src/main/java/org/junit/runner/FilterFactory.java
@@ -0,0 +1,20 @@
+package org.junit.runner;
+
+import org.junit.runner.manipulation.Filter;
+import sun.reflect.generics.reflectiveObjects.NotImplementedException;
+
+public class FilterFactory {
@kcooney

kcooney Mar 1, 2013

Member

Please add Javadoc to all public classes and all public methods.

Also, this class should be abstract or it should be an interface.

@noel-yap

noel-yap Mar 1, 2013

Contributor

Will add JavaDoc.

Class made abstract.

@kcooney kcooney and 1 other commented on an outdated diff Mar 1, 2013

src/main/java/org/junit/runner/FilterFactory.java
@@ -0,0 +1,20 @@
+package org.junit.runner;
+
+import org.junit.runner.manipulation.Filter;
+import sun.reflect.generics.reflectiveObjects.NotImplementedException;
+
+public class FilterFactory {
+ public Filter createFilter() throws FilterNotFoundException {
@kcooney

kcooney Mar 1, 2013

Member

Why do we need more than one createFilter() method?

Edit: If we have only one createFilter method we can remove FilterNotFoundException

@noel-yap

noel-yap Mar 1, 2013

Contributor

createFilter() is for --filter=factory. createFilter(String) is for --filter=factory=args

FilterNotFoundException is a bad name for the exception. It's an exception meant to be thrown if the filter cannot be created. I'll rename the exception. Do you have any preferences for the name?

@kcooney

kcooney Mar 2, 2013

Member

No suggestions for the name; if you remember, I'm horrible at naming things :-)

I think we can combine FilterNotFoundException and FilterNotCreatedException into one exception (FilterCreationException ?) that's thrown either when the filter factory cannot be found/created or when the filter factory's create() method throws a RuntimeException

@kcooney kcooney and 1 other commented on an outdated diff Mar 1, 2013

src/main/java/org/junit/runner/FilterFactory.java
+package org.junit.runner;
+
+import org.junit.runner.manipulation.Filter;
+import sun.reflect.generics.reflectiveObjects.NotImplementedException;
+
+public class FilterFactory {
+ public Filter createFilter() throws FilterNotFoundException {
+ throw new NotImplementedException();
+ }
+
+ public Filter createFilter(final String args) throws FilterNotFoundException {
+ throw new NotImplementedException();
+ }
+
+ public static class FilterNotFoundException extends ClassNotFoundException {
+ public FilterNotFoundException(final String message, final Exception exception) {
@kcooney

kcooney Mar 1, 2013

Member

style-nit: In the junit code, I don't think we ever use "final" for parameters unless they are used in anonymous classes

@noel-yap

noel-yap Mar 1, 2013

Contributor

Done. I've also changed my IDE not to make such things final.

@kcooney kcooney and 1 other commented on an outdated diff Mar 1, 2013

src/main/java/org/junit/runner/FilterFactoryFactory.java
@@ -0,0 +1,38 @@
+package org.junit.runner;
+
+import org.junit.runner.manipulation.Filter;
+
+public class FilterFactoryFactory {
@kcooney

kcooney Mar 1, 2013

Member

I think this can be a package-scope class in whichever package uses it

Edit: I'm guessing you want some public API for using FilterFactory. I suggest two methods in Filter:

public final Filter create(Class<? extends FilterFactory> factoryClass, FilterFactoryParams params);
public final Filter create(String factoryClassName, FilterFactoryParams params);
@noel-yap

noel-yap Mar 1, 2013

Contributor

Done.

@kcooney kcooney and 1 other commented on an outdated diff Mar 1, 2013

src/main/java/org/junit/runner/FilterFactoryFactory.java
@@ -0,0 +1,38 @@
+package org.junit.runner;
+
+import org.junit.runner.manipulation.Filter;
+
+public class FilterFactoryFactory {
+ public Filter apply(final String filterSpec)
+ throws FilterFactoryNotFoundException, FilterFactory.FilterNotFoundException {
+ if (filterSpec.contains("=")) {
+ final String[] tuple = filterSpec.split("=", 2);
@kcooney

kcooney Mar 1, 2013

Member

style-nit: We don't use "final" for local variables, either

@noel-yap

noel-yap Mar 1, 2013

Contributor

Removed.

@kcooney kcooney and 1 other commented on an outdated diff Mar 1, 2013

src/main/java/org/junit/runner/FilterFactoryFactory.java
+import org.junit.runner.manipulation.Filter;
+
+public class FilterFactoryFactory {
+ public Filter apply(final String filterSpec)
+ throws FilterFactoryNotFoundException, FilterFactory.FilterNotFoundException {
+ if (filterSpec.contains("=")) {
+ final String[] tuple = filterSpec.split("=", 2);
+
+ final FilterFactory filterFactory = create(tuple[0]);
+ final String args = tuple[1];
+
+ return filterFactory.createFilter(args);
+ } else {
+ final FilterFactory filterFactory = create(filterSpec);
+
+ return filterFactory.createFilter();
@kcooney

kcooney Mar 1, 2013

Member

Here we can pass an empty string

@noel-yap

noel-yap Mar 1, 2013

Contributor

I'd rather not assume that someone would want to have an empty string mean something other than no arguments. null can be passed in, but I suspect that'll cause lots of problems down the road.

@kcooney

kcooney Mar 2, 2013

Member

I think I'm misunderstanding something. By my reading of the code, the syntax of the filter flag is:

--filter=com.google.testing.Include=arg

It sounds like you are suggesting that the following two could mean different things:

--filter=com.google.testing.Include
--filter=com.google.testing.Include=

That seems a bit confusing. Can you give an example where these would mean something different?

@kcooney

kcooney Mar 10, 2013

Member

@noel-yap Could you also respond to this comment? Thanks.

@kcooney kcooney and 1 other commented on an outdated diff Mar 1, 2013

src/main/java/org/junit/runner/FilterFactoryFactory.java
+
+ final FilterFactory filterFactory = create(tuple[0]);
+ final String args = tuple[1];
+
+ return filterFactory.createFilter(args);
+ } else {
+ final FilterFactory filterFactory = create(filterSpec);
+
+ return filterFactory.createFilter();
+ }
+ }
+
+ private FilterFactory create(final String filterFactoryFqcn) throws FilterFactoryNotFoundException {
+ try {
+ final Class<? extends FilterFactory> filterFactoryClass =
+ (Class<? extends FilterFactory>) Class.forName(filterFactoryFqcn);
@kcooney

kcooney Mar 1, 2013

Member

suggestion: instead of a normal cast, I suggest using Class.asSubclass

@noel-yap

noel-yap Mar 1, 2013

Contributor

NEAT! Class.asSubClass used.

@noel-yap

noel-yap Mar 1, 2013

Contributor

Thread.getContextClassLoader() used.

@kcooney kcooney and 1 other commented on an outdated diff Mar 1, 2013

src/main/java/org/junit/runner/JUnitCore.java
}
/**
* @param system
* @args args from main()
*/
- private Result runMain(JUnitSystem system, String... args) {
+ public Result runMain(JUnitSystem system, String... args) {
@kcooney

kcooney Mar 1, 2013

Member

Why are you making this public?

@noel-yap

noel-yap Mar 1, 2013

Contributor

Hmm, the only external caller I see is the integration test. I've made it package scope.

@kcooney kcooney and 1 other commented on an outdated diff Mar 1, 2013

src/main/java/org/junit/runner/JUnitCore.java
for (String each : args) {
try {
- classes.add(Class.forName(each));
+ if (each.startsWith("--")) {
+ if (each.startsWith("--filter")) {
+ String filterSpec = each.substring(each.indexOf('=') + 1);
@kcooney

kcooney Mar 1, 2013

Member

I think that this:

--flag flagValue

Is a bit more common than this:

--flag=flagValue

I don't feel strongly about whether we should support the second version, but I think we should support the first

@noel-yap

noel-yap Mar 1, 2013

Contributor

Having to deal with variable number of args per flag starts making the command line parsing more complicated. How willing is the project to start depending on a third-party library to handle command line parsing?

@kcooney

kcooney Mar 2, 2013

Member

Then let's not make the number of arguments per flag variable and stick with the --flag flagValue syntax. I'm personally fine with the idea that the flags must be at the beginning of the command line, so you just chomp the args two at a time until you stop seeing args proceeded by '--'

@kcooney kcooney and 1 other commented on an outdated diff Mar 1, 2013

...ava/org/junit/runner/FilterOptionIntegrationTest.java
+import static junit.framework.Assert.assertTrue;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class FilterOptionIntegrationTest {
+ private static final String INCLUDES_DUMMY_CATEGORY_0 = "--filter=" +
+ Categories.CategoryFilter.IncludesAnyFilterFactory.class.getName() + "=" + DummyCategory0.class.getName();
+ private static final String EXCLUDES_DUMMY_CATEGORY_1 = "--filter=" +
+ Categories.CategoryFilter.ExcludesAnyFilterFactory.class.getName() + "=" + DummyCategory1.class.getName();
+
+ private JUnitCore jUnitCore;
+ private TestListener testListener;
+
+ @Before
+ public void setUp() {
+ jUnitCore = new JUnitCore();
@kcooney

kcooney Mar 1, 2013

Member

In JUnit4-style tests, you can assign variables where they are defined (and make them final if you want :-). JUnit4 won't create an instance of the class until just before it is run, and it will make it eligible for garbage collection right after it is run.

@noel-yap

noel-yap Mar 1, 2013

Contributor

I had forgotten JUnit creates a new object per test. Thanks for the reminder.

@kcooney kcooney and 1 other commented on an outdated diff Mar 1, 2013

...ava/org/junit/runner/FilterOptionIntegrationTest.java
+ DummyTestClass0.class.getName(),
+ DummyTestClass1.class.getName(),
+ DummyTestClass01.class.getName(),
+ DummyTestClass0TestMethod1.class.getName());
+
+ assertNotStarted(DummyTestClass.class);
+ assertFinished(DummyTestClass0.class);
+ assertNotStarted(DummyTestClass1.class);
+ assertNotStarted(DummyTestClass01.class);
+ assertNotStarted(DummyTestClass0TestMethod1.class);
+ assertThat("runCount does not match", result.getRunCount(), is(1));
+ assertThat("failureCount does not match", result.getFailureCount(), is(0));
+ }
+
+ private Result runJUnit(final String... args) {
+ return jUnitCore.runMain(new RealSystem(), args);
@kcooney

kcooney Mar 1, 2013

Member

I don't think you want to use RealSystem() here; doesn't that really call System.exit()?

@noel-yap

noel-yap Mar 1, 2013

Contributor

I really didn't like the output when this test ran but didn't know how to make it quiet. Thanks.

Member

kcooney commented Mar 1, 2013

@noel-yap Overall looks very nice. Like the tests :-)

Owner

dsaff commented Mar 1, 2013

@noel-yap, looking forward to checking this out. Your comments indicate that another commit is on its way, so I may wait to get all the changes at once.

SuiteTest.suiteShouldBeOKwithNonDefaultConstructor() is asserting that since we never try to construct the class to which @runwith(Suite.class) is attached, we shouldn't do any validation of any constructors we might find on that class. I forget now what the use case is, but if it's possible to not break it, it would be ideal.

Contributor

noel-yap commented Mar 1, 2013

@dsaff , after making some of the changes @kcooney suggested, all tests now pass. I slightly suspect it's from replacing PassThroughFilter with Filter.ALL (since all other changes have been more aesthetic), but even that change doesn't look like it does much different. Anyway, all tests pass now so I'm not gonna investigate further.

Contributor

noel-yap commented Mar 1, 2013

Some stuff I still need to do:

  • JavaDoc
  • tests for new code (yeah, I didn't TDD this)
  • factor out the command line parsing

Some open issues:

  • Is it OK to introduce a dependency on a third-party command-line parser? I'm guessing it's not OK, but I don't want to make assumptions.
Owner

dsaff commented Mar 1, 2013

@noel-yap,

Yes, I'd really prefer to avoid a third-party command-line parser. My hope (fingers crossed) is that we can keep it simple enough to make rolling our own cost-effective.

@kcooney kcooney and 1 other commented on an outdated diff Mar 2, 2013

...ava/org/junit/experimental/categories/Categories.java
+ @Override
+ public Filter createFilter(String categories) throws FilterNotCreatedException {
+ try {
+ return createFilter(parseCategories(categories));
+ } catch (Exception e) {
+ throw new FilterNotCreatedException("Could not create IncludesAny filter.", e);
+ }
+ }
+
+ protected abstract Filter createFilter(Class<?>[] categories) throws Exception;
+
+ public Class<?>[] parseCategories(String categories) throws ClassNotFoundException {
+ List<Class<?>> categoryClasses = new ArrayList<Class<?>>();
+
+ for (String category : categories.split(",")) {
+ Class<?> categoryClass = Class.forName(category);
@kcooney

kcooney Mar 2, 2013

Member

Here you are using Class.forName() instead of the context class loader

@noel-yap

noel-yap Mar 4, 2013

Contributor

Fixed.

Fixed the one in JUnitCore, too.

@Tibor17 Tibor17 and 2 others commented on an outdated diff Mar 2, 2013

...ava/org/junit/experimental/categories/Categories.java
+ static abstract class CategoriesFilterFactory extends FilterFactory {
+ @Override
+ public Filter createFilter(String categories) throws FilterNotCreatedException {
+ try {
+ return createFilter(parseCategories(categories));
+ } catch (Exception e) {
+ throw new FilterNotCreatedException("Could not create IncludesAny filter.", e);
+ }
+ }
+
+ protected abstract Filter createFilter(Class<?>[] categories) throws Exception;
+
+ public Class<?>[] parseCategories(String categories) throws ClassNotFoundException {
+ List<Class<?>> categoryClasses = new ArrayList<Class<?>>();
+
+ for (String category : categories.split(",")) {
@Tibor17

Tibor17 Mar 2, 2013

Contributor

String#split is performance issue.
Instead use StringTokenizer pls.

@kcooney

kcooney Mar 2, 2013

Member

@Tibor17 http://en.wikipedia.org/wiki/Program_optimization#Quotes

(really, for users' test runs, this will only get called at most once per test run; the cost of the reflective calls will be much greater)

@noel-yap

noel-yap Mar 4, 2013

Contributor

I compared both implementations and the one using split() is much cleaner so I'll stick with that considering the performance gain won't be much.

@Tibor17 Tibor17 commented on an outdated diff Mar 2, 2013

src/main/java/org/junit/runner/FilterFactoryFactory.java
@@ -0,0 +1,39 @@
+package org.junit.runner;
+
+import org.junit.runner.manipulation.Filter;
+
+class FilterFactoryFactory {
+ public Filter createFilterFromFilterSpec(String filterSpec)
+ throws FilterFactoryNotFoundException, FilterFactory.FilterNotCreatedException {
+ String filterFactoryFqcn;
+ FilterFactoryParams args;
+
+ if (filterSpec.contains("=")) {
+ String[] tuple = filterSpec.split("=", 2);
@Tibor17

Tibor17 Mar 2, 2013

Contributor

again

JavaDoc added.
"--filter arg" support added (as opposed to "--filter=arg").
Command line parser factored into JUnitCommandLineParser class.
Unit tests added.
Owner

dsaff commented Mar 7, 2013

@noel-yap,

Looks like this has drifted against HEAD. Can you pull/merge/push? Thanks.

Merge branch 'master' into filter-option
Conflicts:
	src/test/java/org/junit/tests/AllTests.java

@kcooney kcooney and 1 other commented on an outdated diff Mar 10, 2013

.../junit/experimental/categories/ExcludeCategories.java
+ * Usage from command line:
+ * <code>
+ * --filter=org.junit.experimental.categories.ExcludeCategories=package.of.FirstCategory,package.of.SecondCategory
+ * </code>
+ *
+ * Usage from API:
+ * <code>
+ * new ExcludeCategories().createFilter(new Class[]{
+ * FirstCategory.class,
+ * SecondCategory.class
+ * });
+ * </code>
+ */
+public final class ExcludeCategories extends CategoryFilterFactory {
+ @Override
+ public Filter createFilter(Class<?>[] categories) {
@kcooney

kcooney Mar 10, 2013

Member

Could you use varargs here?

@noel-yap

noel-yap Mar 11, 2013

Contributor

Done.

@kcooney kcooney and 1 other commented on an outdated diff Mar 10, 2013

.../junit/experimental/categories/ExcludeCategories.java
+ *
+ * Usage from API:
+ * <code>
+ * new ExcludeCategories().createFilter(new Class[]{
+ * FirstCategory.class,
+ * SecondCategory.class
+ * });
+ * </code>
+ */
+public final class ExcludeCategories extends CategoryFilterFactory {
+ @Override
+ public Filter createFilter(Class<?>[] categories) {
+ return new ExcludesAny(categories);
+ }
+
+ public static class ExcludesAny extends CategoryFilter {
@kcooney

kcooney Mar 10, 2013

Member

Can this be private?

@noel-yap

noel-yap Mar 11, 2013

Contributor

Done.

@kcooney kcooney and 1 other commented on an outdated diff Mar 10, 2013

src/main/java/org/junit/runner/FilterFactory.java
@@ -0,0 +1,37 @@
+package org.junit.runner;
+
+import org.junit.runner.manipulation.Filter;
+import sun.reflect.generics.reflectiveObjects.NotImplementedException;
@kcooney

kcooney Mar 10, 2013

Member

Could we use java.lang.UnsupportedOperationException instead?

@noel-yap

noel-yap Mar 11, 2013

Contributor

Done.

@kcooney kcooney and 1 other commented on an outdated diff Mar 10, 2013

src/main/java/org/junit/runner/FilterFactoryParams.java
@@ -0,0 +1,68 @@
+package org.junit.runner;
+
+import org.junit.internal.ClassUtil;
+import org.junit.runner.manipulation.Filter;
+
+import static org.junit.runner.FilterFactoryFactory.FilterFactoryNotCreatedException;
+
+/**
+ * Parameters to a {@link FilterFactory}.
+ */
+public abstract class FilterFactoryParams {
@kcooney

kcooney Mar 10, 2013

Member

I was actually thinking that FilterFactoryParams would be a non-abstract, final POJO with getter methods and a constructor that took a String. I was also thinking that FilterFactoryParams would be passed to FilterFactory.createFilter. That way, we can add more optional parameters to FilterFactoryParams and pass it to FilterFactory implementations without having to add more methods to FilterFactory

@noel-yap

noel-yap Mar 11, 2013

Contributor

Done.

@kcooney kcooney and 1 other commented on an outdated diff Mar 10, 2013

src/main/java/org/junit/runner/FilterFactoryFactory.java
+ * Creates a {@link Filter}.
+ *
+ * @param filterFactoryFqcn The fully qualified class name of the {@link FilterFactory}
+ * @param args The arguments to the {@link FilterFactory}
+ * @throws FilterFactory.FilterNotCreatedException
+ * @throws FilterFactoryNotCreatedException
+ */
+ public Filter createFilter(String filterFactoryFqcn, FilterFactoryParams args)
+ throws FilterFactory.FilterNotCreatedException, FilterFactoryNotCreatedException {
+ return args.apply(filterFactoryFqcn);
+ }
+
+ /**
+ * Exception thrown if the {@link FilterFactory} cannot be created.
+ */
+ public static class FilterFactoryNotCreatedException extends ClassNotFoundException {
@kcooney

kcooney Mar 10, 2013

Member

I vote for getting rid of this and just using FilterNotCreatedException.

@noel-yap

noel-yap Mar 11, 2013

Contributor

I've found it helpful differentiating between the two type of errors.

@kcooney

kcooney Mar 12, 2013

Member

Could one exception be a subclass of another? Some of the methods throw both, and that seems to be a loss of abstraction (not to mention a bit wordy)

@noel-yap

noel-yap Mar 12, 2013

Contributor

Done.

@kcooney kcooney and 1 other commented on an outdated diff Mar 10, 2013

...ain/java/org/junit/runner/JUnitCommandLineParser.java
+ }
+
+ void parseParameters(String[] args) {
+ for (String arg : args) {
+ try {
+ classes.add(ClassUtil.getClass(arg));
+ } catch (ClassNotFoundException e) {
+ system.out().println("Could not find class: " + arg);
+ Description description = Description.createSuiteDescription(arg);
+ Failure failure = new Failure(description, e);
+ failures.add(failure);
+ }
+ }
+ }
+
+ public static class Error extends Exception {
@kcooney

kcooney Mar 10, 2013

Member

This is a confusing name (too similar to java.lang.Error). Can you think of a more specific name?

@noel-yap

noel-yap Mar 11, 2013

Contributor

Renamed to CommandLineParserError.

@kcooney kcooney and 2 others commented on an outdated diff Mar 10, 2013

src/main/java/org/junit/runner/JUnitCore.java
- List<Failure> missingClasses = new ArrayList<Failure>();
- for (String each : args) {
- try {
- classes.add(Class.forName(each));
- } catch (ClassNotFoundException e) {
- system.out().println("Could not find class: " + each);
- Description description = Description.createSuiteDescription(each);
- Failure failure = new Failure(description, e);
- missingClasses.add(failure);
- }
- }
+
+ JUnitCommandLineParser jUnitCommandLineParser = new JUnitCommandLineParser(system);
+ jUnitCommandLineParser.parseArgs(args);
+
+ filter = filter.intersect(jUnitCommandLineParser.getFilter());
@kcooney

kcooney Mar 10, 2013

Member

It seems strange to me that runMain() changes the sate of JUnitCore (which is does now, by changing the filter field). Could JUnitCommandLineParser have a createRequest method that returns a Request?

Edit: To clarify, you could do this, then get rid of the filter field.

@noel-yap

noel-yap Mar 11, 2013

Contributor

Users should be able to use the API to add filters. Currently, this is done via JUnitCore.addFilter(). It doesn't make sense to me for users using the API to have to go through JUnitCommandLineParser to do this. It sounds like there should be some other class that handles this. Any suggestions for the name of this class?

@kcooney

kcooney Mar 12, 2013

Member

You can use the Request API to add filters. The JUnitCore facade makes it easier to do some operations, but if you need that kind of power, I personally think it's reasonable to ask that you use Request. @dsaff do you have an opinion here?

@noel-yap Does it not seem strange to you to have runMain() change the currently-set filter?

Edit: It looks like runMain() already had the side-effect of adding a listener. Let's just change runMain() to a static method. It was private before, so that should be safe.

@dsaff

dsaff Mar 12, 2013

Owner

What I'd prefer is that there should be a call that takes the command-line args and returns a Request. This could be used here, or by any other framework (Eclipse, maven) that wants to use the same command-line processing.

@noel-yap

noel-yap Mar 14, 2013

Contributor

I've created JUnitCommandLineParser.createRequest(), modified JUnitCore.runMain() to use it, and removed JUnitCore.filter.

@kcooney , FilterOptionIntegrationTest currently uses JUnitCore.addListener so JUnitCore.runMain() can't currently be made static.

@kcooney

kcooney Mar 14, 2013

Member

@noel-yap We can't see your changes until you push to github

@noel-yap

noel-yap Mar 14, 2013

Contributor

@kcooney , yeah, something's come up that's taken a higher priority. I'll push when I get a chance what I have now, but I haven't yet gone through to ensure all public stuff has JavaDoc or ensured that new code has tests.

@kcooney kcooney and 1 other commented on an outdated diff Mar 10, 2013

...xperimental/categories/CategoryFilterFactoryTest.java
+import org.junit.runner.FilterFactory;
+import org.junit.runner.manipulation.Filter;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class CategoryFilterFactoryTest {
+ @Rule
+ public ExpectedException expectedException = ExpectedException.none();
+
+ @Test
+ public void shouldCreateFilter() throws Exception {
+ CategoryFilterFactory categoryFilterFactory = new CategoryFilterFactoryStub();
+ Filter filter = categoryFilterFactory.createFilter(CategoryFilterFactoryStub.class.getName());
+
+ assertThat(filter, is((Filter) null));
@kcooney

kcooney Mar 10, 2013

Member

It seems strange to test for null in this test, since it is testing the happy path. Can you create a FilterStub?

@noel-yap

noel-yap Mar 11, 2013

Contributor

Done.

@kcooney kcooney and 1 other commented on an outdated diff Mar 10, 2013

...xperimental/categories/CategoryFilterFactoryTest.java
+
+public class CategoryFilterFactoryTest {
+ @Rule
+ public ExpectedException expectedException = ExpectedException.none();
+
+ @Test
+ public void shouldCreateFilter() throws Exception {
+ CategoryFilterFactory categoryFilterFactory = new CategoryFilterFactoryStub();
+ Filter filter = categoryFilterFactory.createFilter(CategoryFilterFactoryStub.class.getName());
+
+ assertThat(filter, is((Filter) null));
+ }
+
+ @Test
+ public void shouldThrowException() throws Exception {
+ expectedException.expect(FilterFactory.FilterNotCreatedException.class);
@kcooney

kcooney Mar 10, 2013

Member

You should call this right before the line you expect to throw the exception, and have no code after that line.

@noel-yap

noel-yap Mar 11, 2013

Contributor

Done.

@kcooney kcooney and 1 other commented on an outdated diff Mar 10, 2013

...t/java/org/junit/runner/FilterFactoryFactoryTest.java
+public class FilterFactoryFactoryTest {
+ @Test
+ public void shouldCreateFilterWithArguments() throws Exception {
+ FilterFactoryFactory filterFactoryFactory = new FilterFactoryFactory();
+ Filter filter = filterFactoryFactory.createFilterFromFilterSpec(
+ ExcludeCategories.class.getName() + "=" + DummyCategory.class.getName());
+
+ assertThat(filter, instanceOf(ExcludeCategories.ExcludesAny.class));
+ }
+
+ @Test
+ public void shouldCreateFilterWithNoArguments() throws Exception {
+ FilterFactoryFactory filterFactoryFactory = new FilterFactoryFactory();
+ Filter filter = filterFactoryFactory.createFilterFromFilterSpec(FilterFactoryStub.class.getName());
+
+ assertThat(filter, is((Filter) null));
@kcooney

kcooney Mar 10, 2013

Member

Again, testing for null is odd here.

@noel-yap

noel-yap Mar 11, 2013

Contributor

Done.

@kcooney kcooney and 1 other commented on an outdated diff Mar 10, 2013

...t/java/org/junit/runner/FilterFactoryFactoryTest.java
+ }
+
+ @Test
+ public void shouldCreateFilterWithNoArguments() throws Exception {
+ FilterFactoryFactory filterFactoryFactory = new FilterFactoryFactory();
+ Filter filter = filterFactoryFactory.createFilterFromFilterSpec(FilterFactoryStub.class.getName());
+
+ assertThat(filter, is((Filter) null));
+ }
+
+ @Test
+ public void shouldCreateFilter() throws Exception {
+ FilterFactoryFactory filterFactoryFactory = new FilterFactoryFactory();
+ Filter filter = filterFactoryFactory.createFilter(FilterFactoryStub.class, new FilterFactoryParams.ZeroArg());
+
+ assertThat(filter, is((Filter) null));
@noel-yap

noel-yap Mar 11, 2013

Contributor

Done.

@kcooney kcooney and 1 other commented on an outdated diff Mar 10, 2013

...st/java/org/junit/runner/FilterFactoryParamsTest.java
+
+import static org.hamcrest.CoreMatchers.instanceOf;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.runner.FilterFactory.FilterNotCreatedException;
+import static org.junit.runner.FilterFactoryFactory.FilterFactoryNotCreatedException;
+import static org.junit.runner.FilterFactoryParams.OneArg;
+
+public class FilterFactoryParamsTest {
+ @Rule
+ public ExpectedException expectedException = ExpectedException.none();
+
+ @Test
+ public void shouldThrowFilterNotCreatedException() throws Exception {
+ expectedException.expect(FilterNotCreatedException.class);
+
+ FilterFactoryParams filterFactoryParams = new FilterFactoryParamsStubThatThrowsFilterNotCreatedException();
@kcooney

kcooney Mar 10, 2013

Member

Move this above the call to expectedException.expect

@noel-yap

noel-yap Mar 11, 2013

Contributor

Done.

@kcooney kcooney and 1 other commented on an outdated diff Mar 10, 2013

...st/java/org/junit/runner/FilterFactoryParamsTest.java
+ public ExpectedException expectedException = ExpectedException.none();
+
+ @Test
+ public void shouldThrowFilterNotCreatedException() throws Exception {
+ expectedException.expect(FilterNotCreatedException.class);
+
+ FilterFactoryParams filterFactoryParams = new FilterFactoryParamsStubThatThrowsFilterNotCreatedException();
+
+ filterFactoryParams.apply(IncludeCategories.class.getName());
+ }
+
+ @Test
+ public void shouldThrowFilterFactoryNotCreatedException() throws Exception {
+ expectedException.expect(FilterFactoryNotCreatedException.class);
+
+ FilterFactoryParams filterFactoryParams = new FilterFactoryParamsStubThatThrowsException();
@noel-yap

noel-yap Mar 11, 2013

Contributor

Done.

@kcooney kcooney and 1 other commented on an outdated diff Mar 10, 2013

...ava/org/junit/runner/FilterOptionIntegrationTest.java
+ private static final String INCLUDES_DUMMY_CATEGORY_0 = "--filter=" +
+ IncludeCategories.class.getName() + "=" + DummyCategory0.class.getName();
+ private static final String EXCLUDES_DUMMY_CATEGORY_1 = "--filter=" +
+ ExcludeCategories.class.getName() + "=" + DummyCategory1.class.getName();
+
+ private JUnitCore jUnitCore = new JUnitCore();
+ private TestListener testListener = new TestListener();
+
+ @Before
+ public void setUp() {
+ jUnitCore.addListener(testListener);
+ }
+
+ @Test
+ public void shouldRunAllTests() {
+ final Result result = runJUnit(
@kcooney

kcooney Mar 10, 2013

Member

nit: remove final (here and elsewhere in this file)

@noel-yap

noel-yap Mar 11, 2013

Contributor

Done.

@kcooney kcooney and 1 other commented on an outdated diff Mar 10, 2013

...ava/org/junit/runner/FilterOptionIntegrationTest.java
+ assertFalse(
+ testClass.getName() + " expected not to have been started but was",
+ testListener.testStarted(testClass));
+ }
+
+ private static class TestListener extends RunListener {
+ private Set<String> startedTests = new HashSet<String>();
+ private Set<String> finishedTests = new HashSet<String>();
+
+ @Override
+ public void testFinished(final Description description) {
+ finishedTests.add(description.getClassName());
+ }
+
+ public boolean testFinished(final Class<?> testClass) {
+ return finishedTests.contains(testClass.getName());
@kcooney

kcooney Mar 10, 2013

Member

Unless I'm missing something, testFinished(class) == testStarted(class) (either a test starts and finishes, or it never starts). Can we just have one method (perhaps named wasRun)?

@noel-yap

noel-yap Mar 11, 2013

Contributor

Done.

I think if things work as expected, testFinished(class) == testStarted(class), but I think it's better if such an assumption isn't made so wasRun(class) is implemented in terms of testFinished and testStarted.

Member

kcooney commented Mar 10, 2013

@noel-yap Thanks for the changes. Looks like a few public classes and methods are missing Javadoc, but I didn't point them all out in case you were still working on things.

Contributor

noel-yap commented Mar 11, 2013

@kcooney , I missed some either accidentally or on purpose (I didn't comment public methods of non-public classes). I'll go through them all to make sure all public classes and methods have JavaDoc.

Member

kcooney commented Mar 12, 2013

@noel-yap were you planning on doing another push with the changes you've made?

I don't know if we generally add JavaDoc for public methods of non-public classes, but I thought I saw missing Javadoc for public classes.

@kcooney kcooney commented on an outdated diff Mar 12, 2013

src/main/java/org/junit/runner/FilterFactoryParams.java
+ * @throws FilterFactory.FilterNotCreatedException
+ * @throws FilterFactoryNotCreatedException
+ */
+ public Filter apply(String filterFactoryFqcn)
+ throws FilterFactory.FilterNotCreatedException, FilterFactoryNotCreatedException {
+ try {
+ FilterFactory filterFactory = ClassUtil.getClass(filterFactoryFqcn)
+ .asSubclass(FilterFactory.class)
+ .getConstructor()
+ .newInstance();
+
+ return apply(filterFactory);
+ } catch (FilterFactory.FilterNotCreatedException e) {
+ throw e;
+ } catch (final Exception e) {
+ throw new FilterFactoryNotCreatedException(e.getMessage());
@kcooney

kcooney Mar 12, 2013

Member

Actually, it occurred to me that the reason I was bothered is because some methods throw both exceptions. Perhaps one should wrap the other.

Note that we could get to this line if FilterFactory.createFilter() throws a runtime exception.

@dsaff It occurs to me that since you are thinking of creating factories for Rules we should decide if factory methods in JUnit should declare that they throw checked exceptions.

@dsaff dsaff and 2 others commented on an outdated diff Mar 15, 2013

src/main/java/org/junit/runner/FilterFactory.java
@@ -0,0 +1,32 @@
+package org.junit.runner;
+
+import org.junit.runner.manipulation.Filter;
+
+/**
+ * Extend this class to create a factory that creates {@link Filter}.
+ */
+public abstract class FilterFactory {
@dsaff

dsaff Mar 15, 2013

Owner

Sorry I'm just now getting into the meat of the code. The two-stage interface here feels unnecessary to me: I was thinking of something that just took a String and returned a Filter. Can you help me understand the motivation for the intervening Params class?

@noel-yap

noel-yap Mar 15, 2013

Contributor

Suggested by @kcooney , it's to allow implementions a way to have factories that accept multiple parameters of different types. For example, CategoryFilterFactoryParams has a getCategories method and CategoryFilterFactory knows about it but other implementations of FilterFactory don't have to get bogged down with whatever other implementations need.

Now that I look it over, I think I see what you mean. @kcooney , can you chime in?

@kcooney

kcooney Mar 16, 2013

Member

@dsaff If the factory method just takes a String, then if in the future we want to pass more parameters, things will get really messy. As soon as this change is in, I'm going to work on adding a second parameter

@noel-yap parse() doesn't belong here; it should go in the calling code

@dsaff

dsaff Mar 18, 2013

Owner

@kcooney, I can see where you're going with this. I was confused by having the factory responsible both for parsing the string and consuming the parsed content.

Which second parameter were you thinking about?

@noel-yap

noel-yap Mar 18, 2013

Contributor

@kcooney , I was thinking a second parameter can be added by those that need it. For example:

public class MyFilterFactoryParams extends FilterFactoryParams {
  private int arg0;
  private double arg1;
  public MyFilterFactoryParams(String args) {
    this(getFirstParam(args), getSecondParam(args));
  }
  public MyFilterFactoryParams(int arg0, double arg1) {
    this.arg0 = arg0;
    this.arg1 = arg1;
  }
  int getArg0() {
    return arg0;
  }
  double getArg1() {
    return arg1;
  }
}

Since MyFilterFactory knows it's going to use MyFilterFactoryParams, it can safely perform a downcast.

What did you have in mind?

@kcooney

kcooney Mar 19, 2013

Member

@dsaff I would like to pass the top level Description to the filter factory. I can think of two cases where I cannot define the filter without looking at the Description tree.

I agree that having the factory parse the strong to create the Params object and then consuming the params object is confusing

@kcooney kcooney and 1 other commented on an outdated diff Mar 16, 2013

src/main/java/org/junit/runner/FilterFactoryParams.java
@@ -0,0 +1,7 @@
+package org.junit.runner;
+
+/**
+ * Parameters to a {@link FilterFactory}.
+ */
+public interface FilterFactoryParams {
@kcooney

kcooney Mar 16, 2013

Member

@noel-yap My thought was that this class would be a final class:

public final class FilterFactoryParams {
  private final String args;

  public FilterFactoryParams(String args) {
    this.args = args;
  }

  public String getArgs() { return args; }
}

We could later add more parameters to the constructor and more getter methods

@noel-yap

noel-yap Mar 18, 2013

Contributor

The way it's implemented now, there's no need to add more constructors. The idea is that FilterFactory implementations know which FilterFactoryParams implementation it'll use so new constructors can be added to those implementations.

@kcooney

kcooney Mar 19, 2013

Member

@noel-yap There would be a need to add more constructors if the JUnit framework itself needed to pass more data to the filter factory. I do agree with David that it's odd that you have the FilterFactory implementations create the FilterFactoryParams implementations when the factory that created the instance is the only consumer of the instance.

@noel-yap

noel-yap Mar 20, 2013

Contributor

I think I understand better what you're thinking. Please take a look at the latest push.

@kcooney kcooney and 1 other commented on an outdated diff Mar 16, 2013

src/main/java/org/junit/runner/FilterFactory.java
+
+/**
+ * Extend this class to create a factory that creates {@link Filter}.
+ */
+public abstract class FilterFactory {
+ public FilterFactoryParams parseArgs(String args) throws FilterNotCreatedException {
+ throw new UnsupportedOperationException();
+ }
+
+ /**
+ * Creates a {@link Filter} using a ${FilterFactoryParams} argument.
+ *
+ * @param params Parameters needed to create the {@link Filter}
+ * @throws FilterNotCreatedException
+ */
+ abstract public Filter createFilter(FilterFactoryParams params) throws FilterNotCreatedException;
@kcooney

kcooney Mar 16, 2013

Member

@dsaff since you are thinking of also creating factory classes for Rules, I'm wondering what you want to do with exceptions in the factories. A few options

a. Have factory methods throw a factory-specific checked exception (like what we have here)
b. Have factory methods not declare any exceptions
c. Hibrid:

public abstract class FooFactory {

  protected abstract Foo create(FooFactoryParams params);

  public final Foo createFoo(FooFactoryParams params) throws FooNotCreatedException {
    try {
      Foo foo = create(params);
      if (foo == null) {
        throw new FooNotCreatedException();
      }
      return foo;
    } catch (RuntimeException e) {
      throw new FooNotCreatedException(e);
   }
}
@dsaff

dsaff Mar 18, 2013

Owner

When possible, I'd prefer us to have no exceptions thrown at all: for example, rather than throwing an exception, a rule factory can substitute an always-failing Statement for the Statement it's given to flag any errors.

In this case, we probably don't have that option. I think a single declared exception is probably the way to go.

@kcooney kcooney and 1 other commented on an outdated diff Mar 16, 2013

src/main/java/org/junit/runner/JUnitCore.java
+import org.junit.runner.notification.RunNotifier;
+
+/**
+ * <code>JUnitCore</code> is a facade for running tests. It supports running JUnit 4 tests,
+ * JUnit 3.8.x tests, and mixtures. To run tests from the command line, run
+ * <code>java org.junit.runner.JUnitCore TestClass1 TestClass2 ...</code>.
+ * For one-shot test runs, use the static method {@link #runClasses(Class[])}.
+ * If you want to add special listeners,
+ * create an instance of {@link org.junit.runner.JUnitCore} first and use it to run the tests.
+ *
+ * @see org.junit.runner.Result
+ * @see org.junit.runner.notification.RunListener
+ * @see org.junit.runner.Request
+ * @since 4.0
+ */
+public class JUnitCore {
@kcooney

kcooney Mar 16, 2013

Member

@noel-yap Looks like somehow this file got reformatted (different line endings?). Could you clean it up so we can see the diffs?

@noel-yap

noel-yap Mar 18, 2013

Contributor

Done.

I had to set autocrlf for another project; I guess I should've set only for that one project.

@dsaff dsaff and 1 other commented on an outdated diff Mar 20, 2013

.../junit/experimental/categories/ExcludeCategories.java
@@ -0,0 +1,49 @@
+package org.junit.experimental.categories;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+
+import org.junit.runner.manipulation.Filter;
+
+import static org.junit.experimental.categories.Categories.CategoryFilter;
+
+/**
+ * {@link org.junit.runner.FilterFactory} to exclude categories.
+ *
+ * The {@link Filter} that is created will filter out tests that are categorized with any of the given categories.
@dsaff

dsaff Mar 20, 2013

Owner

Can we wrap the javadoc at 100 columns?

@noel-yap

noel-yap Mar 20, 2013

Contributor

Done.

@dsaff dsaff and 1 other commented on an outdated diff Mar 20, 2013

.../junit/experimental/categories/ExcludeCategories.java
+
+import static org.junit.experimental.categories.Categories.CategoryFilter;
+
+/**
+ * {@link org.junit.runner.FilterFactory} to exclude categories.
+ *
+ * The {@link Filter} that is created will filter out tests that are categorized with any of the given categories.
+ *
+ * Usage from command line:
+ * <code>
+ * --filter=org.junit.experimental.categories.ExcludeCategories=package.of.FirstCategory,package.of.SecondCategory
+ * </code>
+ *
+ * Usage from API:
+ * <code>
+ * new ExcludeCategories().createFilter(new Class[]{
@dsaff

dsaff Mar 20, 2013

Owner

Could easily change it to createFilter(Class...) to make it a little easier.

@noel-yap

noel-yap Mar 20, 2013

Contributor

Done.

@dsaff dsaff and 1 other commented on an outdated diff Mar 20, 2013

.../junit/experimental/categories/IncludeCategories.java
+ * The {@link Filter} that is created will filter out tests that are categorized with any of the given categories.
+ *
+ * Usage from command line:
+ * <code>
+ * --filter=org.junit.experimental.categories.IncludeCategories=package.of.FirstCategory,package.of.SecondCategory
+ * </code>
+ *
+ * Usage from API:
+ * <code>
+ * new IncludeCategories().createFilter(new Class[]{
+ * FirstCategory.class,
+ * SecondCategory.class
+ * });
+ * </code>
+ */
+public final class IncludeCategories extends CategoryFilterFactory {
@dsaff

dsaff Mar 20, 2013

Owner

Same thoughts from ExcludeCategories

@noel-yap

noel-yap Mar 20, 2013

Contributor

Done.

@dsaff dsaff and 1 other commented on an outdated diff Mar 20, 2013

...it/experimental/categories/CategoryFilterFactory.java
+
+import org.junit.internal.ClassUtil;
+import org.junit.runner.FilterFactory;
+import org.junit.runner.FilterFactoryParams;
+import org.junit.runner.manipulation.Filter;
+
+/**
+ * Implementation of FilterFactory for Category filtering.
+ */
+public abstract class CategoryFilterFactory extends FilterFactory {
+ @Override
+ public Filter createFilter(FilterFactoryParams params) throws FilterNotCreatedException {
+ try {
+ return createFilter(parseCategories(params.getArgs()));
+ } catch (ClassNotFoundException e) {
+ throw new FilterNotCreatedException(e.getMessage());
@dsaff

dsaff Mar 20, 2013

Owner

Can we pass e, and not just the message?

@noel-yap

noel-yap Mar 20, 2013

Contributor

Done.

@dsaff dsaff and 1 other commented on an outdated diff Mar 20, 2013

src/main/java/org/junit/runner/FilterFactory.java
@@ -0,0 +1,25 @@
+package org.junit.runner;
+
+import org.junit.runner.manipulation.Filter;
+
+/**
+ * Extend this class to create a factory that creates {@link Filter}.
+ */
+public abstract class FilterFactory {
@dsaff

dsaff Mar 20, 2013

Owner

I think this should be an interface

@noel-yap

noel-yap Mar 20, 2013

Contributor

Done.

@dsaff dsaff and 1 other commented on an outdated diff Mar 20, 2013

src/main/java/org/junit/runner/FilterFactoryFactory.java
@@ -0,0 +1,102 @@
+package org.junit.runner;
+
+import org.junit.internal.ClassUtil;
+import org.junit.runner.manipulation.Filter;
+
+import static org.junit.runner.FilterFactory.FilterNotCreatedException;
+
+/**
+ * Extend this class to create a factory that creates a factory that creates a {@link Filter}.
+ */
+class FilterFactoryFactory {
@dsaff

dsaff Mar 20, 2013

Owner

Although a clever name, there's no public method on this class that creates a FilterFactory

@noel-yap

noel-yap Mar 20, 2013

Contributor

I'll mull this one over and would appreciate suggestions.

@noel-yap

noel-yap Mar 21, 2013

Contributor

A coworker suggested renaming the current FilterFactory to FilterFactoryDelegate and FilterFactoryFactory to FilterFactory. What do you guys think?

@dsaff

dsaff Mar 22, 2013

Owner

FilterFactory is a good name for the public API that does what it does. Honestly, this whole class could just be inlined into JUnitCommandLineParser, right?

@kcooney kcooney and 1 other commented on an outdated diff Apr 9, 2013

...ain/java/org/junit/runner/JUnitCommandLineParser.java
+ } else {
+ parserErrors.add(new CommandLineParserError("JUnit knows nothing about the " + arg + " option"));
+ }
+ } else {
+ return copyArray(args, i, args.length);
+ }
+ } catch (FilterFactory.FilterNotCreatedException e) {
+ system.out().println("Could not find filter: " + e.getMessage());
+ parserErrors.add(e);
+ } catch(FilterFactories.FilterFactoryNotCreatedException e) {
+ system.out().println("Could not find filter factory: " + e.getMessage());
+ parserErrors.add(e);
+ }
+ }
+
+ return null;
@kcooney

kcooney Apr 9, 2013

Member

Could this return an empty array instead of null?

@noel-yap

noel-yap Apr 29, 2013

Contributor

Good catch. Done.

@kcooney kcooney and 1 other commented on an outdated diff Apr 9, 2013

...ain/java/org/junit/runner/JUnitCommandLineParser.java
+ private String[] copyArray(String[] args, int from, int to) {
+ ArrayList<String> result = new ArrayList<String>();
+
+ for (int j = from; j != to; ++j) {
+ result.add(args[j]);
+ }
+
+ return result.toArray(new String[]{});
+ }
+
+ void parseParameters(String[] args) {
+ for (String arg : args) {
+ try {
+ classes.add(Classes.getClass(arg));
+ } catch (ClassNotFoundException e) {
+ system.out().println("Could not find class: " + arg);
@kcooney

kcooney Apr 9, 2013

Member

why not do this?

parserErrors.add(new IllegalArgumentException("Could not find class [" + arg + "]", e));

(though I would suspect that the ClassNotFoundException has a useful message). It just seems odd to print out an error, when the caller is almost certain to take the exception(s) and present them to the user.

@noel-yap

noel-yap Apr 29, 2013

Contributor

Done.

Owner

noel-yap replied Apr 30, 2013

Done.

Might be good to throw a NPE if "args" is null

Owner

noel-yap replied Apr 30, 2013

Done.

@dsaff dsaff and 1 other commented on an outdated diff Apr 30, 2013

...it/experimental/categories/CategoryFilterFactory.java
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.junit.internal.Classes;
+import org.junit.runner.FilterFactory;
+import org.junit.runner.FilterFactoryParams;
+import org.junit.runner.manipulation.Filter;
+
+/**
+ * Implementation of FilterFactory for Category filtering.
+ */
+abstract class CategoryFilterFactory implements FilterFactory {
+ /**
+ * Creates a {@link org.junit.experimental.categories.Categories.CategoryFilter} given a
+ * ${FilterFactoryParams} argument.
@dsaff

dsaff Apr 30, 2013

Owner

What is ${} here?

@noel-yap

noel-yap Apr 30, 2013

Contributor

Fixed.

@dsaff dsaff and 1 other commented on an outdated diff Apr 30, 2013

.../org/junit/internal/runners/ErrorReportingRunner.java
@@ -53,7 +53,7 @@ public void run(RunNotifier notifier) {
private Description describeCause(Throwable child) {
return Description.createTestDescription(fTestClass,
- "initializationError");
+ "initializationError:" + child);
@dsaff

dsaff Apr 30, 2013

Owner

Spaces in descriptions can really throw off runners. The full stack trace will be in the details, anyway.

@noel-yap

noel-yap May 1, 2013

Contributor

Change reverted.

@dsaff dsaff and 1 other commented on an outdated diff Apr 30, 2013

src/main/java/org/junit/runner/FilterFactory.java
+/**
+ * Extend this class to create a factory that creates {@link Filter}.
+ */
+public interface FilterFactory {
+ /**
+ * Creates a {@link Filter} given a ${FilterFactoryParams} argument.
+ *
+ * @param params Parameters needed to create the {@link Filter}
+ * @throws FilterNotCreatedException
+ */
+ Filter createFilter(FilterFactoryParams params) throws FilterNotCreatedException;
+
+ /**
+ * Exception thrown if the {@link Filter} cannot be created.
+ */
+ public static class FilterNotCreatedException extends ClassNotFoundException {
@dsaff

dsaff Apr 30, 2013

Owner

I don't think extending ClassNotFoundException makes sense here.

@noel-yap

noel-yap May 1, 2013

Contributor

Changed to inherit from Exception.

@dsaff dsaff and 1 other commented on an outdated diff Apr 30, 2013

src/main/java/org/junit/runner/FilterFactories.java
+ return createFilterFactory(filterFactoryClass);
+ }
+
+ static FilterFactory createFilterFactory(Class<? extends FilterFactory> filterFactoryClass)
+ throws FilterFactoryNotCreatedException {
+ try {
+ return filterFactoryClass.getConstructor().newInstance();
+ } catch (Exception e) {
+ throw new FilterFactoryNotCreatedException(e);
+ }
+ }
+
+ /**
+ * Exception thrown if the {@link FilterFactory} cannot be created.
+ */
+ public static class FilterFactoryNotCreatedException extends FilterNotCreatedException {
@dsaff

dsaff Apr 30, 2013

Owner

How would a client of this class use the distinction between this class and its superclass?

@noel-yap

noel-yap May 1, 2013

Contributor

The difference is where the error is. With the Category filter, FilterFactoryNotCreatedException is thrown if the user provides a bad name (eg via typo) for the filter and FilterNotCreatedException is thrown if the user provides a bad name for the category value. If this differentiation is deemed not worthwhile, the two exceptions can be collapsed.

@dsaff

dsaff May 1, 2013

Owner

I think a single exception class, with clear error messages, makes more sense, since I can't think of a situation where either error scenario would produce any behavior beyond a swift exit with feedback to the user.

@noel-yap

noel-yap May 1, 2013

Contributor

Done.

@dsaff dsaff and 1 other commented on an outdated diff Apr 30, 2013

src/main/java/org/junit/runner/FilterFactoryParams.java
@@ -0,0 +1,36 @@
+package org.junit.runner;
+
+/**
@dsaff

dsaff Apr 30, 2013

Owner

We'd be better off without any of the javadoc in this class. Better would be a brief class-level comment about why this exists (especially because it's taken @kcooney several times before I got my head around why this is a good idea).

@noel-yap

noel-yap May 1, 2013

Contributor

@kcooney , preference for the class-level comment?

Member

kcooney commented May 1, 2013

@noel-yap Looks good. I think @dsaff can take it from here.

Contributor

noel-yap commented May 2, 2013

I've removed the Javadoc from FilterFactoryParams. @kcooney, can you provide the appropriate comments when you start working on the file?

Member

kcooney commented May 4, 2013

@noel-yap I can update comments on FilterFactoryParams after your submit.
(sorry about the delay; the GitHub comment UI makes it really hard to have conversations about anything but the latest commit in a pull request, so threads on older commits tend to die)

Contributor

noel-yap commented May 4, 2013

@kcooney , I understand; the UI is lacking in the way you describe. What's the next step? AFAIK, someone on your end just needs to merge my branch into junit/master. Is there anything else I need to do on my end (eg remerge)?

Member

kcooney commented May 4, 2013

@noel-yap I don't have admin privileges, so I can't pull in your change into junit-team/junit. I also can't tell if there are conflicts from main (last commit on main was April 8th, so you're probably good on that one).

@dsaff Is there anything else you want Noel to do? I'm happy with the change. You might want to do a non-fast-forward merge, but that's up to you

Owner

dsaff commented May 5, 2013

There's no git conflicts. I'll continue reviewing now, but may not finish until Monday.

@dsaff dsaff and 1 other commented on an outdated diff May 5, 2013

...it/experimental/categories/CategoryFilterFactory.java
+ *
+ * @param params Parameters needed to create the {@link Filter}
+ * @throws FilterNotCreatedException
+ */
+ @Override
+ public Filter createFilter(FilterFactoryParams params) throws FilterNotCreatedException {
+ try {
+ return createFilter(parseCategories(params.getArgs()));
+ } catch (ClassNotFoundException e) {
+ throw new FilterNotCreatedException(e);
+ }
+ }
+
+ /**
+ * Creates a {@link org.junit.experimental.categories.Categories.CategoryFilter} given an array of classes used as
+ * {@link Category} values.
@dsaff

dsaff May 5, 2013

Owner

Strictly speaking, it takes Classes, which may be values of @Category annotations. (I think the wording used on ExcludeCategories is good.)

@noel-yap

noel-yap May 6, 2013

Contributor

Done.

@dsaff dsaff and 1 other commented on an outdated diff May 5, 2013

.../org/junit/internal/runners/ErrorReportingRunner.java
@@ -52,8 +52,7 @@ public void run(RunNotifier notifier) {
}
private Description describeCause(Throwable child) {
- return Description.createTestDescription(fTestClass,
- "initializationError");
+ return Description.createTestDescription(fTestClass, "initializationError:");
@dsaff

dsaff May 5, 2013

Owner

Let's just leave this class unchanged.

@noel-yap

noel-yap May 6, 2013

Contributor

Done.

@dsaff dsaff and 1 other commented on an outdated diff May 5, 2013

src/main/java/org/junit/runner/FilterFactories.java
+ *
+ * A filter specification is of the form "package.of.FilterFactory=args-to-filter-factory" or
+ * "package.of.FilterFactory".
+ *
+ * @param filterSpec The filter specification
+ * @throws org.junit.runner.FilterFactory.FilterNotCreatedException
+ */
+ public static Filter createFilterFromFilterSpec(Description description, String filterSpec)
+ throws FilterFactory.FilterNotCreatedException {
+
+ if (filterSpec.contains("=")) {
+ String[] tuple = filterSpec.split("=", 2);
+
+ return createFilter(tuple[0], new FilterFactoryParams(tuple[1]));
+ } else {
+ return createFilter(filterSpec, new FilterFactoryParams());
@dsaff

dsaff May 5, 2013

Owner

I think I'd rather explicitly pass in "" here, rather than have a default construction for FilterFactoryParams.

@noel-yap

noel-yap May 6, 2013

Contributor

Done.

@dsaff dsaff commented on the diff May 5, 2013

src/main/java/org/junit/runner/FilterFactoryParams.java
+public final class FilterFactoryParams {
+ private final String args;
+
+ public FilterFactoryParams() {
+ this("");
+ }
+
+ public FilterFactoryParams(String args) {
+ if (args == null) {
+ throw new NullPointerException();
+ }
+
+ this.args = args;
+ }
+
+ public String getArgs() {
@dsaff

dsaff May 5, 2013

Owner

Do you think this makes more sense as getArgs or getArg?

@noel-yap

noel-yap May 6, 2013

Contributor

I guess it depends upon how one looks at it. The field can contain zero or more arguments depending upon how its user decides to parse it. @kcooney , opinions?

@kcooney

kcooney May 8, 2013

Member

@noel-yap @dsaff I think getArgs makes sense

Contributor

noel-yap commented May 20, 2013

@dsaff , has this been pulled into junit-team/junit?

Owner

dsaff commented May 21, 2013

@noel-yap,

No it hasn't. Sorry. Github is frustrating sometimes--I never got notification that kcooney answered your question(s) 14 days ago. Looking through again...

@dsaff dsaff and 1 other commented on an outdated diff May 21, 2013

.../org/junit/internal/runners/ErrorReportingRunner.java
@@ -53,7 +53,7 @@ public void run(RunNotifier notifier) {
private Description describeCause(Throwable child) {
return Description.createTestDescription(fTestClass,
- "initializationError");
+ "initializationError:");
@dsaff

dsaff May 21, 2013

Owner

No colon, please

@noel-yap

noel-yap May 27, 2013

Contributor

Done.

@dsaff dsaff and 1 other commented on an outdated diff May 21, 2013

src/main/java/org/junit/runner/FilterFactory.java
@@ -0,0 +1,25 @@
+package org.junit.runner;
+
+import org.junit.runner.manipulation.Filter;
+
+/**
+ * Extend this class to create a factory that creates {@link Filter}.
+ */
+public interface FilterFactory {
+ /**
+ * Creates a {@link Filter} given a ${FilterFactoryParams} argument.

@dsaff dsaff and 1 other commented on an outdated diff May 21, 2013

...ain/java/org/junit/runner/JUnitCommandLineParser.java
+import static org.junit.runner.Description.createSuiteDescription;
+
+class JUnitCommandLineParser {
+ private final JUnitSystem system;
+
+ private Filter filter = Filter.ALL;
+ private List<Class<?>> classes = new ArrayList<Class<?>>();
+ private List<Throwable> parserErrors = new ArrayList<Throwable>();
+
+ /**
+ * Constructs a {@link JUnitCommandLineParser}.
+ *
+ * @param system {@link JUnitSystem} to be used.
+ */
+ public JUnitCommandLineParser(JUnitSystem system) {
+ this.system = system;
@dsaff

dsaff May 21, 2013

Owner

I'd rather drop the system field entirely--having a secondary out-of-band channel for parse errors is of dubious added value.

If that field is dropped, then this class could become a publicly-immutable JUnitCommandLineParseResult, with the only public creation being a static method like JUnitCommandLineParseResult.parse(args). And then we could just make getFilter, getClasses, etc, public without caveat.

@noel-yap

noel-yap May 27, 2013

Contributor

Done.

Owner

dsaff commented May 29, 2013

@noel-yap, thanks for the updates. It may be a day or two before I can review again.

Contributor

noel-yap commented Jun 4, 2013

@dsaff , ping.

Owner

dsaff commented Jun 5, 2013

Thanks. Merging. Can you add a section to the release notes at https://github.com/junit-team/junit/wiki/4.12-release-notes? Thanks.

dsaff added a commit that referenced this pull request Jun 5, 2013

@dsaff dsaff merged commit aeaecd2 into junit-team:master Jun 5, 2013

Owner

marcphilipp commented Jun 5, 2013

The Jenkins build failed after merging this pull request:

[ERROR] https://junit.ci.cloudbees.com/job/JUnit/ws/src/main/java/org/junit/experimental/categories/CategoryFilterFactory.java:[22,5] method does not override a method from its superclass

See https://junit.ci.cloudbees.com/job/JUnit/46/console for details.

Contributor

noel-yap commented Jun 5, 2013

I'll get on it ASAP.
On Jun 5, 2013 6:22 AM, "Marc Philipp" notifications@github.com wrote:

The Jenkins build failed after merging this pull request:

[ERROR]
https://junit.ci.cloudbees.com/job/JUnit/ws/src/main/java/org/junit/experimental/categories/CategoryFilterFactory.java:[22,5]
method does not override a method from its superclass

See https://junit.ci.cloudbees.com/job/JUnit/46/console for details.


Reply to this email directly or view it on GitHubhttps://github.com/junit-team/junit/pull/647#issuecomment-18974939
.

Contributor

noel-yap commented Jun 5, 2013

@marcphilipp and @dsaff , I am unable to reproduce this error. Also, CategoryFilterFactory implements FilterFactory which is part of the pull request so I'm not sure how this error can be occurring.

I looked through the workspace in https://junit.ci.cloudbees.com/job/JUnit/ and both https://junit.ci.cloudbees.com/job/JUnit/ws/src/main/java/org/junit/experimental/categories/CategoryFilterFactory.java/*view*/ and https://junit.ci.cloudbees.com/job/JUnit/ws/src/main/java/org/junit/runner/FilterFactory.java/*view*/ look good. Any ideas what may be causing this error?

Is it complaining about the @override annotation?

Owner

marcphilipp commented Jun 5, 2013

We're still using JDK5 in order to stay useable by "old" projects. In JDK5 you cannot annotate a method that implements an interface method with @Override.

Does it compile in Eclipse?

Contributor

noel-yap commented Jun 5, 2013

I use IntelliJ and it compiles fine there as well as from the command line.
I'll take out the annotation ASAP.
On Jun 5, 2013 11:41 AM, "Marc Philipp" notifications@github.com wrote:

We're still using JDK5 in order to stay useable by "old" projects. In JDK5
you cannot annotate a method that implements an interface method with
@override.

Does it compile in Eclipse?


Reply to this email directly or view it on GitHubhttps://github.com/junit-team/junit/pull/647#issuecomment-18998756
.

Owner

dsaff commented Jun 5, 2013

Thanks, @noel-yap. I'm going on vacation in a couple hours--any chance of a pull request before then? Thanks!

Contributor

noel-yap commented Jun 5, 2013

I just pushed an update.

On Wed, Jun 5, 2013 at 12:19 PM, David Saff notifications@github.comwrote:

Thanks, @noel-yap https://github.com/noel-yap. I'm going on vacation in
a couple hours--any chance of a pull request before then? Thanks!


Reply to this email directly or view it on GitHubhttps://github.com/junit-team/junit/pull/647#issuecomment-19001831
.

Owner

marcphilipp commented Jun 5, 2013

Your commit doesn't show up in this pull request, probably because it has already been merged. I think you have to create a new pull request.

Contributor

noel-yap commented Jun 5, 2013

Done.

On Wed, Jun 5, 2013 at 1:04 PM, Marc Philipp notifications@github.comwrote:

Your commit doesn't show up in this pull request, probably because it has
already been merged. I think you have to create a new pull request.


Reply to this email directly or view it on GitHubhttps://github.com/junit-team/junit/pull/647#issuecomment-19004630
.

Owner

marcphilipp commented Jun 5, 2013

Unfortunately, there are a couple more of those in the test source folder:

[ERROR] /scratch/jenkins/workspace/JUnit/src/test/java/org/junit/experimental/categories/CategoryFilterFactoryTest.java:[45,25] createFilter(java.lang.Class[]) in org.junit.experimental.categories.CategoryFilterFactoryTest.CategoryFilterFactoryStub cannot override createFilter(java.lang.Class...) in org.junit.experimental.categories.CategoryFilterFactory; overriding method is missing '...'
[ERROR] /scratch/jenkins/workspace/JUnit/src/test/java/org/junit/experimental/categories/CategoryFilterFactoryTest.java:[45,25] createFilter(java.lang.Class[]) in org.junit.experimental.categories.CategoryFilterFactoryTest.CategoryFilterFactoryStub cannot override createFilter(java.lang.Class...) in org.junit.experimental.categories.CategoryFilterFactory; overriding method is missing '...'
[ERROR] /scratch/jenkins/workspace/JUnit/src/test/java/org/junit/runner/FilterFactoriesTest.java:[68,9] method does not override a method from its superclass
[ERROR] /scratch/jenkins/workspace/JUnit/src/test/java/org/junit/runner/FilterFactoriesTest.java:[75,9] method does not override a method from its superclass
[ERROR] /scratch/jenkins/workspace/JUnit/src/test/java/org/junit/runner/JUnitCommandLineParseResultTest.java:[139,9] method does not override a method from its superclass

Contributor

noel-yap commented Jun 5, 2013

I think the I caught the rest in the latest pull request. Let me know if
there's still brokenness.

Thanks,
Noel
On Jun 5, 2013 1:44 PM, "Marc Philipp" notifications@github.com wrote:

Unfortunately, there are a couple more of those in the test source folder:

[ERROR]
/scratch/jenkins/workspace/JUnit/src/test/java/org/junit/experimental/categories/CategoryFilterFactoryTest.java:[45,25]
createFilter(java.lang.Class[]) in org.junit.experimental.categories.CategoryFilterFactoryTest.CategoryFilterFactoryStub cannot override createFilter(java.lang.Class...) in
org.junit.experimental.categories.CategoryFilterFactory; overriding method
is missing '...'
[ERROR]
/scratch/jenkins/workspace/JUnit/src/test/java/org/junit/experimental/categories/CategoryFilterFactoryTest.java:[45,25]
createFilter(java.lang.Class[]) in org.junit.experimental.categories.CategoryFilterFactoryTest.CategoryFilterFactoryStub cannot override createFilter(java.lang.Class...) in
org.junit.experimental.categories.CategoryFilterFactory; overriding method
is missing '...'
[ERROR]
/scratch/jenkins/workspace/JUnit/src/test/java/org/junit/runner/FilterFactoriesTest.java:[68,9]
method does not override a method from its superclass
[ERROR]
/scratch/jenkins/workspace/JUnit/src/test/java/org/junit/runner/FilterFactoriesTest.java:[75,9]
method does not override a method from its superclass
[ERROR]
/scratch/jenkins/workspace/JUnit/src/test/java/org/junit/runner/JUnitCommandLineParseResultTest.java:[139,9]
method does not override a method from its superclass


Reply to this email directly or view it on GitHubhttps://github.com/junit-team/junit/pull/647#issuecomment-19007115
.

Member

kcooney commented Jun 6, 2013

You could verify locally by running the tests in ant
On Jun 5, 2013 4:04 PM, "Noel Yap" notifications@github.com wrote:

I think the I caught the rest in the latest pull request. Let me know if
there's still brokenness.

Thanks,
Noel
On Jun 5, 2013 1:44 PM, "Marc Philipp" notifications@github.com wrote:

Unfortunately, there are a couple more of those in the test source
folder:

[ERROR]

/scratch/jenkins/workspace/JUnit/src/test/java/org/junit/experimental/categories/CategoryFilterFactoryTest.java:[45,25]

createFilter(java.lang.Class<?>[]) in

org.junit.experimental.categories.CategoryFilterFactoryTest.CategoryFilterFactoryStub

cannot override createFilter(java.lang.Class<?>...) in
org.junit.experimental.categories.CategoryFilterFactory; overriding
method
is missing '...'
[ERROR]

/scratch/jenkins/workspace/JUnit/src/test/java/org/junit/experimental/categories/CategoryFilterFactoryTest.java:[45,25]

createFilter(java.lang.Class<?>[]) in

org.junit.experimental.categories.CategoryFilterFactoryTest.CategoryFilterFactoryStub

cannot override createFilter(java.lang.Class<?>...) in
org.junit.experimental.categories.CategoryFilterFactory; overriding
method
is missing '...'
[ERROR]

/scratch/jenkins/workspace/JUnit/src/test/java/org/junit/runner/FilterFactoriesTest.java:[68,9]

method does not override a method from its superclass
[ERROR]

/scratch/jenkins/workspace/JUnit/src/test/java/org/junit/runner/FilterFactoriesTest.java:[75,9]

method does not override a method from its superclass
[ERROR]

/scratch/jenkins/workspace/JUnit/src/test/java/org/junit/runner/JUnitCommandLineParseResultTest.java:[139,9]

method does not override a method from its superclass


Reply to this email directly or view it on GitHub<
https://github.com/junit-team/junit/pull/647#issuecomment-19007115>
.


Reply to this email directly or view it on GitHubhttps://github.com/junit-team/junit/pull/647#issuecomment-19015104
.

Owner

marcphilipp commented Jun 6, 2013

It did not compile in Eclipse either. Now it does! I will merge #691.

@noel-yap Thanks! For the future, please check whether you can change your compiler settings in IntelliJ or use JDK5 to compile.

Owner

dsaff commented Nov 18, 2014

I don't think this ever got added to the 4.12 release notes. It's a major feature in 4.12 (IMHO). Would anyone on this thread like to add it, or should I?

Owner

marcphilipp commented Nov 22, 2014

@dsaff Do you have time to add this pull request to the release notes?

Owner

dsaff commented Nov 24, 2014

Sure, I can do it in the next day or so.

Owner

marcphilipp commented Nov 29, 2014

@dsaff Any news on this? I would not like to release 4.12 without this being documented.

Owner

dsaff commented Dec 3, 2014

OK, sorry for the delay. Taking a look now.

@stefanbirkner stefanbirkner added this to the 4.12 milestone Dec 4, 2014

Owner

marcphilipp commented Dec 4, 2014

LGTM, thanks!

@claycephas claycephas referenced this pull request in Microsoft/vscode-java-debug Nov 6, 2017

Open

[Junit] Junit support for java debug #76

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