phet commented on code in PR #3616:
URL: https://github.com/apache/gobblin/pull/3616#discussion_r1095350586
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -402,8 +423,29 @@ public void testCommit() throws IOException {
// previously existing paths should not have permissions changed
fileStatus = this.fs.getFileStatus(existingOutputPath);
Assert.assertEquals(fileStatus.getPermission(), existingPathPermission);
-
+ verifyAclEntries(writer, pathToAclEntries, expectedOutputPath);
Assert.assertFalse(this.fs.exists(writer.stagingDir));
+
+ }
+
+ private void verifyAclEntries(FileAwareInputStreamDataWriter writer,
ConcurrentHashMap pathToAclEntries, Path expectedOutputPath) {
+ // fetching and preparing file paths from FileAwareInputStreamDataWriter
object
+ Path stagingDir = writer.stagingDir;
+ Path outputDir = writer.outputDir;
+ String[] splitExpectedOutputPath =
expectedOutputPath.toString().split("output");
+ Path dstOutputPath = new
Path(outputDir.toString().concat(splitExpectedOutputPath[1])).getParent();
+ Path stgFilePath = new
Path("file:".concat(stagingDir.toString().concat("/file")));
Review Comment:
don't think I've ever actually used the `String.concat` method before...
what about the canonical `+`? is there some advantage, since:
```
new Path("file:" + stagingDir + "/file");
```
seems easier to read?
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -402,8 +423,29 @@ public void testCommit() throws IOException {
// previously existing paths should not have permissions changed
fileStatus = this.fs.getFileStatus(existingOutputPath);
Assert.assertEquals(fileStatus.getPermission(), existingPathPermission);
-
+ verifyAclEntries(writer, pathToAclEntries, expectedOutputPath);
Assert.assertFalse(this.fs.exists(writer.stagingDir));
+
+ }
+
+ private void verifyAclEntries(FileAwareInputStreamDataWriter writer,
ConcurrentHashMap pathToAclEntries, Path expectedOutputPath) {
+ // fetching and preparing file paths from FileAwareInputStreamDataWriter
object
+ Path stagingDir = writer.stagingDir;
Review Comment:
not immediately seeing... why do we need to verify staging? isn't dest the
only one that matters?
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -65,17 +70,22 @@
import org.apache.gobblin.util.WriterUtils;
import org.apache.gobblin.util.io.StreamUtils;
-import static org.mockito.Mockito.*;
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.doCallRealMethod;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.spy;
public class FileAwareInputStreamDataWriterTest {
FileSystem fs;
Path testTempPath;
+ public static ConcurrentHashMap<Path, List<AclEntry>> pathToAclEntries;
Review Comment:
I mentioned before and still advise not to use `static` / class state in
unit tests, since parallel test runners may give odd results.
also, consider: if `fs` is instance-level and `pathToAclEntries` is here to
augment that, why wouldn't we want that instance-level too?
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -481,4 +523,11 @@ public void cleanup() {
}
}
-}
+ protected static class TestLocalFileSystem extends LocalFileSystem {
Review Comment:
why not encapsulate the `ConcurrentHashMap` here? just add a method to
access it for verification (copying-on-write would be most typical).
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -402,8 +423,29 @@ public void testCommit() throws IOException {
// previously existing paths should not have permissions changed
fileStatus = this.fs.getFileStatus(existingOutputPath);
Assert.assertEquals(fileStatus.getPermission(), existingPathPermission);
-
+ verifyAclEntries(writer, pathToAclEntries, expectedOutputPath);
Review Comment:
reqs-wise, how does this feature treat ACLs on parent dirs that it's
creating--will they be preserved or not? a unit test should codify "the spec".
hence, either way, shouldn't we exercise that scenario to assert it's handled
in the proper way?
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/copy/writer/FileAwareInputStreamDataWriterTest.java:
##########
@@ -380,12 +401,12 @@ public void testCommit() throws IOException {
FileStatus fileStatus = this.fs.getFileStatus(existingOutputPath);
FsPermission existingPathPermission = fileStatus.getPermission();
+
Review Comment:
snuck in?
--
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]
