Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1713)

Issue 58560043: FSA Add FsAdaptorTest and supporting test infrastructure (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 2 months ago by Brett
Modified:
10 years ago
Reviewers:
mifern, pjo, myk, ejona, Tanmay Vartak
Visibility:
Public.

Description

This 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+2554 lines, -20 lines) Patch
M src/com/google/enterprise/adaptor/fs/FsAdaptor.java View 1 2 3 4 5 8 chunks +44 lines, -11 lines 4 comments Download
M src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java View 1 2 3 4 2 chunks +3 lines, -2 lines 0 comments Download
A test/com/google/enterprise/adaptor/fs/AccumulatingDocIdPusher.java View 1 2 3 4 1 chunk +96 lines, -0 lines 0 comments Download
M test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java View 1 2 3 4 5 2 chunks +898 lines, -7 lines 0 comments Download
A test/com/google/enterprise/adaptor/fs/MockAdaptorContext.java View 1 chunk +111 lines, -0 lines 0 comments Download
A test/com/google/enterprise/adaptor/fs/MockDocIdCodec.java View 1 2 3 1 chunk +37 lines, -0 lines 0 comments Download
A test/com/google/enterprise/adaptor/fs/MockFile.java View 1 2 3 4 1 chunk +399 lines, -0 lines 0 comments Download
A test/com/google/enterprise/adaptor/fs/MockFileDelegate.java View 1 2 3 4 1 chunk +178 lines, -0 lines 0 comments Download
A test/com/google/enterprise/adaptor/fs/MockFileDelegateTest.java View 1 2 3 4 1 chunk +222 lines, -0 lines 0 comments Download
A test/com/google/enterprise/adaptor/fs/MockFileTest.java View 1 2 3 4 1 chunk +309 lines, -0 lines 0 comments Download
A test/com/google/enterprise/adaptor/fs/MockRequest.java View 1 chunk +50 lines, -0 lines 0 comments Download
A test/com/google/enterprise/adaptor/fs/MockResponse.java View 1 2 3 4 1 chunk +124 lines, -0 lines 0 comments Download
A test/com/google/enterprise/adaptor/fs/UnsupportedDocIdPusher.java View 1 chunk +83 lines, -0 lines 0 comments Download

Messages

Total messages: 43
Brett
10 years, 2 months ago (2014-01-30 07:02:21 UTC) #1
ejona
Nowhere near reviewed, but sending what I have. I expect I will have some input ...
10 years, 2 months ago (2014-01-30 20:18:25 UTC) #2
pjo
Thank you https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adaptor/fs/FsAdaptor.java File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adaptor/fs/FsAdaptor.java#newcode123 src/com/google/enterprise/adaptor/fs/FsAdaptor.java:123: Set<String> supportedWindowsAccounts; make visible for testing accessor ...
10 years, 2 months ago (2014-01-30 21:36:02 UTC) #3
ejona
I'm now questioning if this CL needs to be split up (leaning toward no). I ...
10 years, 2 months ago (2014-01-30 22:16:19 UTC) #4
ejona
https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/adaptor/fs/MockFile.java File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/58560043/diff/1/test/com/google/enterprise/adaptor/fs/MockFile.java#newcode131 test/com/google/enterprise/adaptor/fs/MockFile.java:131: void delete() { Unused?
10 years, 1 month ago (2014-01-31 23:40:11 UTC) #5
Brett
https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adaptor/fs/FsAdaptor.java File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adaptor/fs/FsAdaptor.java#newcode123 src/com/google/enterprise/adaptor/fs/FsAdaptor.java:123: Set<String> supportedWindowsAccounts; On 2014/01/30 21:36:02, pjo wrote: > make ...
10 years, 1 month ago (2014-02-01 01:24:34 UTC) #6
ejona
https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java File src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java (right): https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java#newcode220 src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:220: label = (parts.length > 0) ? parts[parts.length - 1] ...
10 years, 1 month ago (2014-02-01 01:34:47 UTC) #7
Brett
https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java File src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java (right): https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java#newcode220 src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:220: label = (parts.length > 0) ? parts[parts.length - 1] ...
10 years, 1 month ago (2014-02-03 23:08:22 UTC) #8
Brett
Eric's and PJ's feedback
10 years, 1 month ago (2014-02-03 23:10:33 UTC) #9
ejona
Please address or respond to comments in full. MockFile still has the structural concerns I ...
10 years, 1 month ago (2014-02-04 01:37:10 UTC) #10
pjo
Thank you. https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java#newcode912 test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:912: Thread.sleep(100L); annotate the need for this sleep ...
10 years, 1 month ago (2014-02-04 03:02:20 UTC) #11
ejona
https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise/adaptor/fs/MockFile.java File test/com/google/enterprise/adaptor/fs/MockFile.java (right): https://codereview.appspot.com/58560043/diff/20001/test/com/google/enterprise/adaptor/fs/MockFile.java#newcode131 test/com/google/enterprise/adaptor/fs/MockFile.java:131: void delete() { On 2014/02/04 01:37:11, ejona wrote: > ...
10 years, 1 month ago (2014-02-04 18:04:25 UTC) #12
Brett
https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java File src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java (right): https://codereview.appspot.com/58560043/diff/1/src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java#newcode220 src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:220: label = (parts.length > 0) ? parts[parts.length - 1] ...
10 years, 1 month ago (2014-02-05 04:28:31 UTC) #13
Brett
Eric's and PJ's feedback
10 years, 1 month ago (2014-02-05 04:44:54 UTC) #14
ejona
What is left is PJ and me to look through MockFile again to see if ...
10 years, 1 month ago (2014-02-05 22:37:15 UTC) #15
mifern
Some comments added. I have a hard time following what we're trying to test. I ...
10 years, 1 month ago (2014-02-05 23:15:39 UTC) #16
pjo
Let's split the work of reviewing this CL across more people: Miguel, Are the right ...
10 years, 1 month ago (2014-02-06 00:40:01 UTC) #17
myk
The files I was asked to look at (Mock*.java) are mostly great as they are. ...
10 years, 1 month ago (2014-02-07 20:17:36 UTC) #18
ejona
https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise/adaptor/fs/MockAdaptorContext.java File test/com/google/enterprise/adaptor/fs/MockAdaptorContext.java (right): https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise/adaptor/fs/MockAdaptorContext.java#newcode15 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 ...
10 years, 1 month ago (2014-02-08 01:54:22 UTC) #19
Brett
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() { On 2014/02/05 23:15:40, mifern wrote: > ...
10 years, 1 month ago (2014-02-08 03:23:31 UTC) #20
ejona
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() { On 2014/02/08 03:23:32, Brett wrote: > ...
10 years, 1 month ago (2014-02-10 20:55:28 UTC) #21
mifern
https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java#newcode136 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))? ...
10 years, 1 month ago (2014-02-11 02:30:31 UTC) #22
ejona
https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java#newcode136 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 ...
10 years, 1 month ago (2014-02-11 03:25:39 UTC) #23
Brett
Various Feedback
10 years, 1 month ago (2014-02-11 03:26:56 UTC) #24
Brett
https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right): https://codereview.appspot.com/58560043/diff/40001/test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java#newcode136 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 ...
10 years, 1 month ago (2014-02-12 23:29:40 UTC) #25
Brett
There has been no review feedback on this CL for a week. This is blocking ...
10 years, 1 month ago (2014-02-18 20:55:40 UTC) #26
pjo
Miguel, please give another round of feedback for FsAdaptorTest Tanmay please take a look at ...
10 years, 1 month ago (2014-02-18 21:53:53 UTC) #27
Tanmay Vartak
On 2014/02/18 21:53:53, pjo wrote: > Miguel, please give another round of feedback for FsAdaptorTest ...
10 years, 1 month ago (2014-02-18 22:19:01 UTC) #28
pjo
I think: # SP AccumulatingDocIdPusher should reset groups # SP versionshould return immutable named resources ...
10 years, 1 month ago (2014-02-18 22:30:06 UTC) #29
brett.michael.johnson_gmail.com
> AccumulatingDocIdPusher.java and UnsupportedDocIdPusher.java looks fine > to me. > > I have a question ...
10 years, 1 month ago (2014-02-18 22:33:38 UTC) #30
ejona
https://codereview.appspot.com/58560043/diff/60001/src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java File src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java (right): https://codereview.appspot.com/58560043/diff/60001/src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java#newcode219 src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:219: String[] parts = doc.getUniqueId().split("/", -1); Tanmay made a really ...
10 years, 1 month ago (2014-02-18 22:35:08 UTC) #31
brett.michael.johnson_gmail.com
When I look at FsAdaptor, it always passes in the pathname as the label to ...
10 years, 1 month ago (2014-02-18 22:52:35 UTC) #32
brett.michael.johnson_gmail.com
Once again, I was fooled by the FsAdaptor.getPathName() method that should really be called getFileName(). ...
10 years, 1 month ago (2014-02-18 23:39:45 UTC) #33
Brett
Fix HtmlResponseWriter.computeLabel splitter. Rename FsAdaptor.getPathName() to getFolderName() and make it work with root paths.
10 years, 1 month ago (2014-02-19 00:51:23 UTC) #34
mifern
Needs more work. https://codereview.appspot.com/58560043/diff/60001/src/com/google/enterprise/adaptor/fs/FsAdaptor.java File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/58560043/diff/60001/src/com/google/enterprise/adaptor/fs/FsAdaptor.java#newcode262 src/com/google/enterprise/adaptor/fs/FsAdaptor.java:262: if (monitor != null) { I ...
10 years, 1 month ago (2014-02-19 01:49:58 UTC) #35
Brett
https://codereview.appspot.com/58560043/diff/60001/src/com/google/enterprise/adaptor/fs/FsAdaptor.java File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/58560043/diff/60001/src/com/google/enterprise/adaptor/fs/FsAdaptor.java#newcode262 src/com/google/enterprise/adaptor/fs/FsAdaptor.java:262: if (monitor != null) { On 2014/02/19 01:49:59, mifern ...
10 years, 1 month ago (2014-02-19 04:43:52 UTC) #36
ejona
https://codereview.appspot.com/58560043/diff/80001/src/com/google/enterprise/adaptor/fs/FsAdaptor.java File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/58560043/diff/80001/src/com/google/enterprise/adaptor/fs/FsAdaptor.java#newcode464 src/com/google/enterprise/adaptor/fs/FsAdaptor.java:464: String getFolderName(Path file) { This new name is not ...
10 years, 1 month ago (2014-02-19 17:41:27 UTC) #37
Brett
Eric's feedback
10 years ago (2014-03-06 02:20:25 UTC) #38
pjo
Miguel, please take another look also. Thank you, PJ - technology's compounding interest - On ...
10 years ago (2014-03-06 02:30:53 UTC) #39
mifern
Changing the code to allow monitor to be null seems like the wrong approach to ...
10 years ago (2014-03-06 06:35:51 UTC) #40
ejona
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.
10 years ago (2014-03-06 17:30:26 UTC) #41
pjo
Brett, please insert a TODO to consider alternatives to checking for null monitor. I think ...
10 years ago (2014-03-06 22:06:54 UTC) #42
Brett
10 years ago (2014-03-07 22:32:55 UTC) #43
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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b