Issue 58930043: FSA Add NioFileDelegateTest (Closed)
|
Created:
10 years, 3 months ago by Brett Modified:
10 years, 3 months ago CC:
connector-cr_google.com Visibility:
Public. |
DescriptionFSA Add NioFileDelegateTest
Patch Set 1 #
Total comments: 18
Patch Set 2 : Eric's feedback #
Total comments: 1
Patch Set 3 : Eric's feedback #
Total comments: 14
Patch Set 4 : Replace TempFiles with JUnit TempFolders Rule #MessagesTotal messages: 15
Looks pretty good, just a hair shy. https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/NioFileDelegateTest.java (right): https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/NioFileDelegateTest.java:45: public void setUp() throws Exception { No need for setUp(). Set these as part of their declaration. https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/NioFileDelegateTest.java:132: assertEquals(content, CharStreams.toString(new InputStreamReader( Note that in the future you can use assertArrayEquals() to compare the byte[]s. https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/NioFileDelegateTest.java:183: static class TestNioFileDelegate extends NioFileDelegate { This can be private. https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/TempFiles.java (right): https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/TempFiles.java:31: * This functionality was removed from Guava. Where in Guava? I can't find anything very similar to this. IoTestCase is the closest I found, and this seems more "inspired by" Guava than a copy. By "removed from Guava", you mean that it used to be a class and was removed, or that you copied it from Guava? https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/TempFiles.java:44: TempFiles(String prefix) throws IOException { On these package-private classes (or a class with non-public visibility), I find it generally helpful to use the public/private/package-protected almost as if it was public. I'm mentioning it now because it has become a recurring theme. Although it has no impact on true visibility, seeing something as "public" means that it is part of the API that the class exposes, and the fact that we are choosing to expose the class is completely orthogonal. Lacking the public identifier actively makes the code harder to read, because I have to wonder whether this method is part of the general API. There are cases where that sort of thing doesn't apply (generally with inner classes that interact very closely with the outer class or are structs), but this class would seem to benefit as it has a very clearly defined API, strong cohesion, and low coupling. https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/TempFiles.java:54: Path newTempFile() throws IOException { Although I don't want to pick on this class too much because it is a pretty reasonable helper class, the organization of the overloading is problematic for me. The fact that 'parent' is the "most optional" of the arguments is unexpected/hard to read given the positioning of the arguments and the hierarchical nature of parent directory. Given the actual usages, it doesn't even seem to be any gain to make tempRoot the most optional. Based on my count, you have null in just as many method calls using newTempFile(), newTempFile(String, String), newTempFile(Path, String, String) as newTempFile(), newTempFile(Path), newTempFile(Path, String, String). And newTempDir() is the only variant that is used, so it is not impacted by such a change. It is "fine" how it is now, but honestly my eyes cross each time I am looking at the arguments. That may not be helped by how many "null"s are floating around since they have no type. https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/TempFiles.java:101: class DeleteFileVisitor implements FileVisitor<Path> { private and static https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/TempFiles.java:120: public FileVisitResult postVisitDirectory(Path directory, IOException ioe) ioe may need to be logged.
Sign in to reply to this message.
Forgot to mention that it gets 93% code coverage in NioFileDelegate. https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/NioFileDelegateTest.java (right): https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/NioFileDelegateTest.java:45: public void setUp() throws Exception { On 2014/01/31 20:21:02, ejona wrote: > No need for setUp(). Set these as part of their declaration. I agree the delegate need only be created once. The temp file directory does need to get cleaned up tho. It doesn't need to be created and torn down for each test, so teardown can be run as AfterClass? https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/NioFileDelegateTest.java:183: static class TestNioFileDelegate extends NioFileDelegate { On 2014/01/31 20:21:02, ejona wrote: > This can be private. Done. https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/TempFiles.java (right): https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/TempFiles.java:31: * This functionality was removed from Guava. On 2014/01/31 20:21:02, ejona wrote: > Where in Guava? I can't find anything very similar to this. IoTestCase is the > closest I found, and this seems more "inspired by" Guava than a copy. By > "removed from Guava", you mean that it used to be a class and was removed, or > that you copied it from Guava? Files.deleteRecursively() and Files.deleteDirectoryContents() were deprecated in guava r10 and removed in r11. This code was not taken from the guava code, and Java 7 NIO makes the recursive delete much easier. I will clarify the comment. https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/TempFiles.java:44: TempFiles(String prefix) throws IOException { On 2014/01/31 20:21:02, ejona wrote: > On these package-private classes (or a class with non-public visibility), I find > it generally helpful to use the public/private/package-protected almost as if it > was public. I'm mentioning it now because it has become a recurring theme. > Although it has no impact on true visibility, seeing something as "public" means > that it is part of the API that the class exposes, and the fact that we are > choosing to expose the class is completely orthogonal. Lacking the public > identifier actively makes the code harder to read, because I have to wonder > whether this method is part of the general API. > > There are cases where that sort of thing doesn't apply (generally with inner > classes that interact very closely with the outer class or are structs), but > this class would seem to benefit as it has a very clearly defined API, strong > cohesion, and low coupling. So you are saying make the methods public and keep the class package private? https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/TempFiles.java:54: Path newTempFile() throws IOException { On 2014/01/31 20:21:02, ejona wrote: > Although I don't want to pick on this class too much because it is a pretty > reasonable helper class, the organization of the overloading is problematic for > me. The fact that 'parent' is the "most optional" of the arguments is > unexpected/hard to read given the positioning of the arguments and the > hierarchical nature of parent directory. > > Given the actual usages, it doesn't even seem to be any gain to make tempRoot > the most optional. Based on my count, you have null in just as many method calls > using newTempFile(), newTempFile(String, String), newTempFile(Path, String, > String) as newTempFile(), newTempFile(Path), newTempFile(Path, String, String). > > And newTempDir() is the only variant that is used, so it is not impacted by such > a change. > > It is "fine" how it is now, but honestly my eyes cross each time I am looking at > the arguments. That may not be helped by how many "null"s are floating around > since they have no type. It is obvious that I tend to write helper and mock classes before I actually use them, because I have a design in my head. I knew I was going to use the no-arg methods most of the time, but also knew I needed the prefix/suffix versions for tests for hidden files and content type. I knew I needed the parent versions when adding items to created temp subdirectories. I see your confusion about the position of parent in the params. It just mirrors the position of the args in NIO Files.createTemp*() methods. https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/TempFiles.java:101: class DeleteFileVisitor implements FileVisitor<Path> { On 2014/01/31 20:21:02, ejona wrote: > private and static Done. https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/TempFiles.java:120: public FileVisitResult postVisitDirectory(Path directory, IOException ioe) On 2014/01/31 20:21:02, ejona wrote: > ioe may need to be logged. Done.
Sign in to reply to this message.
Eric's feedback
Sign in to reply to this message.
https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/NioFileDelegateTest.java (right): https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/NioFileDelegateTest.java:45: public void setUp() throws Exception { On 2014/01/31 22:40:01, Brett wrote: > On 2014/01/31 20:21:02, ejona wrote: > > No need for setUp(). Set these as part of their declaration. > > I agree the delegate need only be created once. > The temp file directory does need to get cleaned up tho. > It doesn't need to be created and torn down for each test, > so teardown can be run as AfterClass? In JUnit 4 the test class is instantiated for each test invocation. So @Before is basically the same as doing things in a constructor, except that you can do I/O there and not feel bad. AfterClass doesn't need to come into play. That would be used with static variables and such. https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/TempFiles.java (right): https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/TempFiles.java:44: TempFiles(String prefix) throws IOException { On 2014/01/31 22:40:01, Brett wrote: > So you are saying make the methods public and keep the class package private? Yep. It has no impact to the actual visibility but makes the contents of the class not depend on the class visibility. In a way, it is less coupling. https://codereview.appspot.com/58930043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/TempFiles.java:54: Path newTempFile() throws IOException { On 2014/01/31 22:40:01, Brett wrote: > I see your confusion about the position of parent in the params. > It just mirrors the position of the args in NIO Files.createTemp*() methods. The order of the params makes sense to me, especially with parent first, and it makes sense that it mirrors Files.createTemp(). The problem I have is the overloaded version of newTempFile(String, String), since Path is not the first parameter.
Sign in to reply to this message.
https://codereview.appspot.com/58930043/diff/20001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/TempFiles.java (right): https://codereview.appspot.com/58930043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/TempFiles.java:31: * This replaces the File.deleteRecursively() functionality that was I don't really know what value the talk about Guava provides. It doesn't tell me anything about the history of the code (because it never used deleteRecursively()) and it doesn't tell me anything about the implementation (because it is implemented completely differently).
Sign in to reply to this message.
On 2014/01/31 23:04:07, ejona wrote: > https://codereview.appspot.com/58930043/diff/20001/test/com/google/enterprise... > File test/com/google/enterprise/adaptor/fs/TempFiles.java (right): > > https://codereview.appspot.com/58930043/diff/20001/test/com/google/enterprise... > test/com/google/enterprise/adaptor/fs/TempFiles.java:31: * This replaces the > File.deleteRecursively() functionality that was > I don't really know what value the talk about Guava provides. It doesn't tell me > anything about the history of the code (because it never used > deleteRecursively()) and it doesn't tell me anything about the implementation > (because it is implemented completely differently). Comment removed.
Sign in to reply to this message.
Eric's feedback
Sign in to reply to this message.
https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/NioFileDelegateTest.java (right): https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/NioFileDelegateTest.java:46: temp = new TempFiles("NioFileDelegateTest"); As I said earlier, move to declaration. https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/TempFiles.java (right): https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/TempFiles.java:128: while (retries-- > 0) { Why not a for loop? https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/TempFiles.java:128: while (retries-- > 0) { So you try to delete every folder multiple times, until it throws an exception? You almost certainly are missing a break. Either in the NoSuchFileException, or after the inner try-catch altogether. https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/TempFiles.java:147: log.warning("Interrupted while deleting temp dir: " + directory); Call Thread.currentThread().interrupt(). You need to set the interrupt bit back.
Sign in to reply to this message.
Thank you. https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/TempFiles.java (right): https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/TempFiles.java:32: * Creates and deletes temporary files and directories used by the tests. Explicitly note they are not-mock. https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/TempFiles.java:68: "Attempt to create file outside of temp dir."); indent 1 more
Sign in to reply to this message.
https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/NioFileDelegateTest.java (right): https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/NioFileDelegateTest.java:46: temp = new TempFiles("NioFileDelegateTest"); On 2014/02/04 01:00:38, ejona wrote: > As I said earlier, move to declaration. I really prefer to balance setUp() and tearDown() methods as it makes it clear that work done in setUp() is cleaned up in tearDown(). Your model implies internal knowledge of junit mechanisms. https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/TempFiles.java (right): https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/TempFiles.java:32: * Creates and deletes temporary files and directories used by the tests. On 2014/02/04 02:45:20, pjo wrote: > Explicitly note they are not-mock. Done. https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/TempFiles.java:68: "Attempt to create file outside of temp dir."); On 2014/02/04 02:45:20, pjo wrote: > indent 1 more Done. https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/TempFiles.java:128: while (retries-- > 0) { On 2014/02/04 01:00:38, ejona wrote: > Why not a for loop? Now it can be. The initial construction had scope issues for retries, needed in the exception handler. I redid it as a for loop. https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/TempFiles.java:128: while (retries-- > 0) { On 2014/02/04 01:00:38, ejona wrote: > So you try to delete every folder multiple times, until it throws an exception? > > You almost certainly are missing a break. Either in the NoSuchFileException, or > after the inner try-catch altogether. Excellent catch. This code was annoying. Windows seems to do delete the directory contents asynchronously, yet the check for emptiness of the directory itself seems to be done synchronously. So trying to delete the directory throws a not empty exception while its content(s) finish deleting themselves in the background. In my opinion, unlinking should be done synchronously, and any remaining work could be done asynchronously. Even after adding the retries, one directory would consistently fail to delete. That is when I discovered that Windows won't let you delete a hidden file, so I had to remember to unhide it in a finally clause in the test itself. I can drop the NoSuchFileException handling, as that was a result of the missing break statement. https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/TempFiles.java:147: log.warning("Interrupted while deleting temp dir: " + directory); On 2014/02/04 01:00:38, ejona wrote: > Call Thread.currentThread().interrupt(). You need to set the interrupt bit back. Done.
Sign in to reply to this message.
https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/NioFileDelegateTest.java (right): https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/NioFileDelegateTest.java:46: temp = new TempFiles("NioFileDelegateTest"); On 2014/02/04 19:29:56, Brett wrote: > On 2014/02/04 01:00:38, ejona wrote: > > As I said earlier, move to declaration. > > I really prefer to balance setUp() and tearDown() > methods as it makes it clear that work done in > setUp() is cleaned up in tearDown(). Okay, that's fair. I don't quite see it that way, but that's fine. Either way, you should say so instead of forcing me to comment on it again in order to understand why it wasn't moved. > Your model > implies internal knowledge of junit mechanisms. What are @Before, @After, and @Test to you? The fact that it constructs anew each time was one of the big selling points of JUnit 4. It isn't a hidden secret, but an important feature. @Rules like the thrown.expect() thing actually require that behavior to function; it is a core part of the design and other parts of JUnit4 stop working without it. https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/TempFiles.java (right): https://codereview.appspot.com/58930043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/TempFiles.java:128: while (retries-- > 0) { On 2014/02/04 19:29:56, Brett wrote: > Excellent catch. This code was annoying. > Windows seems to do delete the directory > contents asynchronously, yet the check > for emptiness of the directory itself seems > to be done synchronously. So trying to > delete the directory throws a not empty > exception while its content(s) finish > deleting themselves in the background. > In my opinion, unlinking should be done > synchronously, and any remaining work > could be done asynchronously. > > Even after adding the retries, one directory > would consistently fail to delete. That is > when I discovered that Windows won't let > you delete a hidden file, so I had to remember > to unhide it in a finally clause in the test itself. Have you checked that the retrying is still required? I'm concerned that the issue may be a file needs to be close()d, or some other similar bug. I wouldn't put it past Windows for this to be necessary, but it may be worth the effort to investigate further. I just discovered this class's functionality is available as part of junit: http://junit.org/javadoc/4.8/org/junit/rules/TemporaryFolder.html . Note that newFile() and newFolder() are simply convenience methods. But if the delete retries are required, it probably wouldn't work for us.
Sign in to reply to this message.
Replace TempFiles with JUnit TempFolders Rule
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
|
