You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "raju-balpande (via GitHub)" <gi...@apache.org> on 2023/11/07 14:33:29 UTC
[PR] HDDS-9498. Migrate tests with TemporaryFolder in ozone-recon to JUnit5 [ozone]
raju-balpande opened a new pull request, #5556:
URL: https://github.com/apache/ozone/pull/5556
## What changes were proposed in this pull request?
Migrate the following test classes to JUnit5.
Please describe your PR in detail:
Migrate the following test classes to JUnit5. See parent task for details.
- As a first batch modified following files.
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/TestReconUtils.java
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestBlocksEndPoint.java
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestContainerEndpoint.java
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestFeaturesEndPoint.java
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestNSSummaryEndpointWithFSO.java
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestNSSummaryEndpointWithLegacy.java
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestOmDBInsightEndPoint.java
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestOpenContainerCount.java
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestTriggerDBSyncEndpoint.java
- When making the changes for `TestContainerStateCounts` which extends `AbstractReconSqlDBTest`. so covered some sub-clasess as well.
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestContainerStateCounts.java
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestEndpoints.java
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/tasks/TestOmTableInsightTask.java
-Once these are reviewed will cover the rest of classes in second batch in another PR.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9498
## How was this patch tested?
Tested this patch by manual tests
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9498. Migrate tests with TemporaryFolder in ozone-recon to JUnit5 [ozone]
Posted by "Galsza (via GitHub)" <gi...@apache.org>.
Galsza commented on code in PR #5556:
URL: https://github.com/apache/ozone/pull/5556#discussion_r1393190844
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/tasks/TestOmTableInsightTask.java:
##########
@@ -75,8 +79,9 @@ public class TestOmTableInsightTask extends AbstractReconSqlDBTest {
private void initializeInjector() throws IOException {
reconOMMetadataManager = getTestReconOmMetadataManager(
- initializeNewOmMetadataManager(temporaryFolder.newFolder()),
- temporaryFolder.newFolder());
+ initializeNewOmMetadataManager(Files.createDirectory(
+ temporaryFolder.resolve("JunitOmDBDir")).toFile()),
Review Comment:
Thank you Attila. LGTM
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9498. Migrate tests with TemporaryFolder in ozone-recon to JUnit5 [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5556:
URL: https://github.com/apache/ozone/pull/5556#discussion_r1390254772
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/persistence/AbstractReconSqlDBTest.java:
##########
@@ -36,52 +38,51 @@
import org.jooq.SQLDialect;
import org.jooq.impl.DSL;
import org.jooq.impl.DefaultConfiguration;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Rule;
+import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
-import org.junit.rules.TemporaryFolder;
import com.google.inject.AbstractModule;
import com.google.inject.Guice;
import com.google.inject.Injector;
import com.google.inject.Module;
import com.google.inject.Provider;
+import org.junit.jupiter.api.io.TempDir;
/**
* Class that provides a Recon SQL DB with all the tables created, and APIs
* to access the DAOs easily.
*/
public class AbstractReconSqlDBTest {
- @Rule
- public TemporaryFolder temporaryFolder = new TemporaryFolder();
+ @TempDir
+ protected Path temporaryFolder;
private Injector injector;
private DSLContext dslContext;
private Provider<DataSourceConfiguration> configurationProvider;
public AbstractReconSqlDBTest() {
try {
- temporaryFolder.create();
+ temporaryFolder = Files.createTempDirectory("JunitConfig");
Review Comment:
Good point. But then we don't need the `@TempDir` annotation, some constructor is called for all subclasses.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9498. Migrate tests with TemporaryFolder in ozone-recon to JUnit5 [ozone]
Posted by "raju-balpande (via GitHub)" <gi...@apache.org>.
raju-balpande commented on code in PR #5556:
URL: https://github.com/apache/ozone/pull/5556#discussion_r1389793190
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestContainerEndpoint.java:
##########
@@ -759,7 +760,7 @@ public void testGetMissingContainers() throws IOException, TimeoutException {
new HashSet<>(Arrays.asList("host2", "host3", "host4")));
List<ContainerHistory> containerReplicas = container.getReplicas();
containerReplicas.forEach(history -> {
- Assert.assertTrue(datanodes.contains(history.getDatanodeHost()));
+ Assertions.assertTrue(datanodes.contains(history.getDatanodeHost()));
Review Comment:
Good catch. modified these changes.
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/spi/impl/TestReconDBProvider.java:
##########
@@ -41,18 +41,18 @@
*/
public class TestReconDBProvider {
- @Rule
- public TemporaryFolder tempFolder = new TemporaryFolder();
+ @TempDir
+ private Path temporaryFolder;
private Injector injector;
- @Before
+ @BeforeEach
public void setUp() throws IOException {
- tempFolder.create();
+ //tempFolder.create();
Review Comment:
done.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9498. Migrate tests with TemporaryFolder in ozone-recon to JUnit5 [ozone]
Posted by "Galsza (via GitHub)" <gi...@apache.org>.
Galsza commented on code in PR #5556:
URL: https://github.com/apache/ozone/pull/5556#discussion_r1392216827
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/tasks/TestOmTableInsightTask.java:
##########
@@ -75,8 +79,9 @@ public class TestOmTableInsightTask extends AbstractReconSqlDBTest {
private void initializeInjector() throws IOException {
reconOMMetadataManager = getTestReconOmMetadataManager(
- initializeNewOmMetadataManager(temporaryFolder.newFolder()),
- temporaryFolder.newFolder());
+ initializeNewOmMetadataManager(Files.createDirectory(
+ temporaryFolder.resolve("JunitOmDBDir")).toFile()),
Review Comment:
Could this cause data leakage to other tests? initializeInjector is called before each test where we create a directory with the same name each time.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9498. Migrate tests with TemporaryFolder in ozone-recon to JUnit5 [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5556:
URL: https://github.com/apache/ozone/pull/5556#issuecomment-1806867119
Thanks @raju-balpande for updating the patch. Please also check failure of `TestReconWithDifferentSqlDBs`:
https://github.com/raju-balpande/apache_ozone/actions/runs/6828560052/job/18573158100#step:5:3023
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9498. Migrate tests with TemporaryFolder in ozone-recon to JUnit5 [ozone]
Posted by "raju-balpande (via GitHub)" <gi...@apache.org>.
raju-balpande commented on code in PR #5556:
URL: https://github.com/apache/ozone/pull/5556#discussion_r1389792798
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/persistence/AbstractReconSqlDBTest.java:
##########
@@ -36,52 +38,51 @@
import org.jooq.SQLDialect;
import org.jooq.impl.DSL;
import org.jooq.impl.DefaultConfiguration;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Rule;
+import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
-import org.junit.rules.TemporaryFolder;
import com.google.inject.AbstractModule;
import com.google.inject.Guice;
import com.google.inject.Injector;
import com.google.inject.Module;
import com.google.inject.Provider;
+import org.junit.jupiter.api.io.TempDir;
/**
* Class that provides a Recon SQL DB with all the tables created, and APIs
* to access the DAOs easily.
*/
public class AbstractReconSqlDBTest {
- @Rule
- public TemporaryFolder temporaryFolder = new TemporaryFolder();
+ @TempDir
+ protected Path temporaryFolder;
private Injector injector;
private DSLContext dslContext;
private Provider<DataSourceConfiguration> configurationProvider;
public AbstractReconSqlDBTest() {
try {
- temporaryFolder.create();
+ temporaryFolder = Files.createTempDirectory("JunitConfig");
configurationProvider =
- new DerbyDataSourceConfigurationProvider(temporaryFolder.newFolder());
+ new DerbyDataSourceConfigurationProvider(Files.createDirectory(
+ temporaryFolder.resolve("Config")).toFile());
} catch (IOException e) {
- Assert.fail();
+ Assertions.fail();
}
}
protected AbstractReconSqlDBTest(Provider<DataSourceConfiguration> provider) {
try {
- temporaryFolder.create();
+ temporaryFolder = Files.createTempDirectory("JunitConfig");
Review Comment:
I checked, the temporary folders been created are not being used by subclasses except class TestReconWithDifferentSqlDBs which uses the this constructor with parameter provider. May be a refactoring ticket be created to address such changes. For now, just because it is inside constructor so will need to create the folders.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9498. Migrate tests with TemporaryFolder in ozone-recon to JUnit5 [ozone]
Posted by "raju-balpande (via GitHub)" <gi...@apache.org>.
raju-balpande commented on code in PR #5556:
URL: https://github.com/apache/ozone/pull/5556#discussion_r1389798426
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/persistence/AbstractReconSqlDBTest.java:
##########
@@ -36,52 +38,51 @@
import org.jooq.SQLDialect;
import org.jooq.impl.DSL;
import org.jooq.impl.DefaultConfiguration;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Rule;
+import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
-import org.junit.rules.TemporaryFolder;
import com.google.inject.AbstractModule;
import com.google.inject.Guice;
import com.google.inject.Injector;
import com.google.inject.Module;
import com.google.inject.Provider;
+import org.junit.jupiter.api.io.TempDir;
/**
* Class that provides a Recon SQL DB with all the tables created, and APIs
* to access the DAOs easily.
*/
public class AbstractReconSqlDBTest {
- @Rule
- public TemporaryFolder temporaryFolder = new TemporaryFolder();
+ @TempDir
+ protected Path temporaryFolder;
Review Comment:
I made it private, surprisingly the subclasses are creating their own temporary folders and hence were not dependent on folders been create in Abstract class. (Except TestReconWithDifferentSqlDBs which uses its super constructor but does not use the variable temporaryFolder)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9498. Migrate tests with TemporaryFolder in ozone-recon to JUnit5 [ozone]
Posted by "raju-balpande (via GitHub)" <gi...@apache.org>.
raju-balpande commented on PR #5556:
URL: https://github.com/apache/ozone/pull/5556#issuecomment-1804070923
Hi @adoroszlai , I see the earlier the access modifier for temporaryFolder in AbstractReconSqlDBTest was public.
`public TemporaryFolder temporaryFolder = new TemporaryFolder();` which I modified to protected (more restricted)
`protected Path temporaryFolder;`
But the check-style error is showing it to be made to private. I further checked that the abstract class is extended by 14 classes which leads impact.
<img width="946" alt="image" src="https://github.com/apache/ozone/assets/146973984/544bd976-f1d7-4757-884c-d5e682f2218c">
Please suggest if protected is acceptable to shall I make it private and introduce a getter method which lead the changes in all 14 sub-classes.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9498. Migrate tests with TemporaryFolder in ozone-recon to JUnit5 [ozone]
Posted by "raju-balpande (via GitHub)" <gi...@apache.org>.
raju-balpande commented on code in PR #5556:
URL: https://github.com/apache/ozone/pull/5556#discussion_r1389692662
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/persistence/AbstractReconSqlDBTest.java:
##########
@@ -36,52 +38,51 @@
import org.jooq.SQLDialect;
import org.jooq.impl.DSL;
import org.jooq.impl.DefaultConfiguration;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Rule;
+import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
-import org.junit.rules.TemporaryFolder;
import com.google.inject.AbstractModule;
import com.google.inject.Guice;
import com.google.inject.Injector;
import com.google.inject.Module;
import com.google.inject.Provider;
+import org.junit.jupiter.api.io.TempDir;
/**
* Class that provides a Recon SQL DB with all the tables created, and APIs
* to access the DAOs easily.
*/
public class AbstractReconSqlDBTest {
- @Rule
- public TemporaryFolder temporaryFolder = new TemporaryFolder();
+ @TempDir
+ protected Path temporaryFolder;
private Injector injector;
private DSLContext dslContext;
private Provider<DataSourceConfiguration> configurationProvider;
public AbstractReconSqlDBTest() {
try {
- temporaryFolder.create();
+ temporaryFolder = Files.createTempDirectory("JunitConfig");
Review Comment:
Yes it is created automatically, but this is different case. It is not created before constructors and hence will need to create it. I have tested this scenario.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9498. Migrate tests with TemporaryFolder in ozone-recon to JUnit5 [ozone]
Posted by "raju-balpande (via GitHub)" <gi...@apache.org>.
raju-balpande commented on PR #5556:
URL: https://github.com/apache/ozone/pull/5556#issuecomment-1808453429
> Thanks @raju-balpande for updating the patch. Please also check failure of `TestReconWithDifferentSqlDBs`:
>
> https://github.com/raju-balpande/apache_ozone/actions/runs/6828560052/job/18573158100#step:5:3023
In order to resolve the problem in TestReconWithDifferentSqlDBs,
I noticed that the AbstractReconSqlDBTest is not the abstract class but it's functionality is extended by the subclasses. Under Junit 5 we can parameterize the method but the constructor can not be parameterized and hence, I parameterized the required method and and created the object of AbstractReconSqlDBTest instead of extending it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9498. Migrate tests with TemporaryFolder in ozone-recon to JUnit5 [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5556:
URL: https://github.com/apache/ozone/pull/5556#discussion_r1391446987
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/persistence/AbstractReconSqlDBTest.java:
##########
@@ -53,35 +52,28 @@
* to access the DAOs easily.
*/
public class AbstractReconSqlDBTest {
-
- @Rule
- public TemporaryFolder temporaryFolder = new TemporaryFolder();
+ private Path temporaryFolder;
private Injector injector;
private DSLContext dslContext;
private Provider<DataSourceConfiguration> configurationProvider;
public AbstractReconSqlDBTest() {
try {
- temporaryFolder.create();
+ temporaryFolder = Files.createTempDirectory("JunitConfig");
configurationProvider =
- new DerbyDataSourceConfigurationProvider(temporaryFolder.newFolder());
+ new DerbyDataSourceConfigurationProvider(Files.createDirectory(
+ temporaryFolder.resolve("Config")).toFile());
} catch (IOException e) {
- Assert.fail();
+ Assertions.fail();
}
}
protected AbstractReconSqlDBTest(Provider<DataSourceConfiguration> provider) {
- try {
- temporaryFolder.create();
configurationProvider = provider;
Review Comment:
```suggestion
configurationProvider = provider;
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9498. Migrate tests with TemporaryFolder in ozone-recon to JUnit5 [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5556:
URL: https://github.com/apache/ozone/pull/5556#discussion_r1388200727
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/persistence/AbstractReconSqlDBTest.java:
##########
@@ -36,52 +38,51 @@
import org.jooq.SQLDialect;
import org.jooq.impl.DSL;
import org.jooq.impl.DefaultConfiguration;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Rule;
+import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
-import org.junit.rules.TemporaryFolder;
import com.google.inject.AbstractModule;
import com.google.inject.Guice;
import com.google.inject.Injector;
import com.google.inject.Module;
import com.google.inject.Provider;
+import org.junit.jupiter.api.io.TempDir;
/**
* Class that provides a Recon SQL DB with all the tables created, and APIs
* to access the DAOs easily.
*/
public class AbstractReconSqlDBTest {
- @Rule
- public TemporaryFolder temporaryFolder = new TemporaryFolder();
+ @TempDir
+ protected Path temporaryFolder;
private Injector injector;
private DSLContext dslContext;
private Provider<DataSourceConfiguration> configurationProvider;
public AbstractReconSqlDBTest() {
try {
- temporaryFolder.create();
+ temporaryFolder = Files.createTempDirectory("JunitConfig");
Review Comment:
`@TempDir temporaryFolder` is created automatically by JUnit. Do not create it manually, that might interfere with automatic cleanup.
```suggestion
```
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestContainerEndpoint.java:
##########
@@ -759,7 +760,7 @@ public void testGetMissingContainers() throws IOException, TimeoutException {
new HashSet<>(Arrays.asList("host2", "host3", "host4")));
List<ContainerHistory> containerReplicas = container.getReplicas();
containerReplicas.forEach(history -> {
- Assert.assertTrue(datanodes.contains(history.getDatanodeHost()));
+ Assertions.assertTrue(datanodes.contains(history.getDatanodeHost()));
Review Comment:
`assertTrue` is imported, can be used directly.
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/persistence/AbstractReconSqlDBTest.java:
##########
@@ -36,52 +38,51 @@
import org.jooq.SQLDialect;
import org.jooq.impl.DSL;
import org.jooq.impl.DefaultConfiguration;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Rule;
+import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
-import org.junit.rules.TemporaryFolder;
import com.google.inject.AbstractModule;
import com.google.inject.Guice;
import com.google.inject.Injector;
import com.google.inject.Module;
import com.google.inject.Provider;
+import org.junit.jupiter.api.io.TempDir;
/**
* Class that provides a Recon SQL DB with all the tables created, and APIs
* to access the DAOs easily.
*/
public class AbstractReconSqlDBTest {
- @Rule
- public TemporaryFolder temporaryFolder = new TemporaryFolder();
+ @TempDir
+ protected Path temporaryFolder;
Review Comment:
> shall I make it private and introduce a getter method which lead the changes in all 14 sub-classes
Yes, please make it `private`. It only affects 4 subclasses.
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/persistence/AbstractReconSqlDBTest.java:
##########
@@ -36,52 +38,51 @@
import org.jooq.SQLDialect;
import org.jooq.impl.DSL;
import org.jooq.impl.DefaultConfiguration;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Rule;
+import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
-import org.junit.rules.TemporaryFolder;
import com.google.inject.AbstractModule;
import com.google.inject.Guice;
import com.google.inject.Injector;
import com.google.inject.Module;
import com.google.inject.Provider;
+import org.junit.jupiter.api.io.TempDir;
/**
* Class that provides a Recon SQL DB with all the tables created, and APIs
* to access the DAOs easily.
*/
public class AbstractReconSqlDBTest {
- @Rule
- public TemporaryFolder temporaryFolder = new TemporaryFolder();
+ @TempDir
+ protected Path temporaryFolder;
private Injector injector;
private DSLContext dslContext;
private Provider<DataSourceConfiguration> configurationProvider;
public AbstractReconSqlDBTest() {
try {
- temporaryFolder.create();
+ temporaryFolder = Files.createTempDirectory("JunitConfig");
configurationProvider =
- new DerbyDataSourceConfigurationProvider(temporaryFolder.newFolder());
+ new DerbyDataSourceConfigurationProvider(Files.createDirectory(
+ temporaryFolder.resolve("Config")).toFile());
} catch (IOException e) {
- Assert.fail();
+ Assertions.fail();
}
}
protected AbstractReconSqlDBTest(Provider<DataSourceConfiguration> provider) {
try {
- temporaryFolder.create();
+ temporaryFolder = Files.createTempDirectory("JunitConfig");
Review Comment:
Same here, do not create manually.
```suggestion
```
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/spi/impl/TestReconDBProvider.java:
##########
@@ -41,18 +41,18 @@
*/
public class TestReconDBProvider {
- @Rule
- public TemporaryFolder tempFolder = new TemporaryFolder();
+ @TempDir
+ private Path temporaryFolder;
private Injector injector;
- @Before
+ @BeforeEach
public void setUp() throws IOException {
- tempFolder.create();
+ //tempFolder.create();
Review Comment:
```suggestion
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9498. Migrate tests with TemporaryFolder in ozone-recon to JUnit5 [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5556:
URL: https://github.com/apache/ozone/pull/5556#issuecomment-1799336099
Thanks @raju-balpande for working on this. I'll trigger the workflow when all tests are migrated.
Using more descriptive commit message for each commit would be helpful (for both you and reviewers).
I see that you have cancelled CI in your fork for these commits. You can avoid the need for that by adding `[skip ci]` in the commit message to skip CI checks.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9498. Migrate tests with TemporaryFolder in ozone-recon to JUnit5 [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5556:
URL: https://github.com/apache/ozone/pull/5556#issuecomment-1808955204
@Galsza @VarshaRaviCV please review
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9498. Migrate tests with TemporaryFolder in ozone-recon to JUnit5 [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5556:
URL: https://github.com/apache/ozone/pull/5556#issuecomment-1811177709
Thanks @raju-balpande for the patch, @Galsza for the review.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9498. Migrate tests with TemporaryFolder in ozone-recon to JUnit5 [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #5556:
URL: https://github.com/apache/ozone/pull/5556
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9498. Migrate tests with TemporaryFolder in ozone-recon to JUnit5 [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #5556:
URL: https://github.com/apache/ozone/pull/5556#discussion_r1393113975
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/tasks/TestOmTableInsightTask.java:
##########
@@ -75,8 +79,9 @@ public class TestOmTableInsightTask extends AbstractReconSqlDBTest {
private void initializeInjector() throws IOException {
reconOMMetadataManager = getTestReconOmMetadataManager(
- initializeNewOmMetadataManager(temporaryFolder.newFolder()),
- temporaryFolder.newFolder());
+ initializeNewOmMetadataManager(Files.createDirectory(
+ temporaryFolder.resolve("JunitOmDBDir")).toFile()),
Review Comment:
`temporaryFolder` is a `@TempDir`, so the path is different for each test case. Example:
```
/tmp/junit5823932975861174146/JunitOmDBDir
/tmp/junit842977494714222952/JunitOmDBDir
/tmp/junit2826054961670227457/JunitOmDBDir
/tmp/junit6353237819497228942/JunitOmDBDir
/tmp/junit1432827117522397492/JunitOmDBDir
/tmp/junit4588903889103211047/JunitOmDBDir
/tmp/junit8280824666979319137/JunitOmDBDir
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org