Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enables class-array as parameter to @IncludeCategory and @ExludeCategory #565

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 57 additions & 18 deletions src/main/java/org/junit/experimental/categories/Categories.java
Original file line number Diff line number Diff line change
Expand Up @@ -64,34 +64,54 @@ public class Categories extends Suite {
// someday enable a better new implementation.
// https://github.com/KentBeck/junit/issues/issue/172

/**
* {@link IncludeCategory} determines which Tests are ran by {@link Categories}.
* <p/>
* If specified all Tests annotated with classes specified in the value of this annotation will be ran by
* {@link Categories} as long they are not specified using {@link ExcludeCategory}
*/
@Retention(RetentionPolicy.RUNTIME)
public @interface IncludeCategory {
public Class<?> value();
public Class<?>[] value();
}

/**
* {@link ExcludeCategory} determines which Tests are skipped by {@link Categories}.
* <p/>
* If specified all Tests annotated with classes specified in the value of this annotation will be skipped by
* {@link Categories} regardless of being mentioned in {@link IncludeCategory}
*/
@Retention(RetentionPolicy.RUNTIME)
public @interface ExcludeCategory {
public Class<?> value();
public Class<?>[] value();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For compatibility, we need to leave the old constructor there to delegate to the new constructor.

}

public static class CategoryFilter extends Filter {

public static CategoryFilter include(Class<?> categoryType) {
return new CategoryFilter(new Class<?>[]{categoryType}, null);
}

public static CategoryFilter include(Class<?>[] categoryType) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this Class<?>...

return new CategoryFilter(categoryType, null);
}

private final Class<?> fIncluded;
private final Class<?>[] includedCategories;

private final Class<?> fExcluded;
private final Class<?>[] excludedCategories;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JUnit style is for fields to have an "f" prefix.


public CategoryFilter(Class<?> includedCategory,
Class<?> excludedCategory) {
fIncluded = includedCategory;
fExcluded = excludedCategory;
public CategoryFilter(Class<?>[] includedCategories,
Class<?>[] excludedCategories) {
this.includedCategories = includedCategories;
this.excludedCategories = excludedCategories;
}

@Override
public String describe() {
return "category " + fIncluded;
Integer numIncludedCategories = includedCategories == null ? 0 : includedCategories.length;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just list the included and excluded categories? The main time this is used is when the framework has to inform the user that, unfortunately, the filter excludes all of their tests. In that situation, more information is definitely better than less.

Integer numExcludedCategories = excludedCategories == null ? 0 : excludedCategories.length;
return "category. contains: " + numIncludedCategories + " included categories and "
+ numExcludedCategories + " excluded categories.";
}

@Override
Expand All @@ -107,18 +127,37 @@ public boolean shouldRun(Description description) {
return false;
}

/**
* Checks if the given array of classes (eg includedCategories) contains a given class
* This is used to determine if a specified class in a {@link Category} annotation is represented in the
* class-array specified in a {@link IncludeCategory} or {@link ExcludeCategory} annotation of a suite ran with
* {@link Categories}
*
* @param categories array of categories (specified value of {@link IncludeCategory} or {@link ExcludeCategory})
* @param category defined category (one of the classes specified in {@link Category})
* @return true if the given category class is assignable from any of the given classes in categories
*/
private boolean containsCategory(Class<?>[] categories, Class<?> category){
for(Class<?> cat : categories){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you get the spacing to match the surrounding style? Spaces before braces and between "for (" and "if ("

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, JUnit style is to name loop variables "each", that is: for (Class<?> each : categories) {

if(cat.isAssignableFrom(category)){
return true;
}
}
return false;
}

private boolean hasCorrectCategoryAnnotation(Description description) {
List<Class<?>> categories = categories(description);
if (categories.isEmpty()) {
return fIncluded == null;
return includedCategories == null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One challenge is that a user might pass an empty array, rather than null, to either includedCategories or excludedCategories. I think the easiest way to handle this is to replace nulls with empty arrays in the CategoryFilter constructor, and then we can just test .length == 0 anywhere we currently test for null, and we don't have to worry about accidental NPEs on loops. Make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had implemted that null-safe already. I will re-introduce that.

}
for (Class<?> each : categories) {
if (fExcluded != null && fExcluded.isAssignableFrom(each)) {
for (Class<?> category : categories) {
if (excludedCategories != null && containsCategory(excludedCategories, category)) {
return false;
}
}
for (Class<?> each : categories) {
if (fIncluded == null || fIncluded.isAssignableFrom(each)) {
for (Class<?> category : categories) {
if (includedCategories == null || containsCategory(includedCategories, category)) {
return true;
}
}
Expand Down Expand Up @@ -156,20 +195,20 @@ public Categories(Class<?> klass, RunnerBuilder builder)
throws InitializationError {
super(klass, builder);
try {
filter(new CategoryFilter(getIncludedCategory(klass),
getExcludedCategory(klass)));
filter(new CategoryFilter(getIncludedCategories(klass),
getExcludedCategories(klass)));
} catch (NoTestsRemainException e) {
throw new InitializationError(e);
}
assertNoCategorizedDescendentsOfUncategorizeableParents(getDescription());
}

private Class<?> getIncludedCategory(Class<?> klass) {
private Class<?>[] getIncludedCategories(Class<?> klass) {
IncludeCategory annotation = klass.getAnnotation(IncludeCategory.class);
return annotation == null ? null : annotation.value();
}

private Class<?> getExcludedCategory(Class<?> klass) {
private Class<?>[] getExcludedCategories(Class<?> klass) {
ExcludeCategory annotation = klass.getAnnotation(ExcludeCategory.class);
return annotation == null ? null : annotation.value();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ public void ifNoTestsToRunUseErrorRunner() {
@Test
public void describeACategoryFilter() {
CategoryFilter filter = CategoryFilter.include(SlowTests.class);
assertEquals("category " + SlowTests.class, filter.describe());
assertEquals("category. contains: 1 included categories and 0 excluded categories.", filter.describe());
}

public static class OneThatIsBothFastAndSlow {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
package org.junit.tests.experimental.categories;

import org.junit.Test;
import org.junit.experimental.categories.Categories;
import org.junit.experimental.categories.Category;
import org.junit.runner.JUnitCore;
import org.junit.runner.Result;
import org.junit.runner.RunWith;
import org.junit.runners.Suite;

import static junit.framework.Assert.assertEquals;

/**
* Author: Henning Gross
* Date: 04.12.12
*/
public class MultipleClassesInIncludeCategoryAndExludeCategoryTest {

// category markers
public interface IntegrationTests {
}

public interface SlowTests {
}

public static class A {
@Category(IntegrationTests.class)
@Test
public void a() {
}

@Category(SlowTests.class)
@Test
public void b() {
}

@Test
public void c() {
}
}

@RunWith(Categories.class)
@Categories.IncludeCategory({SlowTests.class, IntegrationTests.class})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we still use @IncludeCategory(SlowTests.class)

I actually forget whether Java will auto-create the 1-element array in this case. If it does, then can you add a test of that behavior?

If that doesn't work, the current change will break all existing category suites, so we'll need to introduce a new syntax. @IncludeCategories seems an obvious choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course it gets initialized correctly. There are already tests for @IncludeCategory(Single.class) and they still run fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good to know.

@Suite.SuiteClasses({A.class})
public static class IncludeCategorySuite {
}

@Test
public void testMultipleClassesInIncludeCategory() {
// a and b will be ran
Result result = JUnitCore.runClasses(IncludeCategorySuite.class);
assertEquals(2, result.getRunCount());
}

@RunWith(Categories.class)
@Categories.ExcludeCategory({SlowTests.class, IntegrationTests.class})
@Suite.SuiteClasses({A.class})
public static class ExcludeCategorySuite {
}

@Test
public void testMultipleClassesInExcludeCategory() {
// only c will be ran (not annotated, due to no @IncludeCategory specified)
Result result = JUnitCore.runClasses(ExcludeCategorySuite.class);
assertEquals(1, result.getRunCount());
}

@RunWith(Categories.class)
@Categories.IncludeCategory(SlowTests.class)
@Categories.ExcludeCategory({SlowTests.class})
@Suite.SuiteClasses({A.class})
public static class OverrideIncludeCategorySuite {
}

@Test
public void testOverrideCategory() {
// only a will be ran (IntegrationTest)
Result result = JUnitCore.runClasses(ExcludeCategorySuite.class);
assertEquals(1, result.getRunCount());
}
}