From 4bc916619fd286b2c0cc4d5c653c96a68801d74e Mon Sep 17 00:00:00 2001 From: Vladimir Sitnikov Date: Sat, 13 Mar 2021 18:35:33 +0300 Subject: [PATCH] Add Matcher#matches to ForbiddenApis to avoid its accidental use Matcher#matches is only needed when implementing new Matcher implementations which is very rare. The common test pitfall is assertThat(contains(actual).matches(expected), is(true)) It should better be written as assertThat(actual, contains(expected)) --- build.gradle.kts | 4 +- .../NormalizationTrimFieldTest.java | 3 +- .../test/AbstractMaterializedViewTest.java | 4 +- .../org/apache/calcite/test/Matchers.java | 2 +- .../java/org/apache/calcite/test/Unsafe.java | 43 +++++++++++++++++++ .../org/apache/calcite/util/UtilTest.java | 26 ++++++----- example/function/build.gradle.kts | 5 --- src/main/config/forbidden-apis/signatures.txt | 3 ++ 8 files changed, 66 insertions(+), 24 deletions(-) create mode 100644 core/src/test/java/org/apache/calcite/test/Unsafe.java diff --git a/build.gradle.kts b/build.gradle.kts index d2f5e843cad..138a39108b3 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -574,6 +574,7 @@ allprojects { configure { failOnUnsupportedJava = false + ignoreSignaturesOfMissingClasses = true bundledSignatures.addAll( listOf( "jdk-unsafe", @@ -675,7 +676,8 @@ allprojects { "**/org/apache/calcite/runtime/Resources${'$'}Inst.class", "**/org/apache/calcite/test/concurrent/ConcurrentTestCommandScript.class", "**/org/apache/calcite/test/concurrent/ConcurrentTestCommandScript${'$'}ShellCommand.class", - "**/org/apache/calcite/util/Unsafe.class" + "**/org/apache/calcite/util/Unsafe.class", + "**/org/apache/calcite/test/Unsafe.class" ) } diff --git a/core/src/test/java/org/apache/calcite/materialize/NormalizationTrimFieldTest.java b/core/src/test/java/org/apache/calcite/materialize/NormalizationTrimFieldTest.java index aab937bde20..9d4cc7722cc 100644 --- a/core/src/test/java/org/apache/calcite/materialize/NormalizationTrimFieldTest.java +++ b/core/src/test/java/org/apache/calcite/materialize/NormalizationTrimFieldTest.java @@ -46,7 +46,6 @@ import static org.apache.calcite.test.Matchers.isLinux; -import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; /** Tests trimming unused fields before materialized view matching. */ @@ -104,6 +103,6 @@ public static Frameworks.ConfigBuilder config() { + "LogicalProject(deptno=[CAST($0):TINYINT], count_sal=[$1])\n" + " LogicalTableScan(table=[[mv0]])\n"; final String relOptimizedStr = RelOptUtil.toString(relOptimized.get(0).getKey()); - assertThat(isLinux(optimized).matches(relOptimizedStr), is(true)); + assertThat(relOptimizedStr, isLinux(optimized)); } } diff --git a/core/src/test/java/org/apache/calcite/test/AbstractMaterializedViewTest.java b/core/src/test/java/org/apache/calcite/test/AbstractMaterializedViewTest.java index 7617813e1fe..eecb65d64c2 100644 --- a/core/src/test/java/org/apache/calcite/test/AbstractMaterializedViewTest.java +++ b/core/src/test/java/org/apache/calcite/test/AbstractMaterializedViewTest.java @@ -51,6 +51,7 @@ import org.apache.calcite.util.ImmutableBeans; import org.apache.calcite.util.Pair; import org.apache.calcite.util.TestUtil; +import org.apache.calcite.util.Util; import com.google.common.collect.ImmutableList; @@ -76,8 +77,9 @@ public abstract class AbstractMaterializedViewTest { protected Function resultContains( final String... expected) { return s -> { + String sLinux = Util.toLinux(s); for (String st : expected) { - if (!Matchers.containsStringLinux(st).matches(s)) { + if (!sLinux.contains(Util.toLinux(st))) { return false; } } diff --git a/core/src/test/java/org/apache/calcite/test/Matchers.java b/core/src/test/java/org/apache/calcite/test/Matchers.java index ea421d2689b..6d21cc77999 100644 --- a/core/src/test/java/org/apache/calcite/test/Matchers.java +++ b/core/src/test/java/org/apache/calcite/test/Matchers.java @@ -362,7 +362,7 @@ private static class ComposingMatcher extends TypeSafeMatcher { } protected boolean matchesSafely(F item) { - return matcher.matches(f.apply(item)); + return Unsafe.matches(matcher, f.apply(item)); } public void describeTo(Description description) { diff --git a/core/src/test/java/org/apache/calcite/test/Unsafe.java b/core/src/test/java/org/apache/calcite/test/Unsafe.java new file mode 100644 index 00000000000..bde19993611 --- /dev/null +++ b/core/src/test/java/org/apache/calcite/test/Unsafe.java @@ -0,0 +1,43 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.calcite.test; + +import org.hamcrest.Matcher; + +/** + * Contains methods that call JDK methods that the + * forbidden + * APIs checker does not approve of. + * + *

This class is excluded from the check, so methods called via this class + * will not fail the build. + */ +public class Unsafe { + private Unsafe() {} + + /** + * {@link Matcher#matches(Object)} is forbidden in regular test code in favour of + * {@link org.hamcrest.MatcherAssert#assertThat}. + * Note: {@code Matcher#matches} is still useful when testing matcher implementations. + * @param matcher matcher + * @param actual actual value + * @return the result of matcher.matches(actual) + */ + public static boolean matches(Matcher matcher, Object actual) { + return matcher.matches(actual); + } +} diff --git a/core/src/test/java/org/apache/calcite/util/UtilTest.java b/core/src/test/java/org/apache/calcite/util/UtilTest.java index c0453c32d04..37c8106ba14 100644 --- a/core/src/test/java/org/apache/calcite/util/UtilTest.java +++ b/core/src/test/java/org/apache/calcite/util/UtilTest.java @@ -36,6 +36,7 @@ import org.apache.calcite.sql.util.SqlString; import org.apache.calcite.test.DiffTestCase; import org.apache.calcite.test.Matchers; +import org.apache.calcite.test.Unsafe; import org.apache.calcite.testlib.annotations.LocaleEnUs; import com.google.common.collect.ImmutableList; @@ -2720,13 +2721,10 @@ private void checkNameMultimap(String s, NameMultimap map) { /** Unit test for {@link Matchers#compose}. */ @Test void testComposeMatcher() { - assertThat("x", is("x")); - assertThat(is("x").matches("x"), is(true)); - assertThat(is("X").matches("x"), is(false)); final Function toUpper = s -> s.toUpperCase(Locale.ROOT); - assertThat(Matchers.compose(is("A"), toUpper).matches("a"), is(true)); - assertThat(Matchers.compose(is("A"), toUpper).matches("A"), is(true)); - assertThat(Matchers.compose(is("a"), toUpper).matches("A"), is(false)); + assertThat(Unsafe.matches(Matchers.compose(is("A"), toUpper), "a"), is(true)); + assertThat(Unsafe.matches(Matchers.compose(is("A"), toUpper), "A"), is(true)); + assertThat(Unsafe.matches(Matchers.compose(is("a"), toUpper), "A"), is(false)); assertThat(describe(Matchers.compose(is("a"), toUpper)), is("is \"a\"")); assertThat(mismatchDescription(Matchers.compose(is("a"), toUpper), "A"), is("was \"A\"")); @@ -2737,18 +2735,18 @@ private void checkNameMultimap(String s, NameMultimap map) { assertThat("xy", isLinux("xy")); assertThat("x\ny", isLinux("x\ny")); assertThat("x\r\ny", isLinux("x\ny")); - assertThat(isLinux("x").matches("x"), is(true)); - assertThat(isLinux("X").matches("x"), is(false)); + assertThat(Unsafe.matches(isLinux("x"), "x"), is(true)); + assertThat(Unsafe.matches(isLinux("X"), "x"), is(false)); assertThat(mismatchDescription(isLinux("X"), "x"), is("was \"x\"")); assertThat(describe(isLinux("X")), is("is \"X\"")); - assertThat(isLinux("x\ny").matches("x\ny"), is(true)); - assertThat(isLinux("x\ny").matches("x\r\ny"), is(true)); + assertThat(Unsafe.matches(isLinux("x\ny"), "x\ny"), is(true)); + assertThat(Unsafe.matches(isLinux("x\ny"), "x\r\ny"), is(true)); //\n\r is not a valid windows line ending - assertThat(isLinux("x\ny").matches("x\n\ry"), is(false)); - assertThat(isLinux("x\ny").matches("x\n\ryz"), is(false)); + assertThat(Unsafe.matches(isLinux("x\ny"), "x\n\ry"), is(false)); + assertThat(Unsafe.matches(isLinux("x\ny"), "x\n\ryz"), is(false)); // left-hand side must be linux or will never match - assertThat(isLinux("x\r\ny").matches("x\r\ny"), is(false)); - assertThat(isLinux("x\r\ny").matches("x\ny"), is(false)); + assertThat(Unsafe.matches(isLinux("x\r\ny"), "x\r\ny"), is(false)); + assertThat(Unsafe.matches(isLinux("x\r\ny"), "x\ny"), is(false)); } /** Tests {@link Util#andThen(UnaryOperator, UnaryOperator)}. */ diff --git a/example/function/build.gradle.kts b/example/function/build.gradle.kts index 7f585cb6531..a5bd1c69c20 100644 --- a/example/function/build.gradle.kts +++ b/example/function/build.gradle.kts @@ -19,10 +19,5 @@ dependencies { api(project(":linq4j")) api("org.checkerframework:checker-qual") - implementation("com.google.guava:guava") { - because("""ForbiddenApis' signatures.txt contains com.google.common.base.Precondition - and it needs the file on a classpath to parse the configuration""" - ) - } testImplementation("sqlline:sqlline") } diff --git a/src/main/config/forbidden-apis/signatures.txt b/src/main/config/forbidden-apis/signatures.txt index 8de47717afb..c498312fc4e 100644 --- a/src/main/config/forbidden-apis/signatures.txt +++ b/src/main/config/forbidden-apis/signatures.txt @@ -101,3 +101,6 @@ com.google.common.collect.Maps#newTreeMap() @defaultMessage Use "new HashSet<>()" com.google.common.collect.Sets#newHashSet() + +@defaultMessage Use "assertThat(expected, matcher)", do not call Matcher#matches directly +org.hamcrest.Matcher#matches(java.lang.Object)