[GitHub] [ignite-3] rpuch commented on a diff in pull request #2531: IGNITE-20328 Cleanup in BaseIgniteAbstractTest

["rpuch (via GitHub)" <gi...@apache.org>]

rpuch commented on code in PR #2531:
URL: https://github.com/apache/ignite-3/pull/2531#discussion_r1312851800


##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/CliCommandTestNotInitializedIntegrationBase.java:
##########
@@ -80,16 +80,8 @@ public class CliCommandTestNotInitializedIntegrationBase extends CliIntegrationT
     @Inject
     private EventListeningActivationPoint eventListeningActivationPoint;
 
-    /**
-     * Invokes before the test will start.
-     *
-     * @param testInfo Test information object.
-     * @throws Exception If failed.
-     */
-    @Override
     @BeforeEach
-    public void setUp(TestInfo testInfo) throws Exception {
-        super.setUp(testInfo);
+    public void setUp() {

Review Comment:
   A method with such  a general and widely used name should probably be `final` to make sure an unintentional override is not possible



##########
modules/core/src/testFixtures/java/org/apache/ignite/internal/testframework/BaseIgniteAbstractTest.java:
##########
@@ -39,67 +40,55 @@
 @ExtendWith(SystemPropertiesExtension.class)
 public abstract class BaseIgniteAbstractTest {
     /** Logger. */
-    protected static IgniteLogger log;
+    protected final IgniteLogger log = Loggers.forClass(getClass());
 
     /** Test start time in milliseconds. */
     private long testStartMs;
 
-    static {
-        /* Init test env. */
-        S.setSensitiveDataLoggingPolicySupplier(() ->
-                SensitiveDataLoggingPolicy.valueOf(getString(IGNITE_SENSITIVE_DATA_LOGGING, "hash").toUpperCase()));
+    @BeforeAll
+    static void setLoggingPolicy() {
+        S.setSensitiveDataLoggingPolicySupplier(() -> {
+            String loggingPolicy = getString(IGNITE_SENSITIVE_DATA_LOGGING, "hash");
+
+            return SensitiveDataLoggingPolicy.valueOf(loggingPolicy.toUpperCase());
+        });
     }
 
     /**
-     * Should be invoked before a test will start.
-     *
-     * @param testInfo Test information object.
-     * @param workDir Work directory.
+     * Prevents accidental leaks from Mockito.
      */
-    protected void setupBase(TestInfo testInfo, @Nullable Path workDir) {
-        log.info(">>> Starting test: {}#{}, displayName: {}, workDir: {}",
+    @AfterAll
+    static void clearInlineMocks() {
+        framework().clearInlineMocks();

Review Comment:
   Let's qualify this as `Mockito.framework()`, otherwise it's not clear what framework it is



##########
modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/AbstractJdbcSelfTest.java:
##########
@@ -115,9 +115,7 @@ public static void afterAllBase(TestInfo testInfo) throws Exception {
     }
 
     @BeforeEach
-    protected void beforeTest(TestInfo testInfo) throws Exception {
-        setupBase(testInfo, WORK_DIR);
-
+    protected void setUpBase() throws Exception {

Review Comment:
   Why is it `protected`?



##########
modules/raft/src/integrationTest/java/org/apache/ignite/raft/jraft/core/ItNodeTest.java:
##########
@@ -3770,8 +3772,6 @@ private static boolean waitForTopology(TestCluster cluster, PeerId peerId, int e
         RaftGroupService grp = cluster.getServer(peerId);
 
         if (grp == null) {
-            log.warn("Node has not been found {}", peerId);

Review Comment:
   Why is this removed?



##########
modules/core/src/test/java/org/apache/ignite/internal/tostring/IgniteToStringBuilderSelfTest.java:
##########
@@ -807,7 +807,7 @@ public String toString() {
      * Class sleeps a short quanta of time to increase chances of data race between {@link IgniteToStringBuilder} iterating over collection
      * user thread concurrently modifying it.
      */
-    private static class SlowToStringObject {

Review Comment:
   Why isn't it static anymore?



##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItCorrelatesTest.java:
##########
@@ -21,24 +21,14 @@
 
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.TestInfo;
 
 /** Tests for correlated queries. */
 public class ItCorrelatesTest extends ClusterPerClassIntegrationTest {
     private static final String DISABLED_JOIN_RULES = " /*+ DISABLE_RULE('MergeJoinConverter', 'NestedLoopJoinConverter') */ ";
 
-    /**
-     * Clear tables after each test.
-     *
-     * @param testInfo Test information object.
-     * @throws Exception If failed.
-     */
     @AfterEach
-    @Override
-    public void tearDown(TestInfo testInfo) throws Exception {
+    public void clearTables() {

Review Comment:
   It's dropping, not clearing. Probably the name is not ideal



##########
modules/core/src/testFixtures/java/org/apache/ignite/internal/testframework/BaseIgniteAbstractTest.java:
##########
@@ -39,67 +40,55 @@
 @ExtendWith(SystemPropertiesExtension.class)
 public abstract class BaseIgniteAbstractTest {
     /** Logger. */
-    protected static IgniteLogger log;
+    protected final IgniteLogger log = Loggers.forClass(getClass());
 
     /** Test start time in milliseconds. */
     private long testStartMs;
 
-    static {
-        /* Init test env. */
-        S.setSensitiveDataLoggingPolicySupplier(() ->
-                SensitiveDataLoggingPolicy.valueOf(getString(IGNITE_SENSITIVE_DATA_LOGGING, "hash").toUpperCase()));
+    @BeforeAll
+    static void setLoggingPolicy() {
+        S.setSensitiveDataLoggingPolicySupplier(() -> {

Review Comment:
   So we do it for each test class. Is this what we want?



-- 
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: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org