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

Warn when feature / glue paths passed as file schemes #2547

Merged
merged 13 commits into from
May 17, 2022
23 changes: 23 additions & 0 deletions core/src/main/java/io/cucumber/core/feature/FeaturePath.java
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
package io.cucumber.core.feature;

import io.cucumber.core.logging.Logger;
import io.cucumber.core.logging.LoggerFactory;

import java.io.File;
import java.net.URI;
import java.util.Locale;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import static io.cucumber.core.resource.ClasspathSupport.CLASSPATH_SCHEME_PREFIX;
import static io.cucumber.core.resource.ClasspathSupport.RESOURCE_SEPARATOR_CHAR;
Expand All @@ -27,6 +32,10 @@
*/
public class FeaturePath {

private static final Logger log = LoggerFactory.getLogger(FeaturePath.class);

private static final Pattern FILESYSTEM_PATH_TO_RESOURCES = Pattern.compile("src/(?:main|test)/resources(/?)(.*)");

private FeaturePath() {

}
Expand Down Expand Up @@ -69,6 +78,7 @@ private static String replaceNonStandardPathSeparator(String featureIdentifier)
}

private static URI parseAssumeFileScheme(String featureIdentifier) {
warnWhenFileSystemPathToResources(featureIdentifier);
File featureFile = new File(featureIdentifier);
return featureFile.toURI();
}
Expand Down Expand Up @@ -101,4 +111,17 @@ private static String normalize(final String value) {
return value.toLowerCase(Locale.US).replaceAll("[^a-z0-9]+", "");
}

private static void warnWhenFileSystemPathToResources(String featureIdentifier) {
Matcher matcher = FILESYSTEM_PATH_TO_RESOURCES.matcher(featureIdentifier);
if (matcher.matches()) {
if (!matcher.group(1).equals("/") && matcher.group(2).length() > 0) {
return;
}
log.warn(
() -> String.format("Consider replacing feature path '%s' with '%s'.",
featureIdentifier,
matcher.replaceAll("classpath:$2")));
}
}

}
36 changes: 36 additions & 0 deletions core/src/main/java/io/cucumber/core/feature/GluePath.java
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
package io.cucumber.core.feature;

import io.cucumber.core.logging.Logger;
import io.cucumber.core.logging.LoggerFactory;

import java.io.File;
import java.net.URI;
import java.net.URISyntaxException;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import static io.cucumber.core.resource.ClasspathSupport.CLASSPATH_SCHEME;
import static io.cucumber.core.resource.ClasspathSupport.CLASSPATH_SCHEME_PREFIX;
Expand All @@ -29,6 +34,11 @@
*/
public class GluePath {

private static final Logger log = LoggerFactory.getLogger(GluePath.class);

private static final Pattern FILESYSTEM_PATH_TO_PACKAGES = Pattern
.compile("src/(?:main|test)/(?:java|kotlin|scala|groovy)(|/|/.+)");
tal66 marked this conversation as resolved.
Show resolved Hide resolved

private GluePath() {

}
Expand Down Expand Up @@ -70,6 +80,8 @@ private static String replaceNonStandardPathSeparator(String featureIdentifier)
private static URI parseAssumeClasspathScheme(String gluePath) {
URI uri = URI.create(gluePath);

warnWhenFileSystemPath(gluePath);

String schemeSpecificPart = uri.getSchemeSpecificPart();
if (!isValidIdentifier(schemeSpecificPart)) {
throw new IllegalArgumentException("The glue path contained invalid identifiers " + uri);
Expand All @@ -92,6 +104,30 @@ private static URI parseAssumeClasspathScheme(String gluePath) {
return uri;
}

private static void warnWhenFileSystemPath(String gluePath) {
Matcher matcher = FILESYSTEM_PATH_TO_PACKAGES.matcher(gluePath);
if (!matcher.matches()) {
return;
}
log.warn(() -> {
String classPathResource = matcher.group(1);
if (classPathResource.startsWith("/")) {
classPathResource = classPathResource.substring(1);
}
if (classPathResource.endsWith("/")) {
classPathResource = classPathResource.substring(0, classPathResource.length() - 1);
}
String packageName = classPathResource.replaceAll("/",".");
String message = "" +
"Cucumber was given the glue path '%s'. This path points to a source directory\n" +
"in your project. However cucumber looks for glue (i.e. step definitions) on the\n" +
"classpath.\n" +
"\n" +
"To avoid confusion consider using a package name instead for example: '%s'\n";
return String.format(message, gluePath, packageName);
});
}

private static boolean isProbablyPackage(String gluePath) {
return gluePath.contains(PACKAGE_SEPARATOR_STRING)
&& !gluePath.contains(RESOURCE_SEPARATOR_STRING);
Expand Down
37 changes: 37 additions & 0 deletions core/src/test/java/io/cucumber/core/feature/FeaturePathTest.java
Original file line number Diff line number Diff line change
@@ -1,18 +1,26 @@
package io.cucumber.core.feature;

import io.cucumber.core.logging.LogRecordListener;
import io.cucumber.core.logging.LoggerFactory;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.condition.DisabledOnOs;
import org.junit.jupiter.api.condition.EnabledOnOs;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

import java.io.File;
import java.net.URI;
import java.util.stream.Stream;

import static org.hamcrest.CoreMatchers.endsWith;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.Is.is;
import static org.hamcrest.text.MatchesPattern.matchesPattern;
import static org.junit.jupiter.api.Assertions.assertAll;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.condition.OS.WINDOWS;
import static org.junit.jupiter.params.provider.Arguments.arguments;

class FeaturePathTest {

Expand Down Expand Up @@ -146,4 +154,33 @@ void can_parse_windows_file_path_with_standard_file_separator() {
() -> assertThat(uri.getSchemeSpecificPart(), is("/C:/path/to/file.feature")));
}

@ParameterizedTest
@MethodSource("FeaturePathAndPatternProvider")
void warn_when_features_as_filesystem_path_to_resources(String featurePath, String logPattern) {
// warn when 'src/{test,main}/resources' is used

LogRecordListener logRecordListener = new LogRecordListener();
LoggerFactory.addListener(logRecordListener);

FeaturePath.parse(featurePath);

LoggerFactory.removeListener(logRecordListener);

String logMessage = "";
if (!logRecordListener.getLogRecords().isEmpty()) {
logMessage = logRecordListener.getLogRecords().get(0).getMessage();
}

assertThat(logMessage, matchesPattern(logPattern));
}

static Stream<Arguments> FeaturePathAndPatternProvider() {
return Stream.of(
arguments("src/test/resources_abcd", ""),
arguments("src/test/resources/", ".*replacing.*src/test/resources/.*'classpath:'.*"),
arguments("src/test/resources/features",
".*replacing.*src/test/resources/features.*'classpath:features'.*"),
arguments("src/main/resources/features/feature1",
".*replacing.*src/main/resources/features/feature1.*'classpath:features/feature1'.*"));
}
}
91 changes: 72 additions & 19 deletions core/src/test/java/io/cucumber/core/feature/GluePathTest.java
Original file line number Diff line number Diff line change
@@ -1,17 +1,33 @@
package io.cucumber.core.feature;

import io.cucumber.core.logging.LogRecordListener;
import io.cucumber.core.logging.LoggerFactory;
import org.hamcrest.CoreMatchers;
import org.hamcrest.Matcher;
import org.hamcrest.text.IsEmptyString;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.condition.EnabledOnOs;
import org.junit.jupiter.api.condition.OS;
import org.junit.jupiter.api.function.Executable;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

import java.net.URI;
import java.util.logging.LogRecord;
import java.util.stream.Stream;

import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.nullValue;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.isEmptyString;
import static org.hamcrest.Matchers.matchesPattern;
import static org.hamcrest.core.Is.is;
import static org.hamcrest.core.IsEqual.equalTo;
import static org.hamcrest.text.IsEmptyString.emptyOrNullString;
import static org.junit.jupiter.api.Assertions.assertAll;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.params.provider.Arguments.arguments;

class GluePathTest {

Expand All @@ -20,17 +36,17 @@ void can_parse_empty_glue_path() {
URI uri = GluePath.parse("");

assertAll(
() -> assertThat(uri.getScheme(), is("classpath")),
() -> assertThat(uri.getSchemeSpecificPart(), is("/")));
() -> assertThat(uri.getScheme(), is("classpath")),
() -> assertThat(uri.getSchemeSpecificPart(), is("/")));
}

@Test
void can_parse_root_package() {
URI uri = GluePath.parse("classpath:/");

assertAll(
() -> assertThat(uri.getScheme(), is("classpath")),
() -> assertThat(uri.getSchemeSpecificPart(), is("/")));
() -> assertThat(uri.getScheme(), is("classpath")),
() -> assertThat(uri.getSchemeSpecificPart(), is("/")));
}

@Test
Expand All @@ -39,60 +55,60 @@ void can_parse_eclipse_plugin_default_glue() {
URI uri = GluePath.parse("classpath:");

assertAll(
() -> assertThat(uri.getScheme(), is("classpath")),
() -> assertThat(uri.getSchemeSpecificPart(), is("/")));
() -> assertThat(uri.getScheme(), is("classpath")),
() -> assertThat(uri.getSchemeSpecificPart(), is("/")));
}

@Test
void can_parse_classpath_form() {
URI uri = GluePath.parse("classpath:com/example/app");

assertAll(
() -> assertThat(uri.getScheme(), is("classpath")),
() -> assertThat(uri.getSchemeSpecificPart(), is("com/example/app")));
() -> assertThat(uri.getScheme(), is("classpath")),
() -> assertThat(uri.getSchemeSpecificPart(), is("com/example/app")));
}

@Test
void can_parse_relative_path_form() {
URI uri = GluePath.parse("com/example/app");

assertAll(
() -> assertThat(uri.getScheme(), is("classpath")),
() -> assertThat(uri.getSchemeSpecificPart(), is("/com/example/app")));
() -> assertThat(uri.getScheme(), is("classpath")),
() -> assertThat(uri.getSchemeSpecificPart(), is("/com/example/app")));
}

@Test
void can_parse_absolute_path_form() {
URI uri = GluePath.parse("/com/example/app");

assertAll(
() -> assertThat(uri.getScheme(), is("classpath")),
() -> assertThat(uri.getSchemeSpecificPart(), is("/com/example/app")));
() -> assertThat(uri.getScheme(), is("classpath")),
() -> assertThat(uri.getSchemeSpecificPart(), is("/com/example/app")));
}

@Test
void can_parse_package_form() {
URI uri = GluePath.parse("com.example.app");

assertAll(
() -> assertThat(uri.getScheme(), is("classpath")),
() -> assertThat(uri.getSchemeSpecificPart(), is("/com/example/app")));
() -> assertThat(uri.getScheme(), is("classpath")),
() -> assertThat(uri.getSchemeSpecificPart(), is("/com/example/app")));
}

@Test
void glue_path_must_have_class_path_scheme() {
Executable testMethod = () -> GluePath.parse("file:com/example/app");
IllegalArgumentException actualThrown = assertThrows(IllegalArgumentException.class, testMethod);
assertThat("Unexpected exception message", actualThrown.getMessage(), is(equalTo(
"The glue path must have a classpath scheme file:com/example/app")));
"The glue path must have a classpath scheme file:com/example/app")));
}

@Test
void glue_path_must_have_valid_identifier_parts() {
Executable testMethod = () -> GluePath.parse("01-examples");
IllegalArgumentException actualThrown = assertThrows(IllegalArgumentException.class, testMethod);
assertThat("Unexpected exception message", actualThrown.getMessage(), is(equalTo(
"The glue path contained invalid identifiers 01-examples")));
"The glue path contained invalid identifiers 01-examples")));
}

@Test
Expand All @@ -101,8 +117,8 @@ void can_parse_windows_path_form() {
URI uri = GluePath.parse("com\\example\\app");

assertAll(
() -> assertThat(uri.getScheme(), is("classpath")),
() -> assertThat(uri.getSchemeSpecificPart(), is(equalTo("/com/example/app"))));
() -> assertThat(uri.getScheme(), is("classpath")),
() -> assertThat(uri.getSchemeSpecificPart(), is(equalTo("/com/example/app"))));
}

@Test
Expand All @@ -111,7 +127,44 @@ void absolute_windows_path_form_is_not_valid() {
Executable testMethod = () -> GluePath.parse("C:\\com\\example\\app");
IllegalArgumentException actualThrown = assertThrows(IllegalArgumentException.class, testMethod);
assertThat("Unexpected exception message", actualThrown.getMessage(), is(equalTo(
"The glue path must have a classpath scheme C:/com/example/app")));
"The glue path must have a classpath scheme C:/com/example/app")));
}

@ParameterizedTest
@MethodSource("GluePathAndPatternProvider")
void warn_when_glue_as_filesystem_path(String gluePath, Matcher<String> logPattern) {
tal66 marked this conversation as resolved.
Show resolved Hide resolved
// warn when 'src/{test,main}/{java,kotlin,scala,groovy}' is used

LogRecordListener logRecordListener = new LogRecordListener();
LoggerFactory.addListener(logRecordListener);

GluePath.parse(gluePath);

LoggerFactory.removeListener(logRecordListener);

String logMessage = logRecordListener.getLogRecords()
.stream()
.findFirst()
.map(LogRecord::getMessage)
.orElse(null);

assertThat(logMessage, logPattern);
}

static Stream<Arguments> GluePathAndPatternProvider() {
return Stream.of(
arguments("src/main/java", containsString("for example: ''")),
arguments("src/main/java/", containsString("for example: ''")),
arguments("src/main/java_other", nullValue()),
arguments("src/main/other", nullValue()),
arguments("src/main/java/com", containsString("for example: 'com'")),
arguments("src/main/java/com/", containsString("for example: 'com'")),
arguments("src/main/groovy/com", containsString("for example: 'com'")),
arguments("src/main/java/com/example", containsString("for example: 'com.example'")),
arguments("src/main/java/com/example/", containsString("for example: 'com.example'")),
arguments("src/main/java/com/example/package", containsString("for example: 'com.example.package'"))

);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import io.cucumber.core.backend.ObjectFactory;
import io.cucumber.core.exception.CucumberException;
import io.cucumber.core.plugin.DefaultSummaryPrinter;
import io.cucumber.core.plugin.HtmlFormatter;
import io.cucumber.core.plugin.NoPublishFormatter;
import io.cucumber.core.plugin.PluginFactory;
Expand All @@ -13,7 +12,6 @@
import io.cucumber.core.snippets.SnippetType;
import io.cucumber.plugin.Plugin;
import io.cucumber.tagexpressions.TagExpressionException;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.function.Executable;

Expand Down
Loading