[
https://issues.apache.org/jira/browse/YARN-11260?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17613583#comment-17613583
]
ASF GitHub Bot commented on YARN-11260:
---------------------------------------
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())`?
> Upgrade JUnit from 4 to 5 in hadoop-yarn-server-timelineservice
> ---------------------------------------------------------------
>
> Key: YARN-11260
> URL: https://issues.apache.org/jira/browse/YARN-11260
> Project: Hadoop YARN
> Issue Type: Sub-task
> Affects Versions: 3.3.4
> Reporter: Ashutosh Gupta
> Assignee: Ashutosh Gupta
> Priority: Major
> Labels: pull-request-available
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
