tkalkirill commented on code in PR #13602:
URL: https://github.com/apache/ignite/pull/13602#discussion_r4095305459
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/AbstractExecutionTest.java:
##########
@@ -156,9 +151,21 @@ public T2<Runnable, Integer> nextTask(Deque<T2<Runnable,
Integer>> tasks) {
}
}
+ /** Task executor. */
+ @Parameter(0)
+ public TaskExecutorType taskExecutorType;
+
+ /** Execution direction. */
+ @Parameter(LAST_PARAM_NUM)
Review Comment:
Maybe `LAST_PARAM_NUM` isn't needed or what is it for?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/ContinuousExecutionTest.java:
##########
@@ -79,11 +81,13 @@ public static List<Object[]> data() {
);
for (Object[] newParam : newParams) {
- for (Object[] inheritedParam : AbstractExecutionTest.parameters())
{
- Object[] both = Stream.concat(Arrays.stream(inheritedParam),
Arrays.stream(newParam))
- .toArray(Object[]::new);
+ for (Object[] inheritedParam : innerParams()) {
Review Comment:
I’d suggest avoiding explicit array copying; this is a test, not production
code, so you can easily use streams or collections instead.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/BaseAggregateTest.java:
##########
@@ -45,41 +44,46 @@
import org.apache.ignite.internal.util.typedef.F;
import org.apache.ignite.internal.util.typedef.X;
import org.apache.ignite.testframework.GridTestUtils;
-import org.apache.ignite.testframework.junits.WithSystemProperty;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runners.Parameterized;
+import org.apache.ignite.testframework.junit.SystemPropertiesExtension;
+import org.apache.ignite.testframework.junit.WithSystemProperty;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.params.Parameter;
+import org.junit.jupiter.params.ParameterizedClass;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
/**
*
*/
@SuppressWarnings("TypeMayBeWeakened")
+@ExtendWith(SystemPropertiesExtension.class)
@WithSystemProperty(key = "calcite.debug", value = "true")
+@ParameterizedClass(name = "Task executor = {0}, Execution strategy = {1},
testAgg={2}")
+@MethodSource("data")
public abstract class BaseAggregateTest extends AbstractExecutionTest {
/** Last parameter number. */
protected static final int TEST_AGG_PARAM_NUM = LAST_PARAM_NUM + 1;
/** */
- @Parameterized.Parameter(TEST_AGG_PARAM_NUM)
+ @Parameter(TEST_AGG_PARAM_NUM)
public TestAggregateType testAgg;
/** */
- @Parameterized.Parameters(name = PARAMS_STRING + ", type={" +
TEST_AGG_PARAM_NUM + "}")
- public static List<Object[]> data() {
- List<Object[]> extraParams = new ArrayList<>();
+ private static List<Arguments> data() {
+ List<Arguments> extraParams = new ArrayList<>();
- ImmutableList<Object[]> newParams = ImmutableList.of(
- new Object[] {TestAggregateType.SINGLE},
- new Object[] {TestAggregateType.MAP_REDUCE}
- );
-
- for (Object[] newParam : newParams) {
- for (Object[] inheritedParam : AbstractExecutionTest.parameters())
{
- Object[] both = Stream.concat(Arrays.stream(inheritedParam),
Arrays.stream(newParam))
- .toArray(Object[]::new);
+ for (TestAggregateType newParam : TestAggregateType.values()) {
+ for (Object[] inheritedParam : innerParams()) {
Review Comment:
I’d suggest avoiding explicit array copying; this is a test, not production
code, so you can easily use streams or collections instead.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/AbstractMultiEngineIntegrationTest.java:
##########
@@ -16,32 +16,28 @@
*/
package org.apache.ignite.internal.processors.query.calcite.integration;
-import java.util.Arrays;
-import java.util.Collection;
import java.util.List;
import org.apache.ignite.cache.query.SqlFieldsQuery;
import org.apache.ignite.calcite.CalciteQueryEngineConfiguration;
import org.apache.ignite.configuration.IgniteConfiguration;
import org.apache.ignite.configuration.SqlConfiguration;
import org.apache.ignite.indexing.IndexingQueryEngineConfiguration;
import org.apache.ignite.internal.IgniteEx;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.params.Parameter;
+import org.junit.jupiter.params.ParameterizedClass;
+import org.junit.jupiter.params.provider.CsvSource;
/** */
-@RunWith(Parameterized.class)
+@ParameterizedClass(name = "Query engine={0}")
+@CsvSource({CalciteQueryEngineConfiguration.ENGINE_NAME,
IndexingQueryEngineConfiguration.ENGINE_NAME})
Review Comment:
Why `CsvSource` and not simply an array of strings as the source?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/CacheStoreTest.java:
##########
@@ -150,6 +168,12 @@ public void testCacheStoreOnDML() throws Exception {
checkReadThrough(1002, 1003, 1004);
}
+ /** */
+ @Override protected List<List<?>> sql(IgniteEx ignite, String sql,
Object... params) {
+ return ignite.context().query().querySqlFields(new
SqlFieldsQuery(sql).setArgs(params), true)
Review Comment:
Do I remember correctly that the cursor needs to be closed?
##########
modules/tools/src/main/java/org/apache/ignite/tools/surefire/testsuites/CheckAllTestsInSuites.java:
##########
@@ -113,6 +113,13 @@ private void processSuite(Description suite, Set<String>
suitedClasses,
}
}
+ /** Calcite module tests inherited from legacy junit4 related classes. */
+ private static final Set<String> CALCITE_TESTS = Set.of(
Review Comment:
Are we going to get rid of this? Can you point it out or create a ticket?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/PlannerTimeoutTest.java:
##########
@@ -33,13 +33,24 @@
import org.apache.ignite.internal.processors.query.calcite.schema.IgniteSchema;
import
org.apache.ignite.internal.processors.query.calcite.trait.IgniteDistributions;
import org.apache.ignite.internal.processors.query.calcite.trait.TraitUtils;
+import org.apache.ignite.internal.util.CommonUtils;
import org.apache.ignite.testframework.GridTestUtils;
-import org.junit.Test;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Order;
+import org.junit.jupiter.api.Test;
/**
* Test planner timeout.
*/
+@Order(1)
Review Comment:
What is this annotation doing here?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/TestSuiteDeclarationArchTest.java:
##########
@@ -0,0 +1,127 @@
+/*
+ * 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.ignite.internal.processors.query.calcite.integration;
+
+import java.util.Set;
+import com.tngtech.archunit.core.domain.JavaClass;
+import com.tngtech.archunit.core.domain.JavaClasses;
+import com.tngtech.archunit.core.domain.JavaMethod;
+import com.tngtech.archunit.core.domain.JavaModifier;
+import com.tngtech.archunit.core.importer.ClassFileImporter;
+import com.tngtech.archunit.core.importer.ImportOption;
+import com.tngtech.archunit.core.importer.Location;
+import com.tngtech.archunit.junit.AnalyzeClasses;
+import com.tngtech.archunit.junit.ArchTest;
+import com.tngtech.archunit.junit.LocationProvider;
+import com.tngtech.archunit.lang.ArchCondition;
+import com.tngtech.archunit.lang.ArchRule;
+import com.tngtech.archunit.lang.ConditionEvents;
+import com.tngtech.archunit.lang.SimpleConditionEvent;
+import org.junit.jupiter.api.Order;
+import org.junit.platform.suite.api.SelectClasses;
+import org.junit.platform.suite.api.Suite;
+
+import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.methods;
+
+/** */
+@AnalyzeClasses(
+ importOptions = ImportOption.OnlyIncludeTests.class,
+ locations = TestSuiteDeclarationArchTest.CalciteLocationProvider.class)
+@Order(1)
+public class TestSuiteDeclarationArchTest {
Review Comment:
There is a lack of documentation explaining the purpose of this test.
##########
modules/calcite/src/test/java/org/apache/ignite/testsuites/ScriptTestSuite.java:
##########
@@ -71,7 +97,253 @@
* @see <a
href="https://www.sqlite.org/sqllogictest/doc/trunk/about.wiki";>Extended format
documentation.</a></a>
*
*/
-@RunWith(ScriptTestRunner.class)
@ScriptRunnerTestsEnvironment(scriptsRoot = "modules/calcite/src/test/sql",
timeout = 180000)
public class ScriptTestSuite {
+ /** Filesystem. */
+ private static final FileSystem FS = FileSystems.getDefault();
+
+ /** Suffix of the scripts that are reported as ignored. */
+ private static final String IGNORED_SUFFIX = ".test_ignore";
+
+ /** Shared finder. */
+ private static final TcpDiscoveryVmIpFinder sharedFinder = new
TcpDiscoveryVmIpFinder().setShared(true);
+
+ /** */
+ private static IgniteLogger log;
+
+ static {
+ try {
+ log = new
GridTestLog4jLogger(U.resolveIgnitePath("modules/core/src/test/config/log4j2-test.xml"));
+ }
+ catch (Exception e) {
+ e.printStackTrace(System.err);
+
+ log = null;
+
+ assert false : "Cannot init logger";
+ }
+ }
+
+ /** Scripts root directory. */
+ private final Path scriptsRoot;
+
+ /** Regex to filter test path to run only specified tests. */
+ private final Pattern testRegex;
+
+ /** Nodes count. */
+ private final int nodes;
+
+ /** Restart cluster for each test group. */
+ private final boolean restartCluster;
+
+ /** Test script timeout. */
+ private final long timeout;
+
+ /** Directory (test group) of the last executed script. */
+ private Path lastTestDir;
+
+ /** */
+ public ScriptTestSuite() {
+ ScriptRunnerTestsEnvironment env =
ScriptTestSuite.class.getAnnotation(ScriptRunnerTestsEnvironment.class);
+
+ assert !F.isEmpty(env.scriptsRoot());
Review Comment:
Please use `assertTrue` or something similar in the tests.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/tx/AbstractQueryTransactionIsolationTest.java:
##########
@@ -0,0 +1,707 @@
+/*
+ * 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.ignite.internal.processors.tx;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.stream.Collectors;
+import javax.cache.processor.EntryProcessor;
+import javax.cache.processor.EntryProcessorException;
+import javax.cache.processor.MutableEntry;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.IgniteException;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.cache.CacheAtomicityMode;
+import org.apache.ignite.cache.CacheMode;
+import org.apache.ignite.cache.CacheWriteSynchronizationMode;
+import org.apache.ignite.cache.QueryEntity;
+import org.apache.ignite.cache.QueryIndex;
+import org.apache.ignite.client.ClientTransaction;
+import org.apache.ignite.client.Config;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.processors.odbc.ClientMessage;
+import org.apache.ignite.internal.processors.odbc.jdbc.JdbcConnectionContext;
+import
org.apache.ignite.internal.processors.platform.client.ClientConnectionContext;
+import org.apache.ignite.internal.processors.query.QueryUtils;
+import
org.apache.ignite.internal.processors.query.calcite.GridCommonAbstractWrapperTest;
+import org.apache.ignite.internal.util.lang.RunnableX;
+import org.apache.ignite.internal.util.nio.GridNioServer;
+import org.apache.ignite.internal.util.nio.GridNioSession;
+import org.apache.ignite.internal.util.tostring.GridToStringInclude;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.S;
+import org.apache.ignite.lang.IgniteBiTuple;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.apache.ignite.transactions.Transaction;
+import org.apache.ignite.transactions.TransactionConcurrency;
+import org.apache.ignite.transactions.TransactionIsolation;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.Parameter;
+
+import static
org.apache.ignite.internal.processors.odbc.ClientListenerNioListener.CONN_CTX_META_KEY;
+import static
org.apache.ignite.internal.processors.tx.AbstractQueryTransactionIsolationTest.ModifyApi.CACHE;
+import static
org.apache.ignite.internal.processors.tx.AbstractQueryTransactionIsolationTest.ModifyApi.ENTRY_PROCESSOR;
+import static
org.apache.ignite.internal.processors.tx.AbstractQueryTransactionIsolationTest.ModifyApi.QUERY;
+import static
org.apache.ignite.transactions.TransactionIsolation.READ_COMMITTED;
+
+/** Copy paste from code module with removed legacy junit stuff. */
Review Comment:
I didn't quite follow your description do I need to mention anything about
JUnit?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/AuthorizationIntegrationTest.java:
##########
@@ -87,15 +94,29 @@ public class AuthorizationIntegrationTest extends
AbstractSecurityTest {
private static final AtomicInteger removeCnt = new AtomicInteger();
/** */
- @Parameterized.Parameter
+ @Parameter(0)
public boolean allowDdl;
/** */
- @Parameterized.Parameters(name = "allowDdl = {0}")
+ @MethodSource("parameters")
public static Iterable<Object> parameters() {
return Arrays.asList(false, true);
}
+ /** */
+ @BeforeAll
+ static void init() {
+ beforeFirstTest0();
Review Comment:
Why did you introduce that?
Is the order actually defined for `BeforeAll`?
Shouldn't you just override the base `BeforeAll`?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/BaseAggregateTest.java:
##########
@@ -45,41 +44,46 @@
import org.apache.ignite.internal.util.typedef.F;
import org.apache.ignite.internal.util.typedef.X;
import org.apache.ignite.testframework.GridTestUtils;
-import org.apache.ignite.testframework.junits.WithSystemProperty;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runners.Parameterized;
+import org.apache.ignite.testframework.junit.SystemPropertiesExtension;
+import org.apache.ignite.testframework.junit.WithSystemProperty;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.params.Parameter;
+import org.junit.jupiter.params.ParameterizedClass;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
/**
*
*/
@SuppressWarnings("TypeMayBeWeakened")
+@ExtendWith(SystemPropertiesExtension.class)
@WithSystemProperty(key = "calcite.debug", value = "true")
+@ParameterizedClass(name = "Task executor = {0}, Execution strategy = {1},
testAgg={2}")
+@MethodSource("data")
public abstract class BaseAggregateTest extends AbstractExecutionTest {
/** Last parameter number. */
protected static final int TEST_AGG_PARAM_NUM = LAST_PARAM_NUM + 1;
/** */
- @Parameterized.Parameter(TEST_AGG_PARAM_NUM)
+ @Parameter(TEST_AGG_PARAM_NUM)
Review Comment:
Maybe `TEST_AGG_PARAM_NUM` isn't needed or what is it for?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/AuthorizationIntegrationTest.java:
##########
@@ -63,7 +68,9 @@
/**
* Test authorization of different operations.
*/
-@RunWith(Parameterized.class)
+@ParameterizedClass(name = "allowDdl = {0}")
+@MethodSource("parameters")
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)
Review Comment:
What is this annotation for?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/CalciteBasicSecondaryIndexIntegrationTest.java:
##########
@@ -920,7 +921,7 @@ public void testOrCondition5() {
// ===== various complex conditions =====
/** */
- @Ignore("TODO")
+ @Disabled("TODO")
Review Comment:
Please fix it. I mean, create or find a ticket.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/AbstractBasicIntegrationTest.java:
##########
@@ -88,6 +93,23 @@ protected boolean destroyCachesAfterTest() {
return true;
}
+ /** */
+ @AfterEach
+ public void runAfterTest() throws Exception {
+ AtomicBoolean afterTestFinished = new AtomicBoolean(false);
+
+ ScheduledExecutorService scheduler =
scheduleThreadDumpOnAfterTestTimeOut(afterTestFinished);
Review Comment:
What is the purpose of this? Couldn't this be done using standard tools?
Don't we just want to stop the executor?
We could take an example from Ignite 3.
##########
modules/core/src/test/java/org/apache/ignite/testframework/junits/GridAbstractTest.java:
##########
@@ -685,6 +685,11 @@ private void resolveWorkDirectory() throws Exception {
sft.mkdirMarshaller();
}
+ /** */
+ protected static void beforeFirstTest0() {
Review Comment:
What is it for, and how does it differ from the `beforeFirstTest`?
##########
modules/tools/src/main/java/org/apache/ignite/tools/junit/JUnitTeamcityReporter.java:
##########
@@ -220,8 +220,12 @@ private String fileName() {
return "test-" + prevSuite + prevFlush + ".xml";
}
- /** */
- private String escapeForTeamcity(String msg) {
+ /**
+ * @param msg Message.
+ *
+ * @return Escaped string.
+ */
+ public static String escapeForTeamcity(String msg) {
Review Comment:
I think it's worth restoring the description; it doesn't add anything new.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/RunningQueriesIntegrationTest.java:
##########
@@ -88,9 +91,12 @@ public class RunningQueriesIntegrationTest extends
AbstractBasicIntegrationTest
private static final long TIMEOUT_IN_MS = 10_000;
/** {@inheritDoc} */
+ @BeforeAll
@Override protected void beforeTestsStarted() throws Exception {
super.beforeTestsStarted();
+ plannerTimeout = getLong(IGNITE_CALCITE_PLANNER_TIMEOUT, 0);
Review Comment:
Why not read it once?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/CalcitePlanningDumpTest.java:
##########
@@ -19,27 +19,27 @@
package org.apache.ignite.internal.processors.query.calcite.integration;
import java.util.stream.Stream;
-
import org.apache.calcite.plan.RelOptPlanner;
import org.apache.calcite.rel.RelCollation;
import org.apache.ignite.internal.processors.query.QueryUtils;
import org.apache.ignite.internal.processors.query.calcite.schema.IgniteTable;
import org.apache.ignite.internal.util.typedef.X;
-import org.apache.ignite.testframework.junits.WithSystemProperty;
-import org.junit.Test;
-
-import static org.apache.ignite.IgniteCommonsSystemProperties.getLong;
+import org.apache.ignite.testframework.junit.SystemPropertiesExtension;
+import org.apache.ignite.testframework.junit.WithSystemProperty;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
import static
org.apache.ignite.internal.processors.query.calcite.CalciteQueryProcessor.IGNITE_CALCITE_PLANNER_TIMEOUT;
import static
org.apache.ignite.testframework.GridTestUtils.assertThrowsWithCause;
/**
* This test assumes no Calcite classes are loaded in the current JVM before
CalciteQueryProcessor.
* Violating this invariant may cause the test to fail due to premature
Calcite initialization.
*/
+@ExtendWith(SystemPropertiesExtension.class)
@WithSystemProperty(key = IGNITE_CALCITE_PLANNER_TIMEOUT, value = "1000")
public class CalcitePlanningDumpTest extends AbstractBasicIntegrationTest {
/** */
- private static final long PLANNER_TIMEOUT =
getLong(IGNITE_CALCITE_PLANNER_TIMEOUT, 0);
+ private static final long PLANNER_TIMEOUT = 1000L;
Review Comment:
Why aren't you reading it from the system property?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/PlannerTimeoutTest.java:
##########
@@ -33,13 +33,24 @@
import org.apache.ignite.internal.processors.query.calcite.schema.IgniteSchema;
import
org.apache.ignite.internal.processors.query.calcite.trait.IgniteDistributions;
import org.apache.ignite.internal.processors.query.calcite.trait.TraitUtils;
+import org.apache.ignite.internal.util.CommonUtils;
import org.apache.ignite.testframework.GridTestUtils;
-import org.junit.Test;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Order;
+import org.junit.jupiter.api.Test;
/**
* Test planner timeout.
*/
+@Order(1)
public class PlannerTimeoutTest extends AbstractPlannerTest {
+ /** */
+ @BeforeAll
+ static void init() {
+ // Additional check for val correctness:
GridTestClockTimer#startTestTimer
+ assertEquals(1, (int)GridTestUtils.getFieldValue(CommonUtils.class,
"gridCnt"));
Review Comment:
Why do you perform the check this way instead of simply checking the number
of nodes say, via `org.apache.ignite.Ignition#allGrids`?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/ViewsIntegrationTest.java:
##########
@@ -51,9 +52,19 @@ public class ViewsIntegrationTest extends
AbstractMultiEngineIntegrationTest {
}
/** {@inheritDoc} */
- @Override protected void afterTest() throws Exception {
- super.afterTest();
+ @BeforeEach
+ @Override protected void beforeTest() throws Exception {
+ persistenceEnabled = false;
+
+ assert G.allGrids().isEmpty() : "Not all Ignite instances stopped
before tests execution:" + G.allGrids();
Review Comment:
I think it would be better to use `assertTrue`.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/QueryChecker.java:
##########
@@ -217,36 +217,56 @@ public static Matcher<String> containsSubPlan(String
subPlan) {
/** */
public static Matcher<String> matches(final String substring) {
- return new SubstringMatcher(substring) {
+ return new SubstringMatcher("contains", false, substring) {
/** {@inheritDoc} */
@Override protected boolean evalSubstringOf(String sIn) {
sIn = sIn.replaceAll("\n", "");
return sIn.matches(substring);
}
+ };
+ }
+ /** Matches only one occurrence. */
+ static Matcher<String> matchesOnce(String pattern) {
+ return occursTimes(pattern, 1, true);
+ }
+
+ /** Has specified number of occurrence. */
+ static Matcher<String> occursTimes(String pattern, int times, boolean
noMore) {
+ return new SubstringMatcher(resolveRelation(times, noMore), false,
pattern) {
/** {@inheritDoc} */
- @Override protected String relationship() {
- return null;
+ @Override protected boolean evalSubstringOf(String strIn) {
+ strIn = strIn.replaceAll(System.lineSeparator(), "");
+
+ return contains(strIn, substring, times, noMore);
}
};
}
- /** Matches only one occurance. */
- public static Matcher<String> matchesOnce(final String substring) {
- return new SubstringMatcher(substring) {
- /** {@inheritDoc} */
- @Override protected boolean evalSubstringOf(String sIn) {
- sIn = sIn.replaceAll("\n", "");
+ /** */
+ private static String resolveRelation(int times, boolean noMore) {
+ assert times >= 0;
Review Comment:
Please use `assertEquals` or something similar in the tests.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/QueryChecker.java:
##########
@@ -217,36 +217,56 @@ public static Matcher<String> containsSubPlan(String
subPlan) {
/** */
public static Matcher<String> matches(final String substring) {
- return new SubstringMatcher(substring) {
+ return new SubstringMatcher("contains", false, substring) {
/** {@inheritDoc} */
@Override protected boolean evalSubstringOf(String sIn) {
sIn = sIn.replaceAll("\n", "");
return sIn.matches(substring);
}
+ };
+ }
+ /** Matches only one occurrence. */
+ static Matcher<String> matchesOnce(String pattern) {
+ return occursTimes(pattern, 1, true);
+ }
+
+ /** Has specified number of occurrence. */
+ static Matcher<String> occursTimes(String pattern, int times, boolean
noMore) {
+ return new SubstringMatcher(resolveRelation(times, noMore), false,
pattern) {
/** {@inheritDoc} */
- @Override protected String relationship() {
- return null;
+ @Override protected boolean evalSubstringOf(String strIn) {
+ strIn = strIn.replaceAll(System.lineSeparator(), "");
+
+ return contains(strIn, substring, times, noMore);
}
};
}
- /** Matches only one occurance. */
- public static Matcher<String> matchesOnce(final String substring) {
- return new SubstringMatcher(substring) {
- /** {@inheritDoc} */
- @Override protected boolean evalSubstringOf(String sIn) {
- sIn = sIn.replaceAll("\n", "");
+ /** */
+ private static String resolveRelation(int times, boolean noMore) {
+ assert times >= 0;
- return containsOnce(sIn, substring);
- }
+ return switch (times) {
+ case 0 -> noMore ? "does not contain" : "can contain";
+ case 1 -> noMore ? "contains once" : "contains only once";
+ default -> (noMore ? "contains " : "contains only ") + times + "
times";
+ };
+ }
- /** {@inheritDoc} */
- @Override protected String relationship() {
- return null;
+ /** Check that {@code s} contains {@code substring} {@code times} times. */
+ static boolean contains(String s, CharSequence substring, int times,
boolean noMore) {
+ Pattern pattern = Pattern.compile(substring.toString());
+ java.util.regex.Matcher matcher = pattern.matcher(s);
Review Comment:
I think it could be implemented via indexOf, but that’s just for testing
purposes, so I won’t insist.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/tx/SqlTransactionsIsolationTest.java:
##########
@@ -222,12 +210,39 @@ public static Collection<?> parameters() {
}
/** {@inheritDoc} */
+ @BeforeEach
@Override protected void beforeTest() throws Exception {
+ partsToKeys.clear();
+
+ jdbcThinConn = ThreadLocal.withInitial(() -> {
Review Comment:
Does it make sense to update this field for every test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
