|
|
DescriptionThis adds unit tests for the FsAdaptor class, including a bunch
of supporting test classes. The changes include:
FsAdaptorTest: 40+ Unit tests for FsAdaptor class
FsAdaptor: Added a test constructor that takes a FileDelegate arg, made some
fields, methods, and inner classes VisibleForTesting
HtmlResponseWriter: Fixed array bounds exception for unix paths
MockFile: A lightweight, in-memory file system
MockFileTest: Sanity checks for MockFile
MockFileDelgate: A FileDelegate backed by MockFiles
MockFileDelegateTest: Sanity checks for MockFileDelegate
MockAdaptorContext: A AdaptorContext wrapping AccumlatingDocIdPusher
MockRequest: A trivial Request implementation
MockResponse: A Response that supports only the features used by the adaptor
AccumulatingDocIdPusher: Imported from SP adaptor and heavily modified
UnsupportedDocIdPusher: Parent class of AccumulatingDocId pusher
MockDocIdCodec: Imported from adaptor framework tests
These unit tests provide 91% code coverage for the FsAdaptor class.
Patch Set 1 #
Total comments: 37
Patch Set 2 : Eric's and PJ's feedback #
Total comments: 18
Patch Set 3 : Eric's and PJ's feedback #
Total comments: 28
Patch Set 4 : Various Feedback #
Total comments: 14
Patch Set 5 : Fix HtmlResponseWriter.computeLabel splitter. Rename FsAdaptor.getPathName() to getFolderName() and… #
Total comments: 5
Patch Set 6 : Eric's feedback #
Total comments: 4
MessagesTotal messages: 43
Nowhere near reviewed, but sending what I have. I expect I will have some input on how this can be split into several CLs. https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:137: /** ThHe FileSystem change Monitor. */ The H and capital F and S confuse me. And capital M. None seem to be class names and I can't figure out why they are capitalized. https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... File src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java (right): https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:220: label = (parts.length > 0) ? parts[parts.length - 1] : doc.getUniqueId(); So this is actually a bug in that we are using the one-argument version of split(). I consider any use of the one-argument split() a bug because it is rarely what you want. If it is what you want, then use the two-argument version explicitly with 0: .split("/", 0) I would normally change the split to: split("/", -1). The parts.length > 0 thing is just detecting all-slash paths? In normal usage, that would mean just root "/" would be impacted. Is that correct? It is hard to tell based on "Fixed array bounds exception for unix paths". https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/AccumulatingDocIdPusher.java (right): https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/AccumulatingDocIdPusher.java:89: return namedResouces; Since it is modified already, might as well pass this through unmodifiableList(). It's not a big deal though. https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:908: // It seems like a long wait, but the minimum is 1s, and my Using an incrementalPollPeriodSecs of 0 should work without issue. https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockFile.java:45: static final FileTime defaultFileTime = FileTime.fromMillis(10000); DEFAULT_FILE_TIME. It is immutable. I believe ditto below. https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/MockFileDelegate.java (right): https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockFileDelegate.java:55: //DEBUGGING throw new FileNotFoundException("not found: " + doc.toString()); Clean up.
Sign in to reply to this message.
Thank you https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:123: Set<String> supportedWindowsAccounts; make visible for testing accessor https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:131: String builtinPrefix; make visible for testing accessor https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:135: String namespace; make visible for testing accesor https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:923: } It's great that all these tests are ready. Can we help reviewers by splitting this ~3K lines CL into smaller pieces, maybe by introducing these tests a handful at a time?
Sign in to reply to this message.
I'm now questioning if this CL needs to be split up (leaning toward no). I skimmed the various tests and they seem reasonably cohesive. More review to come. https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:60: (System.getProperty("os.name").startsWith("Windows")) I don't think this should be dependent on the OS you are running. You may choose to run different tests, but I don't like that you are changing the test you are running. The OS is supposed to be mocked out in these tests, so I also don't see it as necessary. Is it to make Paths.get() happy? If that is the only thing that is using the OS, then it seems we should have an UnsupportedPath that throws UnsupportedOperationException for every method call and use that instead. We don't even need it to know its name — we could just use a different instance for each file and use Object.equals() to differentiate them. For debugging a test failure we make give them more info, but the point is that they don't need much implementation at all. https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:74: @SuppressWarnings("unchecked") I don't see any unchecked cast. What is it? Could you instead place the annotation on a declaration so it only applies to one line? https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:76: context = new MockAdaptorContext(); Do the initialization during declaration? It seems just adaptor.initConfig and overrideKey is necessary here. https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockFile.java:214: boolean isHidden() throws IOException { As discussed previously, this class doesn't do enough checking for validity conditions. For example, here we can isHidden() be true when exists() could return false. Now, there does exist delete() that updates some of these bits, but I really would like to see that each method checks its assumptions, because otherwise I'm not trusting of the class.
Sign in to reply to this message.
https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockFile.java:131: void delete() { Unused?
Sign in to reply to this message.
https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:123: Set<String> supportedWindowsAccounts; On 2014/01/30 21:36:02, pjo wrote: > make visible for testing accessor Done. https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:131: String builtinPrefix; On 2014/01/30 21:36:02, pjo wrote: > make visible for testing accessor Done. https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:135: String namespace; On 2014/01/30 21:36:02, pjo wrote: > make visible for testing accesor Done. https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:137: /** ThHe FileSystem change Monitor. */ On 2014/01/30 20:18:26, ejona wrote: > The H and capital F and S confuse me. And capital M. None seem to be class names > and I can't figure out why they are capitalized. Done. https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... File src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java (right): https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:220: label = (parts.length > 0) ? parts[parts.length - 1] : doc.getUniqueId(); On 2014/01/30 20:18:26, ejona wrote: > So this is actually a bug in that we are using the one-argument version of > split(). I consider any use of the one-argument split() a bug because it is > rarely what you want. If it is what you want, then use the two-argument version > explicitly with 0: .split("/", 0) > > I would normally change the split to: split("/", -1). The parts.length > 0 thing > is just detecting all-slash paths? In normal usage, that would mean just root > "/" would be impacted. Is that correct? It is hard to tell based on "Fixed array > bounds exception for unix paths". You are correct that the bug fixes the array bounds exception when given the unix root as a path. If I switch to split("/", -1), I'm still going to get a zero length array back, right? Or will I get a single empty string? https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/AccumulatingDocIdPusher.java (right): https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/AccumulatingDocIdPusher.java:89: return namedResouces; On 2014/01/30 20:18:26, ejona wrote: > Since it is modified already, might as well pass this through > unmodifiableList(). It's not a big deal though. Done. https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:60: (System.getProperty("os.name").startsWith("Windows")) On 2014/01/30 22:16:19, ejona wrote: > I don't think this should be dependent on the OS you are running. > > You may choose to run different tests, but I don't like that you are changing > the test you are running. The OS is supposed to be mocked out in these tests, so > I also don't see it as necessary. > > Is it to make Paths.get() happy? If that is the only thing that is using the OS, > then it seems we should have an UnsupportedPath that throws > UnsupportedOperationException for every method call and use that instead. We > don't even need it to know its name — we could just use a different instance for > each file and use Object.equals() to differentiate them. For debugging a test > failure we make give them more info, but the point is that they don't need much > implementation at all. This is to make the FsAdaptor.init() use of Path.getRoot() happy. I now have a sneaking suspicion that I can use "/" (or "\\") on Windows and getRoot will succeed. I will try it. Edit: I tried "/" as root on windows and it worked wrt Paths.getRoot(). However, it has issues in newDocId() and getFile(). I will fix those up. So I eliminated the OS dependency here. https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:74: @SuppressWarnings("unchecked") On 2014/01/30 22:16:19, ejona wrote: > I don't see any unchecked cast. What is it? Could you instead place the > annotation on a declaration so it only applies to one line? It is the cast of DocIdPusher to AccumulatingDocIdPusher, and, no Java won't let me apply it to that line because the declaration is above. I will add a local variable, do the cast assignment (with suppress warnings) to that, then assign that to the field. https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:908: // It seems like a long wait, but the minimum is 1s, and my On 2014/01/30 20:18:26, ejona wrote: > Using an incrementalPollPeriodSecs of 0 should work without issue. FsMonitor doesn't allow you to specify 0 for the latency. https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockFile.java:45: static final FileTime defaultFileTime = FileTime.fromMillis(10000); On 2014/01/30 20:18:26, ejona wrote: > DEFAULT_FILE_TIME. It is immutable. I believe ditto below. Done. https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockFile.java:214: boolean isHidden() throws IOException { On 2014/01/30 22:16:19, ejona wrote: > As discussed previously, this class doesn't do enough checking for validity > conditions. For example, here we can isHidden() be true when exists() could > return false. Now, there does exist delete() that updates some of these bits, > but I really would like to see that each method checks its assumptions, because > otherwise I'm not trusting of the class. Done. https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/MockFileDelegate.java (right): https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/MockFileDelegate.java:55: //DEBUGGING throw new FileNotFoundException("not found: " + doc.toString()); On 2014/01/30 20:18:26, ejona wrote: > Clean up. Done.
Sign in to reply to this message.
https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... File src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java (right): https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:220: label = (parts.length > 0) ? parts[parts.length - 1] : doc.getUniqueId(); On 2014/02/01 01:24:34, Brett wrote: > You are correct that the bug fixes the array bounds exception > when given the unix root as a path. If I switch to split("/", -1), > I'm still going to get a zero length array back, right? > Or will I get a single empty string? It will be guaranteed to have at least one element. But if you split "/" on "/", then you will get ["", ""]. That isn't great, but we also wouldn't ever expect to pass "/" to this method. https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:74: @SuppressWarnings("unchecked") On 2014/02/01 01:24:34, Brett wrote: > On 2014/01/30 22:16:19, ejona wrote: > > I don't see any unchecked cast. What is it? Could you instead place the > > annotation on a declaration so it only applies to one line? > > It is the cast of DocIdPusher to AccumulatingDocIdPusher, > and, no Java won't let me apply it to that line because the > declaration is above. I will add a local variable, do the > cast assignment (with suppress warnings) to that, then > assign that to the field. Uhh, really? That doesn't sound right. I thought unchecked casts were only for generics and I don't see any involved there. Something doesn't add up... Using the local variable sounds great. As bothersome as it is, that is the common pattern you have to do for @SuppressWarnings. https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:908: // It seems like a long wait, but the minimum is 1s, and my On 2014/02/01 01:24:34, Brett wrote: > On 2014/01/30 20:18:26, ejona wrote: > > Using an incrementalPollPeriodSecs of 0 should work without issue. > > FsMonitor doesn't allow you to specify 0 for the latency. I think it would be fair for you to change it. Zero disables the batching so it is actually useful, as is evident here.
Sign in to reply to this message.
https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... File src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java (right): https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:220: label = (parts.length > 0) ? parts[parts.length - 1] : doc.getUniqueId(); On 2014/02/01 01:34:47, ejona wrote: > On 2014/02/01 01:24:34, Brett wrote: > > You are correct that the bug fixes the array bounds exception > > when given the unix root as a path. If I switch to split("/", -1), > > I'm still going to get a zero length array back, right? > > Or will I get a single empty string? > > It will be guaranteed to have at least one element. > > But if you split "/" on "/", then you will get ["", ""]. That isn't great, but > we also wouldn't ever expect to pass "/" to this method. I not only expect to pass "/" to this method, I *do* pass "/" to this method in the tests. This is how I found the problem. https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:60: (System.getProperty("os.name").startsWith("Windows")) On 2014/02/01 01:24:34, Brett wrote: > On 2014/01/30 22:16:19, ejona wrote: > > I don't think this should be dependent on the OS you are running. > > > > You may choose to run different tests, but I don't like that you are changing > > the test you are running. The OS is supposed to be mocked out in these tests, > so > > I also don't see it as necessary. > > > > Is it to make Paths.get() happy? If that is the only thing that is using the > OS, > > then it seems we should have an UnsupportedPath that throws > > UnsupportedOperationException for every method call and use that instead. We > > don't even need it to know its name — we could just use a different instance > for > > each file and use Object.equals() to differentiate them. For debugging a test > > failure we make give them more info, but the point is that they don't need > much > > implementation at all. > > This is to make the FsAdaptor.init() use of Path.getRoot() happy. > I now have a sneaking suspicion that I can use "/" (or "\\") on > Windows and getRoot will succeed. I will try it. > > Edit: I tried "/" as root on windows and it worked wrt Paths.getRoot(). > However, it has issues in newDocId() and getFile(). I will fix those up. > So I eliminated the OS dependency here. > It was just FsAdaptorTest.getPath() that was failing, and I fixed that up. Now "/" as the rootPath works for both unix and windows. https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:74: @SuppressWarnings("unchecked") On 2014/02/01 01:34:47, ejona wrote: > On 2014/02/01 01:24:34, Brett wrote: > > On 2014/01/30 22:16:19, ejona wrote: > > > I don't see any unchecked cast. What is it? Could you instead place the > > > annotation on a declaration so it only applies to one line? > > > > It is the cast of DocIdPusher to AccumulatingDocIdPusher, > > and, no Java won't let me apply it to that line because the > > declaration is above. I will add a local variable, do the > > cast assignment (with suppress warnings) to that, then > > assign that to the field. > > Uhh, really? That doesn't sound right. I thought unchecked casts were only for > generics and I don't see any involved there. Something doesn't add up... > > Using the local variable sounds great. As bothersome as it is, that is the > common pattern you have to do for @SuppressWarnings. I did the local variable workaround and it worked just fine. https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:908: // It seems like a long wait, but the minimum is 1s, and my On 2014/02/01 01:34:47, ejona wrote: > On 2014/02/01 01:24:34, Brett wrote: > > On 2014/01/30 20:18:26, ejona wrote: > > > Using an incrementalPollPeriodSecs of 0 should work without issue. > > > > FsMonitor doesn't allow you to specify 0 for the latency. > > I think it would be fair for you to change it. Zero disables the batching so it > is actually useful, as is evident here. Done. I changed the checkArgument to (maxLatencyMillis >= 0).
Sign in to reply to this message.
Eric's and PJ's feedback
Sign in to reply to this message.
Please address or respond to comments in full. MockFile still has the structural concerns I have commented on multiple times. https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... File src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java (right): https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:220: label = (parts.length > 0) ? parts[parts.length - 1] : doc.getUniqueId(); On 2014/02/03 23:08:23, Brett wrote: > On 2014/02/01 01:34:47, ejona wrote: > > But if you split "/" on "/", then you will get ["", ""]. That isn't great, but > > we also wouldn't ever expect to pass "/" to this method. > > I not only expect to pass "/" to this method, I *do* pass "/" to this method > in the tests. This is how I found the problem. Fix the split call. As far as I am concerned, an empty label is fully good and great for /. Just because you do it in a test, doesn't mean it is normal or good. We shouldn't throw an exception, but it isn't really wrong if we have a label for / being "", since it will never likely be seen by a user and both "/" and "" are poor labels. This code only runs on Windows at the moment. On Windows "/" means the root of the current drive, which for us is almost guaranteed to be C:\ which would be very unlikely to be an SMB mount that we should be indexing and we don't even support mapped drives at this point. Even on Linux it would be extremely strange to be '/', because that would mean the root is SMB, which I have never experienced and think it would take a lot of effort to work. '/' is a very poor test string, considering that we require something of the form //share/. I would greatly prefer just fixing the split call to split("/", -1) and then reverting the parts.length > 0 check. https://codereview.appspot.com/58560043/diff/20001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/58560043/diff/20001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:515: "the maxLatencyMillis must be greater than zero"); or equal to https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:75: (AccumulatingDocIdPusher) context.getDocIdPusher(); I just tried this myself and I don't get any unchecked warning. I double checked that -Xlint:unchecked was specified. In every way I know it seems this shouldn't be necessary. I guess I'll take a look after you commit it. https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:907: adaptor.init(context); This is going to be called multiple times during testMonitorMultipleBatches(), which is a bad idea. https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:923: assertEquals(expected, actual); Compare the Records, not the DocIds. https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:131: void delete() { This is still unused.
Sign in to reply to this message.
Thank you. https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:912: Thread.sleep(100L); annotate the need for this sleep https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:915: Set<DocId> expected = Sets.newHashSet(); use treeset https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:917: expected.add(getDocId(name)); can the set be made by passing names into ctor, instead of doing the gradual accumulation? https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:131: void delete() { On 2014/02/04 01:37:11, ejona wrote: > This is still unused. In general, the ignore-some-comments-until they're raised 3-5 times is getting old.
Sign in to reply to this message.
https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:131: void delete() { On 2014/02/04 01:37:11, ejona wrote: > This is still unused. Note that it is important that this is unused, because setExists is also unused, so exists could go away, and you wouldn't have had to put in all the exists checking. This is the sort of thing that you should change after you realize your expectations and your actual needs are different. This is also precisely the reason we are against checking in yet-to-be-used code.
Sign in to reply to this message.
https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... File src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java (right): https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:220: label = (parts.length > 0) ? parts[parts.length - 1] : doc.getUniqueId(); On 2014/02/04 01:37:11, ejona wrote: > On 2014/02/03 23:08:23, Brett wrote: > > On 2014/02/01 01:34:47, ejona wrote: > > > But if you split "/" on "/", then you will get ["", ""]. That isn't great, > but > > > we also wouldn't ever expect to pass "/" to this method. > > > > I not only expect to pass "/" to this method, I *do* pass "/" to this method > > in the tests. This is how I found the problem. > > Fix the split call. > > As far as I am concerned, an empty label is fully good and great for /. Just > because you do it in a test, doesn't mean it is normal or good. We shouldn't > throw an exception, but it isn't really wrong if we have a label for / being "", > since it will never likely be seen by a user and both "/" and "" are poor > labels. > > This code only runs on Windows at the moment. On Windows "/" means the root of > the current drive, which for us is almost guaranteed to be C:\ which would be > very unlikely to be an SMB mount that we should be indexing and we don't even > support mapped drives at this point. Even on Linux it would be extremely strange > to be '/', because that would mean the root is SMB, which I have never > experienced and think it would take a lot of effort to work. '/' is a very poor > test string, considering that we require something of the form //share/. > > I would greatly prefer just fixing the split call to split("/", -1) and then > reverting the parts.length > 0 check. Done. https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:76: context = new MockAdaptorContext(); On 2014/01/30 22:16:19, ejona wrote: > Do the initialization during declaration? It seems just adaptor.initConfig and > overrideKey is necessary here. Done. https://codereview.appspot.com/58560043/diff/20001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/58560043/diff/20001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:515: "the maxLatencyMillis must be greater than zero"); On 2014/02/04 01:37:11, ejona wrote: > or equal to Done. https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:75: (AccumulatingDocIdPusher) context.getDocIdPusher(); On 2014/02/04 01:37:11, ejona wrote: > I just tried this myself and I don't get any unchecked warning. I double checked > that -Xlint:unchecked was specified. In every way I know it seems this shouldn't > be necessary. > > I guess I'll take a look after you commit it. Done. https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:907: adaptor.init(context); On 2014/02/04 01:37:11, ejona wrote: > This is going to be called multiple times during testMonitorMultipleBatches(), > which is a bad idea. Done. https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:912: Thread.sleep(100L); On 2014/02/04 03:02:20, pjo wrote: > annotate the need for this sleep Done. https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:915: Set<DocId> expected = Sets.newHashSet(); On 2014/02/04 03:02:20, pjo wrote: > use treeset I changed the code to compare two sets of Record per your comment below. Cannot use TreeSet, because Record is not Comparable. https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:917: expected.add(getDocId(name)); On 2014/02/04 03:02:20, pjo wrote: > can the set be made by passing names into ctor, instead of doing the gradual > accumulation? I was creating a set of DocIds from an array of string names. How would that work? https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:923: assertEquals(expected, actual); On 2014/02/04 01:37:11, ejona wrote: > Compare the Records, not the DocIds. Done. https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:131: void delete() { On 2014/02/04 18:04:25, ejona wrote: > On 2014/02/04 01:37:11, ejona wrote: > > This is still unused. > > Note that it is important that this is unused, because setExists is also unused, > so exists could go away, and you wouldn't have had to put in all the exists > checking. > > This is the sort of thing that you should change after you realize your > expectations and your actual needs are different. This is also precisely the > reason we are against checking in yet-to-be-used code. I have removed delete() and exists() and all the supporting infrastructure.
Sign in to reply to this message.
Eric's and PJ's feedback
Sign in to reply to this message.
What is left is PJ and me to look through MockFile again to see if we want tweaks and for reviewers to go through the tests. I have skimmed the tests and things looked reasonable, but it wasn't a full review. I feel like this CL is nearing its endgame and just requires reviewer effort of actually reading it. https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:65: (AccumulatingDocIdPusher) context.getDocIdPusher(); This is sort of an odd indentation. No need to change, but wanted to point it out. https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:859: Thread.sleep(100L); // Allow FsMonitor to drain the queue. It looks like a tab made it in here. https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:908: Thread.sleep(100L); // Allow FsMonitor to drain the queue. Another tab. https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:287: return (isRegularFile) ? contentType : null; Remove extraneous parens?
Sign in to reply to this message.
Some comments added. I have a hard time following what we're trying to test. I see lot of mocks and verification but I could not follow how/what adaptor specific code is being tested. https://codereview.appspot.com/58560043/diff/40001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/58560043/diff/40001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:174: FsMonitor getFsMonitor() { Do we really need to expose the FsMonitor? The FsMonitor manages the starting and destroying of the thread that pushes resources to the GSA. It does not seem that the FsMonitor is worthy of mocking. We try to add test for destroying and starting the monitor but that would make the test Windows speicific since the test would have to confirm that a specific thread ID has been terminated. It seems that verifying that the correct items are on the queue would be a good test but then have to add a hook to send the collection of resources some where instead of trying to push them to the GSA. https://codereview.appspot.com/58560043/diff/40001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:262: if (monitor != null) { It should never be the case that monitor is null. If it happens to be that is an error condition.
Sign in to reply to this message.
Let's split the work of reviewing this CL across more people: Miguel, Are the right workflows being tested? M test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java M src/com/google/enterprise/adaptor/fs/FsAdaptor.java Tanmay, please carry fixes to non-FS versions of these files: M src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java A test/com/google/enterprise/adaptor/fs/AccumulatingDocIdPusher.java A test/com/google/enterprise/adaptor/fs/UnsupportedDocIdPusher.java Marc, please try to focus on the minimalness and sensibiulity of these APIs: A test/com/google/enterprise/adaptor/fs/MockAdaptorContext.java A test/com/google/enterprise/adaptor/fs/MockDocIdCodec.java A test/com/google/enterprise/adaptor/fs/MockFile.java A test/com/google/enterprise/adaptor/fs/MockFileDelegate.java A test/com/google/enterprise/adaptor/fs/MockFileDelegateTest.java A test/com/google/enterprise/adaptor/fs/MockFileTest.java A test/com/google/enterprise/adaptor/fs/MockRequest.java A test/com/google/enterprise/adaptor/fs/MockResponse.java Thank you - technology's compounding interest - On Wed, Feb 5, 2014 at 3:15 PM, <mifern@google.com> wrote: > Some comments added. > > I have a hard time following what we're trying to test. I see lot of > mocks and verification but I could not follow how/what adaptor specific > code is being tested. > > > https://codereview.appspot.com/58560043/diff/40001/src/ > com/google/enterprise/adaptor/fs/FsAdaptor.java > File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): > > https://codereview.appspot.com/58560043/diff/40001/src/ > com/google/enterprise/adaptor/fs/FsAdaptor.java#newcode174 > src/com/google/enterprise/adaptor/fs/FsAdaptor.java:174: FsMonitor > getFsMonitor() { > Do we really need to expose the FsMonitor? The FsMonitor manages the > starting and destroying of the thread that pushes resources to the GSA. > It does not seem that the FsMonitor is worthy of mocking. We try to add > test for destroying and starting the monitor but that would make the > test Windows speicific since the test would have to confirm that a > specific thread ID has been terminated. It seems that verifying that the > correct items are on the queue would be a good test but then have to add > a hook to send the collection of resources some where instead of trying > to push them to the GSA. > > https://codereview.appspot.com/58560043/diff/40001/src/ > com/google/enterprise/adaptor/fs/FsAdaptor.java#newcode262 > src/com/google/enterprise/adaptor/fs/FsAdaptor.java:262: if (monitor != > null) { > It should never be the case that monitor is null. If it happens to be > that is an error condition. > > https://codereview.appspot.com/58560043/ >
Sign in to reply to this message.
The files I was asked to look at (Mock*.java) are mostly great as they are. I think 3 of them could be moved into the base library test framework; one other one just needs a single Javadoc change. https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockAdaptorContext.java (right): https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockAdaptorContext.java:15: package com.google.enterprise.adaptor.fs; This line is quite literally the only reference to the file system in the entire class. I would suggest moving this to com.google.enterprise.adaptor (the base library's test directory). https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockDocIdCodec.java (right): https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockDocIdCodec.java:24: * Mock of {@link DocIdDecoder}. Mock of DocIdCodec (instead of DocIdDecoder), no? https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockRequest.java (right): https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockRequest.java:15: package com.google.enterprise.adaptor.fs; Like MockAdaptorContext.java, I think this class should go into the base library test area. https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockResponse.java (right): https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockResponse.java:15: package com.google.enterprise.adaptor.fs; This class as well should work (as is) in the base library test directory.
Sign in to reply to this message.
https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockAdaptorContext.java (right): https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockAdaptorContext.java:15: package com.google.enterprise.adaptor.fs; On 2014/02/07 20:17:37, myk wrote: > This line is quite literally the only reference to the file system in the entire > class. This is actually very common for Mocks and utilities in general. > I would suggest moving this to com.google.enterprise.adaptor (the base library's > test directory). When we do that we have to have stable test class APIs since those classes must be public to be used here. This class's API is not appropriate to be public at the moment. We do need to do this almost now, but it is also easier said than done. I don't think it is appropriate to block this commit on it. https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockRequest.java (right): https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockRequest.java:15: package com.google.enterprise.adaptor.fs; On 2014/02/07 20:17:37, myk wrote: > Like MockAdaptorContext.java, I think this class should go into the base library > test area. This one is actually sorta already there. Unfortunately, it isn't as simple as just moving it, because then it must be public and have API stability. https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockResponse.java (right): https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockResponse.java:15: package com.google.enterprise.adaptor.fs; On 2014/02/07 20:17:37, myk wrote: > This class as well should work (as is) in the base library test directory. Again, it takes design. It seems that people would be happy if this happened soon. It is appropriate to start making it more of a priority.
Sign in to reply to this message.
https://codereview.appspot.com/58560043/diff/40001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/58560043/diff/40001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:174: FsMonitor getFsMonitor() { On 2014/02/05 23:15:40, mifern wrote: > Do we really need to expose the FsMonitor? The FsMonitor manages the starting > and destroying of the thread that pushes resources to the GSA. It does not seem > that the FsMonitor is worthy of mocking. We try to add test for destroying and > starting the monitor but that would make the test Windows speicific since the > test would have to confirm that a specific thread ID has been terminated. It > seems that verifying that the correct items are on the queue would be a good > test but then have to add a hook to send the collection of resources some where > instead of trying to push them to the GSA. FsMonitor is only visible to gain access to its queue. I will comment it as such. https://codereview.appspot.com/58560043/diff/40001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:262: if (monitor != null) { On 2014/02/05 23:15:40, mifern wrote: > It should never be the case that monitor is null. If it happens to be that is an > error condition. The monitor is null if initConfig was never called, and if destroy has already been called. This is mostly to allow convenient used of destroy() in tearDown(). https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:65: (AccumulatingDocIdPusher) context.getDocIdPusher(); On 2014/02/05 22:37:16, ejona wrote: > This is sort of an odd indentation. No need to change, but wanted to point it > out. Done. https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:859: Thread.sleep(100L); // Allow FsMonitor to drain the queue. On 2014/02/05 22:37:16, ejona wrote: > It looks like a tab made it in here. I will re-enable untabify. https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:908: Thread.sleep(100L); // Allow FsMonitor to drain the queue. On 2014/02/05 22:37:16, ejona wrote: > Another tab. Done. https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockDocIdCodec.java (right): https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockDocIdCodec.java:24: * Mock of {@link DocIdDecoder}. On 2014/02/07 20:17:37, myk wrote: > Mock of DocIdCodec (instead of DocIdDecoder), no? Done. https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/MockFile.java:287: return (isRegularFile) ? contentType : null; On 2014/02/05 22:37:16, ejona wrote: > Remove extraneous parens? Done.
Sign in to reply to this message.
https://codereview.appspot.com/58560043/diff/40001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/58560043/diff/40001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:174: FsMonitor getFsMonitor() { On 2014/02/08 03:23:32, Brett wrote: > FsMonitor is only visible to gain access to its queue. I will comment it as > such. You may consider returning the queue itself. That seems even better.
Sign in to reply to this message.
https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:136: ImmutableSet.of("Everyone", "BUILTIN\\Users", "NT AUTH\\New Users"); Can we use ImmutableSet.of(Splitter.on(',').trimResults().split(accounts))? https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:164: adaptor.getPathName(Paths.get("/folder1/folder2/"))); What does this test? "/folder1/folder2/" is not a valid path format that we would expect or support in the adaptor. https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:470: private void testNoPreserveFileLastAccessTime(MockFile file) throws Exception { 80 chars.
Sign in to reply to this message.
https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:136: ImmutableSet.of("Everyone", "BUILTIN\\Users", "NT AUTH\\New Users"); On 2014/02/11 02:30:31, mifern wrote: > Can we use ImmutableSet.of(Splitter.on(',').trimResults().split(accounts))? I sorta liked it hard-coded. It is obvious what the output is, with minimal chance of getting it wrong. Given that the FsAdaptor does "Splitter.on(',').trimResults().split(accountsStr)", I think it is very wise to keep it hard-coded.
Sign in to reply to this message.
Various Feedback
Sign in to reply to this message.
https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:136: ImmutableSet.of("Everyone", "BUILTIN\\Users", "NT AUTH\\New Users"); On 2014/02/11 02:30:31, mifern wrote: > Can we use ImmutableSet.of(Splitter.on(',').trimResults().split(accounts))? That actually duplicates the logic I am testing. This is a functional test that verifies the logic I am testing returns the expected results. https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:164: adaptor.getPathName(Paths.get("/folder1/folder2/"))); On 2014/02/11 02:30:31, mifern wrote: > What does this test? "/folder1/folder2/" is not a valid path format that we > would expect or support in the adaptor. But it is a pathname that the MockFileDelegate will return. Since the logic for getPathName is in FsAdaptor and not delegated, I want to verify it works with mock pathnames, since it is using with nio Path, nio Paths, and java.io.File. https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:470: private void testNoPreserveFileLastAccessTime(MockFile file) throws Exception { On 2014/02/11 02:30:31, mifern wrote: > 80 chars. Done.
Sign in to reply to this message.
There has been no review feedback on this CL for a week. This is blocking https://codereview.appspot.com/64060043/, as it needs the test infrastructure of FsAdaptorTest for its own tests.
Sign in to reply to this message.
Miguel, please give another round of feedback for FsAdaptorTest Tanmay please take a look at the three duplicate classes, keeping a careful eye for differences between SP versions and FS versions and potentially carrying changes back to SP. Thanks Mark for finishing the review of the mocks. - technology's compounding interest - On Wed, Feb 5, 2014 at 4:40 PM, PJ <pjo@google.com> wrote: > > Let's split the work of reviewing this CL across more people: > > Miguel, Are the right workflows being tested? > M test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java > M src/com/google/enterprise/adaptor/fs/FsAdaptor.java > > Tanmay, please carry fixes to non-FS versions of these files: > M src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java > A test/com/google/enterprise/adaptor/fs/AccumulatingDocIdPusher.java > A test/com/google/enterprise/adaptor/fs/UnsupportedDocIdPusher.java > > Marc, please try to focus on the minimalness and sensibiulity of these > APIs: > > A test/com/google/enterprise/adaptor/fs/MockAdaptorContext.java > A test/com/google/enterprise/adaptor/fs/MockDocIdCodec.java > A test/com/google/enterprise/adaptor/fs/MockFile.java > A test/com/google/enterprise/adaptor/fs/MockFileDelegate.java > A test/com/google/enterprise/adaptor/fs/MockFileDelegateTest.java > A test/com/google/enterprise/adaptor/fs/MockFileTest.java > A test/com/google/enterprise/adaptor/fs/MockRequest.java > A test/com/google/enterprise/adaptor/fs/MockResponse.java > > Thank you > > > > - technology's compounding interest > - > > > On Wed, Feb 5, 2014 at 3:15 PM, <mifern@google.com> wrote: > >> Some comments added. >> >> I have a hard time following what we're trying to test. I see lot of >> mocks and verification but I could not follow how/what adaptor specific >> code is being tested. >> >> >> https://codereview.appspot.com/58560043/diff/40001/src/ >> com/google/enterprise/adaptor/fs/FsAdaptor.java >> File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): >> >> https://codereview.appspot.com/58560043/diff/40001/src/ >> com/google/enterprise/adaptor/fs/FsAdaptor.java#newcode174 >> src/com/google/enterprise/adaptor/fs/FsAdaptor.java:174: FsMonitor >> getFsMonitor() { >> Do we really need to expose the FsMonitor? The FsMonitor manages the >> starting and destroying of the thread that pushes resources to the GSA. >> It does not seem that the FsMonitor is worthy of mocking. We try to add >> test for destroying and starting the monitor but that would make the >> test Windows speicific since the test would have to confirm that a >> specific thread ID has been terminated. It seems that verifying that the >> correct items are on the queue would be a good test but then have to add >> a hook to send the collection of resources some where instead of trying >> to push them to the GSA. >> >> https://codereview.appspot.com/58560043/diff/40001/src/ >> com/google/enterprise/adaptor/fs/FsAdaptor.java#newcode262 >> src/com/google/enterprise/adaptor/fs/FsAdaptor.java:262: if (monitor != >> null) { >> It should never be the case that monitor is null. If it happens to be >> that is an error condition. >> >> https://codereview.appspot.com/58560043/ >> > >
Sign in to reply to this message.
On 2014/02/18 21:53:53, pjo wrote: > Miguel, please give another round of feedback for FsAdaptorTest > > Tanmay please take a look at the three duplicate classes, keeping > a careful eye for differences between SP versions and FS versions > and potentially carrying changes back to SP. > > Thanks Mark for finishing the review of the mocks. > > > > > - technology's compounding interest > - > > > On Wed, Feb 5, 2014 at 4:40 PM, PJ <mailto:pjo@google.com> wrote: > > > > > Let's split the work of reviewing this CL across more people: > > > > Miguel, Are the right workflows being tested? > > M test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java > > M src/com/google/enterprise/adaptor/fs/FsAdaptor.java > > > > Tanmay, please carry fixes to non-FS versions of these files: > > M src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java > > A test/com/google/enterprise/adaptor/fs/AccumulatingDocIdPusher.java > > A test/com/google/enterprise/adaptor/fs/UnsupportedDocIdPusher.java > > > > Marc, please try to focus on the minimalness and sensibiulity of these > > APIs: > > > > A test/com/google/enterprise/adaptor/fs/MockAdaptorContext.java > > A test/com/google/enterprise/adaptor/fs/MockDocIdCodec.java > > A test/com/google/enterprise/adaptor/fs/MockFile.java > > A test/com/google/enterprise/adaptor/fs/MockFileDelegate.java > > A test/com/google/enterprise/adaptor/fs/MockFileDelegateTest.java > > A test/com/google/enterprise/adaptor/fs/MockFileTest.java > > A test/com/google/enterprise/adaptor/fs/MockRequest.java > > A test/com/google/enterprise/adaptor/fs/MockResponse.java > > > > Thank you > > > > > > > > - technology's compounding interest > > - > > > > > > On Wed, Feb 5, 2014 at 3:15 PM, <mailto:mifern@google.com> wrote: > > > >> Some comments added. > >> > >> I have a hard time following what we're trying to test. I see lot of > >> mocks and verification but I could not follow how/what adaptor specific > >> code is being tested. > >> > >> > >> https://codereview.appspot.com/58560043/diff/40001/src/ > >> com/google/enterprise/adaptor/fs/FsAdaptor.java > >> File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): > >> > >> https://codereview.appspot.com/58560043/diff/40001/src/ > >> com/google/enterprise/adaptor/fs/FsAdaptor.java#newcode174 > >> src/com/google/enterprise/adaptor/fs/FsAdaptor.java:174: FsMonitor > >> getFsMonitor() { > >> Do we really need to expose the FsMonitor? The FsMonitor manages the > >> starting and destroying of the thread that pushes resources to the GSA. > >> It does not seem that the FsMonitor is worthy of mocking. We try to add > >> test for destroying and starting the monitor but that would make the > >> test Windows speicific since the test would have to confirm that a > >> specific thread ID has been terminated. It seems that verifying that the > >> correct items are on the queue would be a good test but then have to add > >> a hook to send the collection of resources some where instead of trying > >> to push them to the GSA. > >> > >> https://codereview.appspot.com/58560043/diff/40001/src/ > >> com/google/enterprise/adaptor/fs/FsAdaptor.java#newcode262 > >> src/com/google/enterprise/adaptor/fs/FsAdaptor.java:262: if (monitor != > >> null) { > >> It should never be the case that monitor is null. If it happens to be > >> that is an error condition. > >> > >> https://codereview.appspot.com/58560043/ > >> > > > > AccumulatingDocIdPusher.java and UnsupportedDocIdPusher.java looks fine to me. I have a question about "HtmlResponseWriter.java". Is it possible for File System Adaptor to generate DocIds with trailing slashes other than root? For SharePoint we have only root with DocId as "/" and we can make similar change in SharePoint Adaptor
Sign in to reply to this message.
I think: # SP AccumulatingDocIdPusher should reset groups # SP versionshould return immutable named resources Please take a 2nd and closer look at those files and see if there are other improvements/differences. Thank you PJ - technology's compounding interest - On Tue, Feb 18, 2014 at 2:19 PM, <tvartak@google.com> wrote: > On 2014/02/18 21:53:53, pjo wrote: > >> Miguel, please give another round of feedback for FsAdaptorTest >> > > Tanmay please take a look at the three duplicate classes, keeping >> a careful eye for differences between SP versions and FS versions >> and potentially carrying changes back to SP. >> > > Thanks Mark for finishing the review of the mocks. >> > > > > > - technology's compounding interest >> - >> > > > On Wed, Feb 5, 2014 at 4:40 PM, PJ <mailto:pjo@google.com> wrote: >> > > > >> > Let's split the work of reviewing this CL across more people: >> > >> > Miguel, Are the right workflows being tested? >> > M test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java >> > M src/com/google/enterprise/adaptor/fs/FsAdaptor.java >> > >> > Tanmay, please carry fixes to non-FS versions of these files: >> > M src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java >> > A >> > test/com/google/enterprise/adaptor/fs/AccumulatingDocIdPusher.java > >> > A >> > test/com/google/enterprise/adaptor/fs/UnsupportedDocIdPusher.java > >> > >> > Marc, please try to focus on the minimalness and sensibiulity of >> > these > >> > APIs: >> > >> > A test/com/google/enterprise/adaptor/fs/MockAdaptorContext.java >> > A test/com/google/enterprise/adaptor/fs/MockDocIdCodec.java >> > A test/com/google/enterprise/adaptor/fs/MockFile.java >> > A test/com/google/enterprise/adaptor/fs/MockFileDelegate.java >> > A test/com/google/enterprise/adaptor/fs/MockFileDelegateTest.java >> > A test/com/google/enterprise/adaptor/fs/MockFileTest.java >> > A test/com/google/enterprise/adaptor/fs/MockRequest.java >> > A test/com/google/enterprise/adaptor/fs/MockResponse.java >> > >> > Thank you >> > >> > >> > >> > - technology's compounding interest >> > - >> > >> > >> > On Wed, Feb 5, 2014 at 3:15 PM, <mailto:mifern@google.com> wrote: >> > >> >> Some comments added. >> >> >> >> I have a hard time following what we're trying to test. I see lot >> > of > >> >> mocks and verification but I could not follow how/what adaptor >> > specific > >> >> code is being tested. >> >> >> >> >> >> https://codereview.appspot.com/58560043/diff/40001/src/ >> >> com/google/enterprise/adaptor/fs/FsAdaptor.java >> >> File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): >> >> >> >> https://codereview.appspot.com/58560043/diff/40001/src/ >> >> com/google/enterprise/adaptor/fs/FsAdaptor.java#newcode174 >> >> src/com/google/enterprise/adaptor/fs/FsAdaptor.java:174: FsMonitor >> >> getFsMonitor() { >> >> Do we really need to expose the FsMonitor? The FsMonitor manages >> > the > >> >> starting and destroying of the thread that pushes resources to the >> > GSA. > >> >> It does not seem that the FsMonitor is worthy of mocking. We try to >> > add > >> >> test for destroying and starting the monitor but that would make >> > the > >> >> test Windows speicific since the test would have to confirm that a >> >> specific thread ID has been terminated. It seems that verifying >> > that the > >> >> correct items are on the queue would be a good test but then have >> > to add > >> >> a hook to send the collection of resources some where instead of >> > trying > >> >> to push them to the GSA. >> >> >> >> https://codereview.appspot.com/58560043/diff/40001/src/ >> >> com/google/enterprise/adaptor/fs/FsAdaptor.java#newcode262 >> >> src/com/google/enterprise/adaptor/fs/FsAdaptor.java:262: if >> > (monitor != > >> >> null) { >> >> It should never be the case that monitor is null. If it happens to >> > be > >> >> that is an error condition. >> >> >> >> https://codereview.appspot.com/58560043/ >> >> >> > >> > >> > AccumulatingDocIdPusher.java and UnsupportedDocIdPusher.java looks fine > to me. > > I have a question about "HtmlResponseWriter.java". Is it possible for > File System Adaptor to generate DocIds with trailing slashes other than > root? For SharePoint we have only root with DocId as "/" and we can make > similar change in SharePoint Adaptor > > > https://codereview.appspot.com/58560043/ >
Sign in to reply to this message.
> AccumulatingDocIdPusher.java and UnsupportedDocIdPusher.java looks fine > to me. > > I have a question about "HtmlResponseWriter.java". Is it possible for > File System Adaptor to generate DocIds with trailing slashes other than > root? For SharePoint we have only root with DocId as "/" and we can make > similar change in SharePoint Adaptor > The File System Adaptor docIds for directories have trailing slashes, whereas the docIds for regular files do not. > https://codereview.appspot.com/58560043/
Sign in to reply to this message.
https://codereview.appspot.com/58560043/diff/60001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java (right): https://codereview.appspot.com/58560043/diff/60001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:219: String[] parts = doc.getUniqueId().split("/", -1); Tanmay made a really good point, that any folder would get a poor name here if we use -1 (and not 0). So the two of us looked at it together, and this code isn't necessary, since label will always (except for root '/') be populated with the proper name. This code was needed for SharePoint, but it can be ripped out for FileSystem. I don't know what that means for this review though.
Sign in to reply to this message.
When I look at FsAdaptor, it always passes in the pathname as the label to the HtmlResponseWriter methods. Since the offending call to split only happens if that label is null or empty, I should revert my change. Then see how I managed to get to the code that called split on “/", resulting in the array index exception. BTW, in the SP version, it should probably call guava’s Strings.isNullOrEmpty() for that test in computeLabel(). -- Brett M. Johnson On Feb 18, 2014, at 2:35 PM, ejona@google.com wrote: > > https://codereview.appspot.com/58560043/diff/60001/src/com/google/enterprise/... > File src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java > (right): > > https://codereview.appspot.com/58560043/diff/60001/src/com/google/enterprise/... > src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:219: > String[] parts = doc.getUniqueId().split("/", -1); > Tanmay made a really good point, that any folder would get a poor name > here if we use -1 (and not 0). > > So the two of us looked at it together, and this code isn't necessary, > since label will always (except for root '/') be populated with the > proper name. This code was needed for SharePoint, but it can be ripped > out for FileSystem. > > I don't know what that means for this review though. > > https://codereview.appspot.com/58560043/
Sign in to reply to this message.
Once again, I was fooled by the FsAdaptor.getPathName() method that should really be called getFileName(). Given the root (“/“), getPathName() returns the empty string. -- Brett M. Johnson On Feb 18, 2014, at 2:52 PM, Brett Johnson <brett.michael.johnson@gmail.com> wrote: > When I look at FsAdaptor, > it always passes in the pathname > as the label to the HtmlResponseWriter > methods. Since the offending call to > split only happens if that label is null > or empty, I should revert my change. > Then see how I managed to get to the > code that called split on “/", resulting > in the array index exception. > > BTW, in the SP version, it should > probably call guava’s Strings.isNullOrEmpty() > for that test in computeLabel(). > > -- > Brett M. Johnson > > > > > On Feb 18, 2014, at 2:35 PM, ejona@google.com wrote: > >> >> https://codereview.appspot.com/58560043/diff/60001/src/com/google/enterprise/... >> File src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java >> (right): >> >> https://codereview.appspot.com/58560043/diff/60001/src/com/google/enterprise/... >> src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:219: >> String[] parts = doc.getUniqueId().split("/", -1); >> Tanmay made a really good point, that any folder would get a poor name >> here if we use -1 (and not 0). >> >> So the two of us looked at it together, and this code isn't necessary, >> since label will always (except for root '/') be populated with the >> proper name. This code was needed for SharePoint, but it can be ripped >> out for FileSystem. >> >> I don't know what that means for this review though. >> >> https://codereview.appspot.com/58560043/ >
Sign in to reply to this message.
Fix HtmlResponseWriter.computeLabel splitter. Rename FsAdaptor.getPathName() to getFolderName() and make it work with root paths.
Sign in to reply to this message.
Needs more work. https://codereview.appspot.com/58560043/diff/60001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/58560043/diff/60001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:262: if (monitor != null) { I understand this is not a really big issue but this is not the right action for production code. "monitor" should never be null, if it is there is something wrong and we should be throwing an exception instead of ignoring it. It would seem preferable to update the test to initialize monitor correctly instead of changing production code for a test. https://codereview.appspot.com/58560043/diff/60001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/60001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:121: root.setDfsUncActiveStorageUnc(Paths.get("\\\\dfshost\\share")); This should also check that the adaptor has recognized the path as a DFS and UNC path. Or at least a TODO comment. https://codereview.appspot.com/58560043/diff/60001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:194: } Also for completeness, add "assertFalse(adaptor.isVisibleDescendantOfRoot(null));" https://codereview.appspot.com/58560043/diff/60001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:221: adaptor.init(context); Confirm that the adaptor actually thinks the path is a DFS path. https://codereview.appspot.com/58560043/diff/60001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:236: .setInheritFrom(dfsShareAcl).build(); There are no used in the Acl? There should be at least one. https://codereview.appspot.com/58560043/diff/60001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:249: adaptor.init(context); Confirm that adaptor thinks it's a DFS path so that we actually use the DFS active storage.
Sign in to reply to this message.
https://codereview.appspot.com/58560043/diff/60001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/58560043/diff/60001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:262: if (monitor != null) { On 2014/02/19 01:49:59, mifern wrote: > I understand this is not a really big issue but this is not the right action for > production code. "monitor" should never be null, if it is there is something > wrong and we should be throwing an exception instead of ignoring it. It would > seem preferable to update the test to initialize monitor correctly instead of > changing production code for a test. Frankly, I disagree. The tests cannot initialize the monitor in many cases because they don't start it up. Consider those that test only some of the utility methods, or those that test exceptions in initializing the adaptor. The alternative is to move destroying the adaptor out of tearDown() and into a finally clause of every test that might succeed in initializing the adaptor. https://codereview.appspot.com/58560043/diff/60001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java (right): https://codereview.appspot.com/58560043/diff/60001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:219: String[] parts = doc.getUniqueId().split("/", -1); On 2014/02/18 22:35:09, ejona wrote: > Tanmay made a really good point, that any folder would get a poor name here if > we use -1 (and not 0). > > So the two of us looked at it together, and this code isn't necessary, since > label will always (except for root '/') be populated with the proper name. This > code was needed for SharePoint, but it can be ripped out for FileSystem. > > I don't know what that means for this review though. Yes, using -1 does break docIds that end in '/', so that was a poor choice. I changed it to 0, and moved the special case for handling root out of this class. https://codereview.appspot.com/58560043/diff/60001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/60001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:121: root.setDfsUncActiveStorageUnc(Paths.get("\\\\dfshost\\share")); On 2014/02/19 01:49:59, mifern wrote: > This should also check that the adaptor has recognized the path as a DFS and UNC > path. Or at least a TODO comment. Since FsAdaptor considers this to be DFS or UNC if this path is non-null (it does no other validation), any test would be marginal in effectiveness. The adaptor really delegates validation to the FileDelegate, https://codereview.appspot.com/58560043/diff/60001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:194: } On 2014/02/19 01:49:59, mifern wrote: > Also for completeness, add > "assertFalse(adaptor.isVisibleDescendantOfRoot(null));" Done. https://codereview.appspot.com/58560043/diff/60001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:221: adaptor.init(context); On 2014/02/19 01:49:59, mifern wrote: > Confirm that the adaptor actually thinks the path is a DFS path. If it didn't, the named resources checked below would be missing the dfsShareAcl. https://codereview.appspot.com/58560043/diff/60001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:236: .setInheritFrom(dfsShareAcl).build(); On 2014/02/19 01:49:59, mifern wrote: > There are no used in the Acl? There should be at least one. If "used" was supposed to be "users", then more rigorous ACL tests wrt users, groups, and inheritance are performed below. These tests focus on all the other items in the response. https://codereview.appspot.com/58560043/diff/60001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:249: adaptor.init(context); On 2014/02/19 01:49:59, mifern wrote: > Confirm that adaptor thinks it's a DFS path so that we actually use the DFS > active storage. See above.
Sign in to reply to this message.
https://codereview.appspot.com/58560043/diff/80001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/58560043/diff/80001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:464: String getFolderName(Path file) { This new name is not appropriate, because it is (most) commonly used to provide the name for files, not just folders. https://codereview.appspot.com/58560043/diff/80001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:467: return Strings.isNullOrEmpty(name) ? file.getRoot().toString() : name; IsNullOrEmpty is inappropriate here given getName() will only return one, but which is it, null or empty? The docs say it will be empty for /, so just check that one case. If there is some reason to be concerned about null, then we would likely need a comment because I can't tell right now when it would ever be null. https://codereview.appspot.com/58560043/diff/80001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java (right): https://codereview.appspot.com/58560043/diff/80001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:217: if (Strings.isNullOrEmpty(label)) { Swapping to isNullOrEmpty() is sorta annoying, since it is a style decision and the previous style was not wrong. There are times this could be more appropriate, but here it is just annoying given the difficulty we are already having reviewing this CL.
Sign in to reply to this message.
Eric's feedback
Sign in to reply to this message.
Miguel, please take another look also. Thank you, PJ - technology's compounding interest - On Wed, Mar 5, 2014 at 6:20 PM, <Brett.Michael.Johnson@gmail.com> wrote: > Eric's feedback > > http://codereview.appspot.com/58560043/ >
Sign in to reply to this message.
Changing the code to allow monitor to be null seems like the wrong approach to me. It's an invalid state for monitor to be null. It should never happen that monitor is null, but in the off chance that it does happen, we should throw an exception instead of just ignore the error. Maybe PJ can weigh in what he thinks of this. This is what's been holding the CL from getting approved. https://codereview.appspot.com/58560043/diff/100001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/58560043/diff/100001/src/com/google/enterprise... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:263: if (monitor != null) { This is the same point that we disagree on a null for monitor is an invalid state. We should probably be throwing an IllegalStateException when monitor is null.
Sign in to reply to this message.
https://codereview.appspot.com/58560043/diff/100001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/58560043/diff/100001/src/com/google/enterprise... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:20: import com.google.common.base.Strings; Unused.
Sign in to reply to this message.
Brett, please insert a TODO to consider alternatives to checking for null monitor. I think we have LGTM. Thank you all very much - technology's compounding interest - On Thu, Mar 6, 2014 at 9:30 AM, <ejona@google.com> wrote: > > https://codereview.appspot.com/58560043/diff/100001/src/ > com/google/enterprise/adaptor/fs/FsAdaptor.java > File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): > > https://codereview.appspot.com/58560043/diff/100001/src/ > com/google/enterprise/adaptor/fs/FsAdaptor.java#newcode20 > src/com/google/enterprise/adaptor/fs/FsAdaptor.java:20: import > com.google.common.base.Strings; > Unused. > > https://codereview.appspot.com/58560043/ >
Sign in to reply to this message.
Committed 07 March 2014 to File System Adaptor https://code.google.com/p/plexi.fs/ 40c0a57..d73320b master -> master https://codereview.appspot.com/58560043/diff/80001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/58560043/diff/80001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:464: String getFolderName(Path file) { On 2014/02/19 17:41:28, ejona wrote: > This new name is not appropriate, because it is (most) commonly used to provide > the name for files, not just folders. Done. https://codereview.appspot.com/58560043/diff/80001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:467: return Strings.isNullOrEmpty(name) ? file.getRoot().toString() : name; On 2014/02/19 17:41:28, ejona wrote: > IsNullOrEmpty is inappropriate here given getName() will only return one, but > which is it, null or empty? > > The docs say it will be empty for /, so just check that one case. If there is > some reason to be concerned about null, then we would likely need a comment > because I can't tell right now when it would ever be null. Done. https://codereview.appspot.com/58560043/diff/100001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/58560043/diff/100001/src/com/google/enterprise... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:20: import com.google.common.base.Strings; On 2014/03/06 17:30:27, ejona wrote: > Unused. Done. https://codereview.appspot.com/58560043/diff/100001/src/com/google/enterprise... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:263: if (monitor != null) { On 2014/03/06 06:35:52, mifern wrote: > This is the same point that we disagree on a null for monitor is an invalid > state. We should probably be throwing an IllegalStateException when monitor is > null. PJ asked that I add a TODO to consider other options in the future, but leave it as-is for now.
Sign in to reply to this message.
|