Skip to content

Commit

Permalink
Review:
Browse files Browse the repository at this point in the history 
* limit visibility of implementation details
  -> we don't want users of ArchUnit to be able to access the class `LocationPatternImportOption`, but it was public
* use composition instead of inheritance to reuse the patterns
  -> inheritance to share some common logic is IMHO a bad pattern. On this scale it probably does not matter, but ArchUnit usually follows composition over inheritance for such use cases (i.e. when the "is a" relationship is unimportant and it is more a "I want to share some code" intention)
* use predicates instead of Set<Pattern>
  -> I think is easier to understand Predicate<Location> TEST_LOCATION and not(TEST_LOCATION) instead of `return exclude ? false : true` ... `return exclude ? true : false`; in general the boolean `exclude` is a little confusing to me in this context; I've measured performance and found no noticeable difference on ~40000 locations when I compared the predicate based approach to the iteration over the pattern set
* add test
  -> I've added tests analogously to DoNotIncludeTests to ensure this does not break in the future
* rename `OnlyTests` -> `OnlyIncludeTests`, as it is more consistent with the other `ImportOptions` which all contain "include"
* add `OnlyIncludeTests` to `Predefined`, since there are predefined constants for all the other `ImportOptions`

Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
  • Loading branch information
@codecholeric
codecholeric committed Jan 28, 2021
1 parent 6da9e43 commit fa0b8ca
Show file tree
Hide file tree
Showing 2 changed files with 123 additions and 62 deletions.
87 changes: 45 additions & 42 deletions archunit/src/main/java/com/tngtech/archunit/core/importer/ImportOption.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2014-2020 TNG Technology Consulting GmbH
* Copyright 2014-2021 TNG Technology Consulting GmbH
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -15,13 +15,15 @@
*/
package com.tngtech.archunit.core.importer;

import java.util.Set;
import java.util.regex.Pattern;

import com.google.common.collect.ImmutableSet;
import com.google.common.base.Predicate;
import com.google.common.base.Predicates;
import com.tngtech.archunit.PublicAPI;

import static com.google.common.base.Predicates.not;
import static com.tngtech.archunit.PublicAPI.Usage.INHERITANCE;
import static com.tngtech.archunit.core.importer.ImportOption.Predefined.TEST_LOCATION;

/**
* Will be evaluated for every class location, to determine if the class should be imported.<br><br>
Expand All @@ -47,6 +49,14 @@ public boolean includes(Location location) {
return doNotIncludeTests.includes(location);
}
},
ONLY_INCLUDE_TESTS {
private final OnlyIncludeTests onlyIncludeTests = new OnlyIncludeTests();

@Override
public boolean includes(Location location) {
return onlyIncludeTests.includes(location);
}
},
DO_NOT_INCLUDE_JARS {
private final DoNotIncludeJars doNotIncludeJars = new DoNotIncludeJars();

Expand All @@ -65,30 +75,52 @@ public boolean includes(Location location) {
public boolean includes(Location location) {
return doNotIncludeArchives.includes(location);
}
};

static final PatternPredicate MAVEN_TEST_PATTERN = new PatternPredicate(".*/target/test-classes/.*");
static final PatternPredicate GRADLE_TEST_PATTERN = new PatternPredicate(".*/build/classes/([^/]+/)?test/.*");
static final PatternPredicate INTELLIJ_TEST_PATTERN = new PatternPredicate(".*/out/test/classes/.*");
static final Predicate<Location> TEST_LOCATION = Predicates.or(MAVEN_TEST_PATTERN, GRADLE_TEST_PATTERN, INTELLIJ_TEST_PATTERN);

private static class PatternPredicate implements Predicate<Location> {
private final Pattern pattern;

PatternPredicate(String pattern) {
this.pattern = Pattern.compile(pattern);
}

@Override
@SuppressWarnings("ConstantConditions") // ArchUnit never uses null as a valid parameter
public boolean apply(Location input) {
return input.matches(pattern);
}
}
}

/**
* Best effort {@link ImportOption} to check rules only on main classes.<br>
* NOTE: This excludes all class files residing in some directory
* ../target/test-classes/.., ../build/classes/test/.. or ../build/classes/someLang/test/.. (Maven/Gradle standard).
* Thus it is just a best guess, how tests can be identified,
* in other environments, it might be necessary, to implement the correct {@link ImportOption} yourself.
*/
final class DoNotIncludeTests extends LocationPatternImportOption {
final class DoNotIncludeTests implements ImportOption {
private static final Predicate<Location> NO_TEST_LOCATION = not(TEST_LOCATION);

public DoNotIncludeTests() {
super(TESTS_LOCATIONS, true);
@Override
public boolean includes(Location location) {
return NO_TEST_LOCATION.apply(location);
}
}

/**
* Import option to define rules only for Tests.
* See {@link DoNotIncludeTests} for limitations if tests identification.
* Best effort {@link ImportOption} to check rules only on test classes.<br>
* See {@link DoNotIncludeTests} for limitations of test class identification.
*/
final class OnlyTests extends LocationPatternImportOption {

public OnlyTests() {
super(TESTS_LOCATIONS, false);
final class OnlyIncludeTests implements ImportOption {
@Override
public boolean includes(Location location) {
return TEST_LOCATION.apply(location);
}
}

Expand All @@ -105,33 +137,4 @@ public boolean includes(Location location) {
return !location.isArchive();
}
}

/**
* Import option for excluding/including locations matching specific regex patterns.
*/
abstract class LocationPatternImportOption implements ImportOption {
protected static final Pattern MAVEN_PATTERN = Pattern.compile(".*/target/test-classes/.*");
protected static final Pattern GRADLE_PATTERN = Pattern.compile(".*/build/classes/([^/]+/)?test/.*");
protected static final Pattern INTELLIJ_PATTERN = Pattern.compile(".*/out/test/classes/.*");
protected static final Set<Pattern> TESTS_LOCATIONS =
ImmutableSet.of(MAVEN_PATTERN, GRADLE_PATTERN, INTELLIJ_PATTERN);

private final Set<Pattern> patterns;
private final boolean exclude;

public LocationPatternImportOption(Set<Pattern> excludedPatterns, boolean exclude) {
this.patterns = excludedPatterns;
this.exclude = exclude;
}

@Override
public boolean includes(Location location) {
for (Pattern pattern : patterns) {
if (location.matches(pattern)) {
return exclude ? false : true;
}
}
return exclude ? true : false;
}
}
}
98 changes: 78 additions & 20 deletions archunit/src/test/java/com/tngtech/archunit/core/importer/ImportOptionsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,14 @@
import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.util.ArrayList;
import java.util.List;

import com.google.common.collect.ImmutableList;
import com.tngtech.archunit.core.importer.ImportOption.DoNotIncludeArchives;
import com.tngtech.archunit.core.importer.ImportOption.DoNotIncludeJars;
import com.tngtech.archunit.core.importer.ImportOption.DoNotIncludeTests;
import com.tngtech.archunit.core.importer.ImportOption.OnlyIncludeTests;
import com.tngtech.java.junit.dataprovider.DataProvider;
import com.tngtech.java.junit.dataprovider.DataProviderRunner;
import com.tngtech.java.junit.dataprovider.UseDataProvider;
Expand All @@ -15,14 +19,17 @@
import org.junit.rules.TemporaryFolder;
import org.junit.runner.RunWith;

import static com.google.common.collect.ImmutableList.copyOf;
import static com.google.common.collect.Iterables.concat;
import static com.google.common.collect.Iterables.getLast;
import static com.tngtech.archunit.core.importer.ImportOption.Predefined.DO_NOT_INCLUDE_ARCHIVES;
import static com.tngtech.archunit.core.importer.ImportOption.Predefined.DO_NOT_INCLUDE_JARS;
import static com.tngtech.archunit.core.importer.ImportOption.Predefined.DO_NOT_INCLUDE_TESTS;
import static com.tngtech.archunit.core.importer.ImportOption.Predefined.ONLY_INCLUDE_TESTS;
import static com.tngtech.java.junit.dataprovider.DataProviders.$;
import static com.tngtech.java.junit.dataprovider.DataProviders.$$;
import static com.tngtech.java.junit.dataprovider.DataProviders.crossProduct;
import static com.tngtech.java.junit.dataprovider.DataProviders.testForEach;
import static java.util.Arrays.asList;
import static org.assertj.core.api.Assertions.assertThat;

@RunWith(DataProviderRunner.class)
Expand All @@ -46,33 +53,62 @@ public void excludes_test_class(ImportOption doNotIncludeTests) {
}

@DataProvider
public static Object[][] folders() {
return crossProduct(do_not_include_tests(), $$(
public static Object[][] only_include_tests() {
return testForEach(new OnlyIncludeTests(), ONLY_INCLUDE_TESTS);
}

@Test
@UseDataProvider("only_include_tests")
public void excludes_main_class(ImportOption onlyIncludeTests) {
assertThat(onlyIncludeTests.includes(locationOf(OnlyIncludeTests.class)))
.as("includes production location").isFalse();

assertThat(onlyIncludeTests.includes(locationOf(getClass())))
.as("includes test location").isTrue();
}

private static List<FolderPattern> getFolderPatterns() {
return ImmutableList.of(
// Gradle
$(new String[]{"build", "classes", "test"}, false),
$(new String[]{"build", "classes", "java", "test"}, false),
$(new String[]{"build", "classes", "otherlang", "test"}, false),
$(new String[]{"build", "test-classes"}, true),
$(new String[]{"build", "classes", "main"}, true),
$(new String[]{"build", "classes", "java", "main"}, true),
$(new String[]{"build", "classes", "java", "main", "my", "test"}, true),
new FolderPattern("build", "classes", "test").expectTestFolder(),
new FolderPattern("build", "classes", "java", "test").expectTestFolder(),
new FolderPattern("build", "classes", "otherlang", "test").expectTestFolder(),
new FolderPattern("build", "test-classes").expectMainFolder(),
new FolderPattern("build", "classes", "main").expectMainFolder(),
new FolderPattern("build", "classes", "java", "main").expectMainFolder(),
new FolderPattern("build", "classes", "java", "main", "my", "test").expectMainFolder(),

// Maven
$(new String[]{"target", "classes", "test"}, true),
$(new String[]{"target", "test-classes"}, false),
$(new String[]{"target", "classes"}, true),
new FolderPattern("target", "classes", "test").expectMainFolder(),
new FolderPattern("target", "test-classes").expectTestFolder(),
new FolderPattern("target", "classes").expectMainFolder(),

// IntelliJ
$(new String[]{"out", "production", "classes"}, true),
$(new String[]{"out", "test", "classes"}, false),
$(new String[]{"out", "test", "classes", "my", "test"}, false),
$(new String[]{"out", "some", "classes"}, true)
));
new FolderPattern("out", "production", "classes").expectMainFolder(),
new FolderPattern("out", "test", "classes").expectTestFolder(),
new FolderPattern("out", "test", "classes", "my", "test").expectTestFolder(),
new FolderPattern("out", "some", "classes").expectMainFolder()
);
}

@DataProvider
public static Object[][] test_location_predicates_and_expected_folder_patterns() {
List<Object[]> includeMainFolderInput = new ArrayList<>();
List<Object[]> includeTestFolderInput = new ArrayList<>();
for (FolderPattern folderPattern : getFolderPatterns()) {
includeMainFolderInput.add($(folderPattern.folders, folderPattern.isMainFolder));
includeTestFolderInput.add($(folderPattern.folders, !folderPattern.isMainFolder));
}

Object[][] doNotIncludeTestsDataPoints = crossProduct(do_not_include_tests(), includeMainFolderInput.toArray(new Object[0][]));
Object[][] onlyIncludeTestsDataPoints = crossProduct(only_include_tests(), includeTestFolderInput.toArray(new Object[0][]));

return copyOf(concat(asList(doNotIncludeTestsDataPoints), asList(onlyIncludeTestsDataPoints))).toArray(new Object[0][]);
}

@Test
@UseDataProvider("folders")
public void detects_all_output_folder_structures(
@UseDataProvider("test_location_predicates_and_expected_folder_patterns")
public void correctly_detects_all_output_folder_structures(
ImportOption doNotIncludeTests, String[] folderName, boolean expectedInclude) throws IOException {

File folder = temporaryFolder.newFolder(folderName);
Expand Down Expand Up @@ -128,4 +164,26 @@ private static Location locationOf(Class<?> clazz) {
private static boolean comesFromJarArchive(Class<?> clazz) {
return LocationTest.urlOfClass(clazz).getProtocol().equals("jar");
}

private static class FolderPattern {
final String[] folders;
final boolean isMainFolder;

FolderPattern(String... folders) {
this(folders, false);
}

private FolderPattern(String[] folders, boolean isMainFolder) {
this.folders = folders;
this.isMainFolder = isMainFolder;
}

FolderPattern expectMainFolder() {
return new FolderPattern(folders, true);
}

FolderPattern expectTestFolder() {
return new FolderPattern(folders, false);
}
}
}

0 comments on commit fa0b8ca

Please sign in to comment.