You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2022/10/06 14:51:16 UTC
[GitHub] [hadoop] aajisaka commented on a diff in pull request #4775: YARN-11260. Upgrade JUnit from 4 to 5 in hadoop-yarn-server-timelineservice
aajisaka commented on code in PR #4775:
URL: https://github.com/apache/hadoop/pull/4775#discussion_r989128859
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/test/java/org/apache/hadoop/yarn/server/timelineservice/reader/TestTimelineReaderWebServicesBasicAcl.java:
##########
@@ -127,25 +134,23 @@ public class TestTimelineReaderWebServicesBasicAcl {
TimelineReaderWebServices
.checkAccess(manager, adminUgi, entities, userKey, true);
// admin is allowed to view other entities
- Assert.assertTrue(entities.size() == 10);
+ assertTrue(entities.size() == 10);
Review Comment:
Could you replace with `assertEquals(10, entities.size())`?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/test/java/org/apache/hadoop/yarn/server/timelineservice/storage/TestFileSystemTimelineWriterImpl.java:
##########
@@ -191,13 +192,13 @@ public void testWriteMultipleEntities() throws Exception {
FileSystemTimelineWriterImpl.TIMELINE_SERVICE_STORAGE_EXTENSION;
Path path = new Path(fileName);
FileSystem fs = FileSystem.get(conf);
- assertTrue("Specified path(" + fileName + ") should exist: ",
- fs.exists(path));
+ assertTrue(fs.exists(path),
+ "Specified path(" + fileName + ") should exist: ");
FileStatus fileStatus = fs.getFileStatus(path);
- assertTrue("Specified path should be a file",
- !fileStatus.isDirectory());
+ assertTrue(!fileStatus.isDirectory(),
Review Comment:
Could you replace with `assertFalse(fileStatus.isDirectory(), ...)`?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/test/java/org/apache/hadoop/yarn/server/timelineservice/storage/TestFileSystemTimelineWriterImpl.java:
##########
@@ -278,4 +279,13 @@ private List<String> readFromFile(FileSystem fs, Path path)
}
return data;
}
+
+ private static File newFolder(File root, String... subDirs) throws IOException {
+ String subFolder = String.join("/", subDirs);
+ File result = new File(root, subFolder);
+ if (!result.mkdirs()) {
+ throw new IOException("Couldn't create folders " + root);
+ }
+ return result;
+ }
Review Comment:
This method is unused and can be removed.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/test/java/org/apache/hadoop/yarn/server/timelineservice/storage/TestFileSystemTimelineWriterImpl.java:
##########
@@ -191,13 +192,13 @@ public void testWriteMultipleEntities() throws Exception {
FileSystemTimelineWriterImpl.TIMELINE_SERVICE_STORAGE_EXTENSION;
Path path = new Path(fileName);
FileSystem fs = FileSystem.get(conf);
- assertTrue("Specified path(" + fileName + ") should exist: ",
- fs.exists(path));
+ assertTrue(fs.exists(path),
+ "Specified path(" + fileName + ") should exist: ");
FileStatus fileStatus = fs.getFileStatus(path);
- assertTrue("Specified path should be a file",
- !fileStatus.isDirectory());
+ assertTrue(!fileStatus.isDirectory(),
+ "Specified path should be a file");
List<String> data = readFromFile(fs, path);
- assertTrue("data size is:" + data.size(), data.size() == 3);
+ assertTrue(data.size() == 3, "data size is:" + data.size());
Review Comment:
Could you replace with `assertEquals(3, data.size(), ...)`?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/test/java/org/apache/hadoop/yarn/server/timelineservice/storage/TestFileSystemTimelineWriterImpl.java:
##########
@@ -248,13 +249,13 @@ public void testWriteEntitiesWithEmptyFlowName() throws Exception {
FileSystemTimelineWriterImpl.TIMELINE_SERVICE_STORAGE_EXTENSION;
Path path = new Path(fileName);
FileSystem fs = FileSystem.get(conf);
- assertTrue("Specified path(" + fileName + ") should exist: ",
- fs.exists(path));
+ assertTrue(fs.exists(path),
+ "Specified path(" + fileName + ") should exist: ");
FileStatus fileStatus = fs.getFileStatus(path);
- assertTrue("Specified path should be a file",
- !fileStatus.isDirectory());
+ assertTrue(!fileStatus.isDirectory(),
Review Comment:
Could you replace with `assertFalse(fileStatus.isDirectory(), ...)`?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/test/java/org/apache/hadoop/yarn/server/timelineservice/storage/TestFileSystemTimelineWriterImpl.java:
##########
@@ -248,13 +249,13 @@ public void testWriteEntitiesWithEmptyFlowName() throws Exception {
FileSystemTimelineWriterImpl.TIMELINE_SERVICE_STORAGE_EXTENSION;
Path path = new Path(fileName);
FileSystem fs = FileSystem.get(conf);
- assertTrue("Specified path(" + fileName + ") should exist: ",
- fs.exists(path));
+ assertTrue(fs.exists(path),
+ "Specified path(" + fileName + ") should exist: ");
FileStatus fileStatus = fs.getFileStatus(path);
- assertTrue("Specified path should be a file",
- !fileStatus.isDirectory());
+ assertTrue(!fileStatus.isDirectory(),
+ "Specified path should be a file");
List<String> data = readFromFile(fs, path);
- assertTrue("data size is:" + data.size(), data.size() == 2);
+ assertTrue(data.size() == 2, "data size is:" + data.size());
Review Comment:
Could you replace with `assertEquals(2, data.size())`?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/test/java/org/apache/hadoop/yarn/server/timelineservice/reader/TestTimelineReaderWebServicesUtils.java:
##########
@@ -32,20 +29,21 @@
import org.apache.hadoop.yarn.server.timelineservice.reader.filter.TimelineKeyValueFilter;
import org.apache.hadoop.yarn.server.timelineservice.reader.filter.TimelineKeyValuesFilter;
import org.apache.hadoop.yarn.server.timelineservice.reader.filter.TimelinePrefixFilter;
-import org.junit.Assert;
-import org.junit.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.fail;
public class TestTimelineReaderWebServicesUtils {
private static void verifyFilterList(String expr, TimelineFilterList list,
TimelineFilterList expectedList) throws Exception {
assertNotNull(list);
- assertTrue("Unexpected List received after parsing expression " + expr +
- ". Expected=" + expectedList + " but Actual=" + list,
- list.equals(expectedList));
+ assertEquals(list, expectedList);
Review Comment:
Thank you for the refactor using `assertEquals`.
- Would you reverse the argument order? The argument order is expected -> actual.
- After the refactoring, null check in `assertNotNull(list)` is not required and we can remove it. The null check is handled in `assertEquals`.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/test/java/org/apache/hadoop/yarn/server/timelineservice/reader/TestTimelineReaderWebServicesBasicAcl.java:
##########
@@ -127,25 +134,23 @@ public class TestTimelineReaderWebServicesBasicAcl {
TimelineReaderWebServices
.checkAccess(manager, adminUgi, entities, userKey, true);
// admin is allowed to view other entities
- Assert.assertTrue(entities.size() == 10);
+ assertTrue(entities.size() == 10);
// incoming ugi is user1Ugi asking for entities
// only user1 entities are allowed to view
entities = createEntities(5, userKey);
TimelineReaderWebServices
.checkAccess(manager, user1Ugi, entities, userKey, true);
- Assert.assertTrue(entities.size() == 1);
- Assert
- .assertEquals(user1, entities.iterator().next().getInfo().get(userKey));
+ assertTrue(entities.size() == 1);
Review Comment:
Could you replace with `assertEquals(1, entities.size())`?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/test/java/org/apache/hadoop/yarn/server/timelineservice/storage/TestFileSystemTimelineWriterImpl.java:
##########
@@ -127,14 +128,14 @@ public void testWriteEntityToFile() throws Exception {
File.separator + type2 + File.separator + id2 +
FileSystemTimelineWriterImpl.TIMELINE_SERVICE_STORAGE_EXTENSION;
Path path2 = new Path(fileName2);
- assertTrue("Specified path(" + fileName + ") should exist: ",
- fs.exists(path2));
+ assertTrue(fs.exists(path2),
+ "Specified path(" + fileName + ") should exist: ");
FileStatus fileStatus2 = fs.getFileStatus(path2);
- assertTrue("Specified path should be a file",
- !fileStatus2.isDirectory());
+ assertTrue(!fileStatus2.isDirectory(),
+ "Specified path should be a file");
List<String> data2 = readFromFile(fs, path2);
// ensure there's only one entity + 1 new line
- assertTrue("data size is:" + data2.size(), data2.size() == 2);
+ assertTrue(data2.size() == 2, "data size is:" + data2.size());
Review Comment:
Could you replace with `assertEquals`?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/test/java/org/apache/hadoop/yarn/server/timelineservice/storage/TestFileSystemTimelineWriterImpl.java:
##########
@@ -41,21 +42,21 @@
import org.apache.hadoop.yarn.conf.YarnConfiguration;
import org.apache.hadoop.yarn.server.timelineservice.collector.TimelineCollectorContext;
import org.apache.hadoop.yarn.util.timeline.TimelineUtils;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
public class TestFileSystemTimelineWriterImpl {
- @Rule
- public TemporaryFolder tmpFolder = new TemporaryFolder();
+ @TempDir
+ public File tmpFolder;
Review Comment:
Would you make this field private? `@TempDir` field can be private since JUnit 5.8.0 https://junit.org/junit5/docs/5.8.0/release-notes/index.html
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/test/java/org/apache/hadoop/yarn/server/timelineservice/storage/TestFileSystemTimelineWriterImpl.java:
##########
@@ -107,14 +108,14 @@ public void testWriteEntityToFile() throws Exception {
FileSystemTimelineWriterImpl.TIMELINE_SERVICE_STORAGE_EXTENSION;
Path path = new Path(fileName);
FileSystem fs = FileSystem.get(conf);
- assertTrue("Specified path(" + fileName + ") should exist: ",
- fs.exists(path));
+ assertTrue(fs.exists(path),
+ "Specified path(" + fileName + ") should exist: ");
FileStatus fileStatus = fs.getFileStatus(path);
- assertTrue("Specified path should be a file",
- !fileStatus.isDirectory());
+ assertTrue(!fileStatus.isDirectory(),
+ "Specified path should be a file");
List<String> data = readFromFile(fs, path);
// ensure there's only one entity + 1 new line
- assertTrue("data size is:" + data.size(), data.size() == 2);
+ assertTrue(data.size() == 2, "data size is:" + data.size());
Review Comment:
Could you replace with `assertEquals`?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice/src/test/java/org/apache/hadoop/yarn/server/timelineservice/reader/TestTimelineReaderWebServicesBasicAcl.java:
##########
@@ -127,25 +134,23 @@ public class TestTimelineReaderWebServicesBasicAcl {
TimelineReaderWebServices
.checkAccess(manager, adminUgi, entities, userKey, true);
// admin is allowed to view other entities
- Assert.assertTrue(entities.size() == 10);
+ assertTrue(entities.size() == 10);
// incoming ugi is user1Ugi asking for entities
// only user1 entities are allowed to view
entities = createEntities(5, userKey);
TimelineReaderWebServices
.checkAccess(manager, user1Ugi, entities, userKey, true);
- Assert.assertTrue(entities.size() == 1);
- Assert
- .assertEquals(user1, entities.iterator().next().getInfo().get(userKey));
+ assertTrue(entities.size() == 1);
+ assertEquals(user1, entities.iterator().next().getInfo().get(userKey));
// incoming ugi is user2Ugi asking for entities
// only user2 entities are allowed to view
entities = createEntities(8, userKey);
TimelineReaderWebServices
.checkAccess(manager, user2Ugi, entities, userKey, true);
- Assert.assertTrue(entities.size() == 1);
- Assert
- .assertEquals(user2, entities.iterator().next().getInfo().get(userKey));
+ assertTrue(entities.size() == 1);
Review Comment:
Could you replace with `assertEquals(1, entities.size())`?
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org