|
|
Created:
10 years, 2 months ago by Brett Modified:
10 years, 2 months ago CC:
connector-cr_google.com Visibility:
Public. |
DescriptionThe purpose of these classes is to allow construction of
in-memory file system structure with configuration of the
files and directories to suit the tests that will be performed.
MockFile represents a file or directory. It has many setters
that allow the tests to define the file's configuration.
MockFileDelegate is a FileDelegate implementation over the
in-memory file system consisting of MockFiles.
Patch Set 1 #
Total comments: 16
Patch Set 2 : Redesign MockFile, remove MockDirectoryBuilder #
Total comments: 28
Patch Set 3 : Eric's and PJ's feedback #
MessagesTotal messages: 16
Thank you. A couple of quick comments. The functionality of having in memory files is desireable, but I've some hesitations on the representation, especially when not seeing the tests that use it. https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/MockDirectoryBuilder.java (right): https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockDirectoryBuilder.java:50: // Do not alter file configuration, add it to expected results. Expectation is based on presensce? ... at some implicit point ... in time in some process? but not on say ... contents? It seems to me that expected results are something that belongs in particular test using these Mocks. https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockDirectoryBuilder.java:55: public static final ConfigureFile CONFIGURE_FILE_NONE = new ConfigureFile() { I don't get the NONE and the ALL. https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockFile.java:68: LENGTH, NEW_INPUT_STREAM, NEW_DIRECTORY_STREAM, IS_HIDDEN } Functionality of being able to throw exceptions from inside tests is great. But why couldn't a test that wants to throw an exception do so by overriding the particular method? ... inside some test ... static class MyMockFile extends MockFile { @overridde public InputStream getInputStream() { if (usedAfterDelete) throw IOException("no stream 2 b got"); ... } } That also gives the test the representation where it could do so at the beginning, middle, end or any other part of the particular desired implementation of a method, instead of having throw be at the beginning. https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockFile.java:90: protected MockFile(MockFile parent, String name, boolean isDir) { I would like to see the tests that use this representation.
Sign in to reply to this message.
https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/MockDirectoryBuilder.java (right): https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockDirectoryBuilder.java:50: // Do not alter file configuration, add it to expected results. On 2014/01/07 04:37:26, pjo wrote: > Expectation is based on presensce? ... at > some implicit point ... in time in some process? > but not on say ... contents? > > It seems to me that expected results are something > that belongs in particular test using these Mocks. These classes were taken from the FileSystem Connector with very little modification. I wasn't sure that maintaining the expected results would be nearly as useful in the adaptor as it was in the connector, but I left it in just in case. If it turns out to be less than useful, I will rip it out. https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockDirectoryBuilder.java:55: public static final ConfigureFile CONFIGURE_FILE_NONE = new ConfigureFile() { On 2014/01/07 04:37:26, pjo wrote: > I don't get the NONE and the ALL. Basically ALL of the created nodes end up in the expected set, or NONE of the created nodes end up in the expected set. https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockFile.java:68: LENGTH, NEW_INPUT_STREAM, NEW_DIRECTORY_STREAM, IS_HIDDEN } These classes were written before we started using EZMock in the connector tests, where testing exception handling is much easier. An additional complication in the connector tests is setException took an Exception, not just IOException, so we could also test RepositoryExceptions, RepositoryDocumentExceptions, etc. So the throwing exception logic here is much simplified over the version in the connector tests. Using the subclass model would require providing a factory to MockDirectoryBuilder. Having the configure callback set exceptions to be thrown was as convenient as calling any of the other setters on MockFile. On 2014/01/07 04:37:26, pjo wrote: > Functionality of being able to throw exceptions > from inside tests is great. But why couldn't a > test that wants to throw an exception do so by > overriding the particular method? > > ... inside some test ... > > static class MyMockFile extends MockFile { > @overridde > public InputStream getInputStream() { > if (usedAfterDelete) > throw IOException("no stream 2 b got"); > ... > } > } > > That also gives the test the representation > where it could do so at the beginning, middle, > end or any other part of the particular > desired implementation of a method, instead of > having throw be at the beginning. https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockFile.java:90: protected MockFile(MockFile parent, String name, boolean isDir) { On 2014/01/07 04:37:26, pjo wrote: > I would like to see the tests that use > this representation. This constructor is called by MockDirectoryBuilder as it constructs the in-memory filesystem. See the connector tests (although in those tests this class is called MockReadonlyFile): https://code.google.com/p/google-enterprise-connector-file-system/source/brow... https://code.google.com/p/google-enterprise-connector-file-system/source/brow...
Sign in to reply to this message.
Thank you. https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/MockDirectoryBuilder.java (right): https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockDirectoryBuilder.java:50: // Do not alter file configuration, add it to expected results. On 2014/01/08 00:59:23, Brett wrote: > On 2014/01/07 04:37:26, pjo wrote: > > Expectation is based on presensce? ... at > > some implicit point ... in time in some process? > > but not on say ... contents? > > > > It seems to me that expected results are something > > that belongs in particular test using these Mocks. > > These classes were taken from the FileSystem Connector > with very little modification. I wasn't sure that maintaining > the expected results would be nearly as useful in the > adaptor as it was in the connector, but I left it in just in > case. If it turns out to be less than useful, I will rip it out. > > I'm hesitent to introduce these items provisionally. I would rather introduce this functionality piece meal along with tests that utilize it. https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockFile.java:68: LENGTH, NEW_INPUT_STREAM, NEW_DIRECTORY_STREAM, IS_HIDDEN } On 2014/01/08 00:59:23, Brett wrote: > These classes were written before we started using > EZMock in the connector tests, where testing exception > handling is much easier. An additional complication > in the connector tests is setException took an Exception, > not just IOException, so we could also test RepositoryExceptions, > RepositoryDocumentExceptions, etc. So the throwing > exception logic here is much simplified over the version > in the connector tests. > > Using the subclass model > would require providing a factory to MockDirectoryBuilder. > Having the configure callback set exceptions to be thrown > was as convenient as calling any of the other setters on > MockFile. > > On 2014/01/07 04:37:26, pjo wrote: > > Functionality of being able to throw exceptions > > from inside tests is great. But why couldn't a > > test that wants to throw an exception do so by > > overriding the particular method? > > > > ... inside some test ... > > > > static class MyMockFile extends MockFile { > > @overridde > > public InputStream getInputStream() { > > if (usedAfterDelete) > > throw IOException("no stream 2 b got"); > > ... > > } > > } > > > > That also gives the test the representation > > where it could do so at the beginning, middle, > > end or any other part of the particular > > desired implementation of a method, instead of > > having throw be at the beginning. > I don't understand how EZMock is getting involved. I also don't understand how a factory would be required. The example above could be used in any test method of any test class without EZMock and without a factory. Simultenously any exception could be thrown based on any condition of the method.
Sign in to reply to this message.
https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/MockDirectoryBuilder.java (right): https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockDirectoryBuilder.java:50: // Do not alter file configuration, add it to expected results. On 2014/01/08 01:21:33, pjo wrote: > On 2014/01/08 00:59:23, Brett wrote: > > On 2014/01/07 04:37:26, pjo wrote: > > > Expectation is based on presensce? ... at > > > some implicit point ... in time in some process? > > > but not on say ... contents? > > > > > > It seems to me that expected results are something > > > that belongs in particular test using these Mocks. > > > > These classes were taken from the FileSystem Connector > > with very little modification. I wasn't sure that maintaining > > the expected results would be nearly as useful in the > > adaptor as it was in the connector, but I left it in just in > > case. If it turns out to be less than useful, I will rip it out. > > > > > > I'm hesitent to introduce these items provisionally. > I would rather introduce this functionality piece meal > along with tests that utilize it. It is far easier to rip it out if unused than it is to rip it out now, then put it back later. https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockFile.java:68: LENGTH, NEW_INPUT_STREAM, NEW_DIRECTORY_STREAM, IS_HIDDEN } I was just giving a bit of history of testing practices in the connector. We changed the way we tested exception handling once we started using EZMock. WRT no factory, how does MockDirectoryBuilder create the MockFile subclass objects without one? On 2014/01/08 01:21:33, pjo wrote: > On 2014/01/08 00:59:23, Brett wrote: > > These classes were written before we started using > > EZMock in the connector tests, where testing exception > > handling is much easier. An additional complication > > in the connector tests is setException took an Exception, > > not just IOException, so we could also test RepositoryExceptions, > > RepositoryDocumentExceptions, etc. So the throwing > > exception logic here is much simplified over the version > > in the connector tests. > > > > Using the subclass model > > would require providing a factory to MockDirectoryBuilder. > > Having the configure callback set exceptions to be thrown > > was as convenient as calling any of the other setters on > > MockFile. > > > > On 2014/01/07 04:37:26, pjo wrote: > > > Functionality of being able to throw exceptions > > > from inside tests is great. But why couldn't a > > > test that wants to throw an exception do so by > > > overriding the particular method? > > > > > > ... inside some test ... > > > > > > static class MyMockFile extends MockFile { > > > @overridde > > > public InputStream getInputStream() { > > > if (usedAfterDelete) > > > throw IOException("no stream 2 b got"); > > > ... > > > } > > > } > > > > > > That also gives the test the representation > > > where it could do so at the beginning, middle, > > > end or any other part of the particular > > > desired implementation of a method, instead of > > > having throw be at the beginning. > > > > > I don't understand how EZMock is getting involved. > I also don't understand how a factory would be > required. > The example above could be used in any test method > of any test class without EZMock and without a > factory. > Simultenously any exception could be thrown > based on any condition of the method.
Sign in to reply to this message.
Thank you. I appreciate the back and forth. https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/MockDirectoryBuilder.java (right): https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockDirectoryBuilder.java:50: // Do not alter file configuration, add it to expected results. On 2014/01/08 05:36:26, Brett wrote: > On 2014/01/08 01:21:33, pjo wrote: > > On 2014/01/08 00:59:23, Brett wrote: > > > On 2014/01/07 04:37:26, pjo wrote: > > > > Expectation is based on presensce? ... at > > > > some implicit point ... in time in some process? > > > > but not on say ... contents? > > > > > > > > It seems to me that expected results are something > > > > that belongs in particular test using these Mocks. > > > > > > These classes were taken from the FileSystem Connector > > > with very little modification. I wasn't sure that maintaining > > > the expected results would be nearly as useful in the > > > adaptor as it was in the connector, but I left it in just in > > > case. If it turns out to be less than useful, I will rip it out. > > > > > > > > > > I'm hesitent to introduce these items provisionally. > > I would rather introduce this functionality piece meal > > along with tests that utilize it. > > It is far easier to rip it out if unused than it is to rip it out > now, then put it back later. I don't think that's the primary criteria for our code quality. The difference in work will net us a better representation, be easier to review, leave us with a clearer history, and not set precedent for introducing provisional representations. https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockFile.java:68: LENGTH, NEW_INPUT_STREAM, NEW_DIRECTORY_STREAM, IS_HIDDEN } > WRT no factory, how does MockDirectoryBuilder create > the MockFile subclass objects without one? Here is a sketch of a test using Java more directly. This example has not been compiled, it's just written in here: class SomeTestClass { private static class ThrowsOnThirdReadMF extends MockFile { int nreads = 0; public InputStream getInputStream() { if (++nreads >= 3) throw new IOException(); ... } } ... somewhere in testing code ... MockFile mockDir = new MockFile("parentDir").setChildren(new File[] { new MockFile("log1.log").setFileContents("Log file 1"), new MockFile("log1.log.lck").setFileContents("skipped lock file"), new MockFile("log2.log").setFileContents("Log file 2"), new MockFile("subdir").setChildren(new File[] { new MockFile("nested.log").setFileContents("This file skipped."), new ThrowsOnThirdReadMF("throwy").setFileContents("throwy-file") })}); } It seems to me we may be able to drop from this mock representaiton the code which manages expecation of existance and the code which triggeres exceptions and instead use the representation of the Java language ontop of shorter mock representation while gaining in expressivness.
Sign in to reply to this message.
https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockFile.java:68: LENGTH, NEW_INPUT_STREAM, NEW_DIRECTORY_STREAM, IS_HIDDEN } On 2014/01/08 19:42:40, pjo wrote: > MockFile mockDir = new MockFile("parentDir").setChildren(new File[] { > new MockFile("log1.log").setFileContents("Log file 1"), > new MockFile("log1.log.lck").setFileContents("skipped lock file"), > new MockFile("log2.log").setFileContents("Log file 2"), > new MockFile("subdir").setChildren(new File[] { > new MockFile("nested.log").setFileContents("This file skipped."), > new ThrowsOnThirdReadMF("throwy").setFileContents("throwy-file") > })}); > } I am also a fan of more of this style of definition. There are some simple things that could improve it a little more (like having setChildren() File... so you don't need the new File[] stuff and maybe even combining the setting of the contents/defining children into the constructor), but this style seems great. I don't foresee me being happy with anything that resembles this Where exception throwing in the mock. I'm not saying I couldn't be, but that I can't think of such a design. I would love some things to be public, because it is hard to tell with all the package-private what methods are for general consumption. Only have features that you have tests for. If you will have it "real soon", that can be okay, given that that "real soon" is another CL also out for review. https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/MockFileDelegate.java (right): https://codereview.appspot.com/47730043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockFileDelegate.java:112: // TODO: Is this a per share thing or a per file thing? Per link thing. You pass it a link under a namespace and it tells you the real share.
Sign in to reply to this message.
I think this CL should either drastically transform into something that has tests and a small representation, or could be abandoned for another CL that will have tests and a small representation. Thank you, PJ - technology's compounding interest - On Fri, Jan 10, 2014 at 6:02 PM, <ejona@google.com> wrote: > > https://codereview.appspot.com/47730043/diff/1/test/com/ > google/enterprise/adaptor/fs/MockFile.java > File test/com/google/enterprise/adaptor/fs/MockFile.java (right): > > https://codereview.appspot.com/47730043/diff/1/test/com/ > google/enterprise/adaptor/fs/MockFile.java#newcode68 > test/com/google/enterprise/adaptor/fs/MockFile.java:68: LENGTH, > NEW_INPUT_STREAM, NEW_DIRECTORY_STREAM, IS_HIDDEN } > On 2014/01/08 19:42:40, pjo wrote: > >> MockFile mockDir = new MockFile("parentDir").setChildren(new >> > File[] { > >> new MockFile("log1.log").setFileContents("Log file 1"), >> new MockFile("log1.log.lck").setFileContents("skipped lock >> > file"), > >> new MockFile("log2.log").setFileContents("Log file 2"), >> new MockFile("subdir").setChildren(new File[] { >> new MockFile("nested.log").setFileContents("This file >> > skipped."), > >> new >> > ThrowsOnThirdReadMF("throwy").setFileContents("throwy-file") > >> })}); >> } >> > > I am also a fan of more of this style of definition. There are some > simple things that could improve it a little more (like having > setChildren() File... so you don't need the new File[] stuff and maybe > even combining the setting of the contents/defining children into the > constructor), but this style seems great. > > I don't foresee me being happy with anything that resembles this Where > exception throwing in the mock. I'm not saying I couldn't be, but that I > can't think of such a design. > > I would love some things to be public, because it is hard to tell with > all the package-private what methods are for general consumption. > > Only have features that you have tests for. If you will have it "real > soon", that can be okay, given that that "real soon" is another CL also > out for review. > > https://codereview.appspot.com/47730043/diff/1/test/com/ > google/enterprise/adaptor/fs/MockFileDelegate.java > File test/com/google/enterprise/adaptor/fs/MockFileDelegate.java > (right): > > https://codereview.appspot.com/47730043/diff/1/test/com/ > google/enterprise/adaptor/fs/MockFileDelegate.java#newcode112 > test/com/google/enterprise/adaptor/fs/MockFileDelegate.java:112: // > TODO: Is this a per share thing or a per file thing? > Per link thing. You pass it a link under a namespace and it tells you > the real share. > > https://codereview.appspot.com/47730043/ >
Sign in to reply to this message.
Redesign MockFile, remove MockDirectoryBuilder
Sign in to reply to this message.
Thank you. This review has a couple of specific notes about specific points in the code. A higher level point: I think where we should head to now is a CL that contains a couple of concrete tests and a little of this CL. Echoing Eric's point from review for issue 46780045, create a vertical slice. This CL continues to be a horitzontal slice and I'm not comfortable accepting an API that isn't tied to use. I can imagine a sequence of test CLs, like say: #1 Brings up adaptor, destroys it #2 Bad config #3 Simple lister calls (requires some mocks) #4 Simple getDocContent calls (requires some mocks) #5 Handling errors from getDocContents #6 Preserves access time (requires richer mocks) That list is of the top of my head. Maybe it's not the best order, maybe ACLs should be somewhere there. The point I'm trying to make tho, is that having vertical slices that come along with gradually increasing mock representation will help me review this code and give me many chances to critique and suggest adjustments. Otherwise I'd have to imagine where we are headed and that's a little too hard. Please take this code and test code you've been creating and present it in a vertical slice ordering. Thank you https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:72: this.name = name; check not null? https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:86: directoryContents = new ArrayList<MockFile>(); Seems like this method is setChildren, instead of addChildren https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:120: } OK, so delete unliks from parent, but lots of other methods may do unexpected things like exists being true and isRegularFile being true. https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:154: String getParent() { Why doesn't getParent return MockFile? https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:191: this.creationTime= creationTime; s/=/ =/ https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockFileDelegate.java (right): https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFileDelegate.java:36: this.root = root; check not null https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFileDelegate.java:131: // TODO (bmj): implementation throw unsupported operation exception for now? https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFileDelegate.java:136: // TODO (bmj): implementation throw unsupported operation exception for now?
Sign in to reply to this message.
MockFile seems unwieldy; I wouldn't trust many tests that use it, because I can't convince myself it would be configured correctly/the mock is relatively bug free. I'd have to spend more time on it to figure out how to best organize it, but I think putting checks on the gets (like I do in MockFile of the Adaptor) may be appropriate. That would let you define more defaults at construction and have sets be more simplistic. For example, the entire contentType and setFileContents() with isDirectory is a bit maddening to follow/verify. It isn't obvious what state things will be and what is allowed. For instance, setFileContents().addChildren() will work while addChildren().setFileContents() won't, but I think the first case has isRegularFile and isDirectory both become invalid, but there are checks that prevent issues with listing children and getting file contents from being incorrect. https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:64: private String fileContents; This probably should be binary. https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:73: FileTime now = FileTime.fromMillis(System.currentTimeMillis()); I'm not exited about 'now'. That seems to be precisely unhelpful. A hard-coded value would at least not be bothersome. https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:86: directoryContents = new ArrayList<MockFile>(); On 2014/01/16 20:02:56, pjo wrote: > Seems like this method is setChildren, instead of addChildren No, it's addChildren because of the directoryContents == null condition. https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:114: if (parent != null) { Invert condition for readability. if (parent == null) { return true; } Iterator... https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:191: this.creationTime= creationTime; Space before = https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:246: return (shareAclView == null) ? fullAccessAclView : shareAclView; Hmm... Most of the time this is just wrong to be called. If it is null, it should probably throw an exception. https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockFileDelegate.java (right): https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFileDelegate.java:115: return root.getShareAclView(); The passed doc has to be the share (similar for below). https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFileDelegate.java:131: // TODO (bmj): implementation On 2014/01/16 20:02:56, pjo wrote: > throw unsupported operation exception for now? Probably not because we want things to start.
Sign in to reply to this message.
Eric's and PJ's feedback
Sign in to reply to this message.
Thank you. Can you reply to the request to advance to a CL that has a small number of tests and the supporting subset of this representation, please. - technology's compounding interest - On Fri, Jan 17, 2014 at 7:11 PM, <Brett.Michael.Johnson@gmail.com> wrote: > Eric's and PJ's feedback > > http://codereview.appspot.com/47730043/ >
Sign in to reply to this message.
https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:64: private String fileContents; On 2014/01/17 00:18:21, ejona wrote: > This probably should be binary. Done. https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:72: this.name = name; On 2014/01/16 20:02:56, pjo wrote: > check not null? Done. https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:73: FileTime now = FileTime.fromMillis(System.currentTimeMillis()); On 2014/01/17 00:18:21, ejona wrote: > I'm not exited about 'now'. That seems to be precisely unhelpful. A hard-coded > value would at least not be bothersome. Done. https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:86: directoryContents = new ArrayList<MockFile>(); Precisely. Testing the monitor requires adding and deleting files after initial construction of the filesystem. On 2014/01/17 00:18:21, ejona wrote: > On 2014/01/16 20:02:56, pjo wrote: > > Seems like this method is setChildren, instead of addChildren > > No, it's addChildren because of the directoryContents == null condition. https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:120: } On 2014/01/16 20:02:56, pjo wrote: > OK, so delete unliks from parent, > but lots of other methods may do unexpected > things like exists being true and isRegularFile > being true. Fixed. https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:154: String getParent() { On 2014/01/16 20:02:56, pjo wrote: > Why doesn't getParent return MockFile? It was modeled on java.io.File.getParent(), but there is no reason it shouldn't return a MockFile here. I am not frequently using it in the tests yet. https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:191: this.creationTime= creationTime; On 2014/01/17 00:18:21, ejona wrote: > Space before = Done. https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:246: return (shareAclView == null) ? fullAccessAclView : shareAclView; On 2014/01/17 00:18:21, ejona wrote: > Hmm... Most of the time this is just wrong to be called. If it is null, it > should probably throw an exception. Actually, this is for convenience. This, and the other logic wrt null acls, allows me to construct a directory tree with full access acls without thinking about it in the test. This means I only have to set an acl if I intend to test specific acl logic. https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockFileDelegate.java (right): https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFileDelegate.java:36: this.root = root; On 2014/01/16 20:02:56, pjo wrote: > check not null Done. And getFile() and getPath() below. https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFileDelegate.java:115: return root.getShareAclView(); On 2014/01/17 00:18:21, ejona wrote: > The passed doc has to be the share (similar for below). FsAdaptor calls this on the root. For testing, it really only makes sense to call setShareAclView on the root.
Sign in to reply to this message.
The changes to delete() just further strengthen my concern that MockFile seems unwieldy. And tests. We keep wanting to see the tests. https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:86: directoryContents = new ArrayList<MockFile>(); On 2014/01/20 19:50:59, Brett wrote: > Precisely. Testing the monitor requires adding and deleting files after initial > construction of the filesystem. Nope. Testing the monitor does not require any changes to this structure from what I can tell. The monitor is almost completely separate (minus creating DocIds). You just have to send events and make sure you get feeds pushed as you expect. The retriever never enters into it. FS Adaptor has same basic architecture of SP Adaptor and SP Adaptor uses immutable data structures for every test, including the modification detection tests. https://codereview.appspot.com/47730043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:246: return (shareAclView == null) ? fullAccessAclView : shareAclView; On 2014/01/20 19:50:59, Brett wrote: > On 2014/01/17 00:18:21, ejona wrote: > > Hmm... Most of the time this is just wrong to be called. If it is null, it > > should probably throw an exception. > > Actually, this is for convenience. This, and the other logic wrt null acls, > allows me to construct a directory tree with full access acls without thinking > about it in the test. This means I only have to set an acl if I intend to test > specific acl logic. Which means you don't notice if it is called at completely wrong times.
Sign in to reply to this message.
Closed as these changes have been merged into https://codereview.appspot.com/58560043/
Sign in to reply to this message.
|