Issue 57170048: Add WindowsFileDelegateTest (Closed)
|
Created:
10 years, 2 months ago by Brett Modified:
10 years, 2 months ago CC:
connector-cr_google.com Visibility:
Public. |
DescriptionThis adds a basic WindowsFileDelegateTest. It tests newDocId() and
most file system monitor functions using the local file system.
It only runs on Windows.
TODO: Add tests for ACL features.
TODO: Add tests for DFS features.
Patch Set 1 #
Total comments: 8
Patch Set 2 : PJ's feedback #
Total comments: 26
Patch Set 3 : Eric's and Miguel's Feedback. #
Total comments: 6
Patch Set 4 : Eric's and Miguel's Feedback #
Total comments: 8
Patch Set 5 : Eric's Feedback. #
Total comments: 2
MessagesTotal messages: 22
Only a cursory glance. https://codereview.appspot.com/57170048/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java (right): https://codereview.appspot.com/57170048/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:55: private BlockingQueue<Path> queue = new LinkedBlockingQueue<Path>(10); Why limit the queue to 10 elements? If that was an ArrayBlockingQueue that is required and makes sense. As a LinkedBlockingQueue it serves no purpose. https://codereview.appspot.com/57170048/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:60: queue.clear(); This clearing does nothing. https://codereview.appspot.com/57170048/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:270: queue.drainTo(changes, maxBatchSize); You should be interested if there are extra items in the queue. Thus, at the very least this should be an unlimited drainTo(changes).
Sign in to reply to this message.
Thank you. Is there a way to remove the Thread.sleep calls? Can we block somehow until monitor starts? https://codereview.appspot.com/57170048/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java (right): https://codereview.appspot.com/57170048/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:111: delegate.newDocId(Paths.get("\\\\host\\share\\foo\\bar")).getUniqueId()); over80
Sign in to reply to this message.
> Is there a way to remove the Thread.sleep calls? It is taking Windows anywhere between 1/4 second and more than 2 seconds to notify me of changes to the filesystem. Waiting 50ms for the monitor to start up is a drop in the bucket. https://codereview.appspot.com/57170048/diff/1/test/com/google/enterprise/ada... File test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java (right): https://codereview.appspot.com/57170048/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:55: private BlockingQueue<Path> queue = new LinkedBlockingQueue<Path>(10); On 2014/02/08 01:10:30, ejona wrote: > Why limit the queue to 10 elements? If that was an ArrayBlockingQueue that is > required and makes sense. As a LinkedBlockingQueue it serves no purpose. Done. https://codereview.appspot.com/57170048/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:60: queue.clear(); On 2014/02/08 01:10:30, ejona wrote: > This clearing does nothing. Done. https://codereview.appspot.com/57170048/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:111: delegate.newDocId(Paths.get("\\\\host\\share\\foo\\bar")).getUniqueId()); On 2014/02/08 01:29:14, pjo wrote: > over80 Done. https://codereview.appspot.com/57170048/diff/1/test/com/google/enterprise/ada... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:270: queue.drainTo(changes, maxBatchSize); On 2014/02/08 01:10:30, ejona wrote: > You should be interested if there are extra items in the queue. Thus, at the > very least this should be an unlimited drainTo(changes). This is what prompted the CL to BlockingQueueBatcher. I was trying to verify that queue is empty, then noticed that I was getting 3 notifications for the same file. I think the unlimited drainTo(changes) is the right thing to do here. If there were unexpected changes, the assertEquals comparing the sets will fail.
Sign in to reply to this message.
PJ's feedback
Sign in to reply to this message.
> Waiting 50ms for the monitor > to start up is a drop in the bucket. I'm not concerned with the walltime but with potential flakiness. Thank you - technology's compounding interest - On Fri, Feb 7, 2014 at 7:01 PM, <Brett.Michael.Johnson@gmail.com> wrote: > Is there a way to remove the Thread.sleep calls? >> > > It is taking Windows anywhere between 1/4 second and more than 2 seconds > to notify me of changes to the filesystem. Waiting 50ms for the monitor > to start up is a drop in the bucket. > > > > https://codereview.appspot.com/57170048/diff/1/test/com/ > google/enterprise/adaptor/fs/WindowsFileDelegateTest.java > File test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java > (right): > > https://codereview.appspot.com/57170048/diff/1/test/com/ > google/enterprise/adaptor/fs/WindowsFileDelegateTest.java#newcode55 > test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:55: > private BlockingQueue<Path> queue = new LinkedBlockingQueue<Path>(10); > On 2014/02/08 01:10:30, ejona wrote: > >> Why limit the queue to 10 elements? If that was an ArrayBlockingQueue >> > that is > >> required and makes sense. As a LinkedBlockingQueue it serves no >> > purpose. > > Done. > > > https://codereview.appspot.com/57170048/diff/1/test/com/ > google/enterprise/adaptor/fs/WindowsFileDelegateTest.java#newcode60 > test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:60: > queue.clear(); > On 2014/02/08 01:10:30, ejona wrote: > >> This clearing does nothing. >> > > Done. > > > https://codereview.appspot.com/57170048/diff/1/test/com/ > google/enterprise/adaptor/fs/WindowsFileDelegateTest.java#newcode111 > test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:111: > delegate.newDocId(Paths.get("\\\\host\\share\\foo\\bar")).getUniqueId()); > On 2014/02/08 01:29:14, pjo wrote: > >> over80 >> > > Done. > > > https://codereview.appspot.com/57170048/diff/1/test/com/ > google/enterprise/adaptor/fs/WindowsFileDelegateTest.java#newcode270 > test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:270: > queue.drainTo(changes, maxBatchSize); > On 2014/02/08 01:10:30, ejona wrote: > >> You should be interested if there are extra items in the queue. Thus, >> > at the > >> very least this should be an unlimited drainTo(changes). >> > > This is what prompted the CL to BlockingQueueBatcher. > I was trying to verify that queue is empty, then noticed > that I was getting 3 notifications for the same file. > I think the unlimited drainTo(changes) is the right thing > to do here. If there were unexpected changes, the > assertEquals comparing the sets will fail. > > https://codereview.appspot.com/57170048/ >
Sign in to reply to this message.
https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java (right): https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:115: public void testNewDocIdLocalUncPaths() throws Exception { Does this require a special setup on the machine you are running on?
Sign in to reply to this message.
Needs more work. The wait(100) is not ideal since it does not guarantee that the thread has started and that the watch point has been set. You may have to update the monitor code to setup a callback/signal for after the watch point has been set. https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java (right): https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:88: assertTrue(id.contains(root.toString().replace('\\', '/'))); Shouldn't this be id.equals(...)? https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:92: assertTrue(id.contains(root.toString().replace('\\', '/'))); Shouldn't this be id.startsWith(...)? https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:93: assertTrue(id.contains(dir.toString().replace('\\', '/'))); Shouldn't this be id.equals(...)? https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:97: assertTrue(id.contains(root.toString().replace('\\', '/'))); Shouldn't this and the next line be id.startsWith(...)? https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:99: assertTrue(id.contains(file.toString().replace('\\', '/'))); Shouldn't this be id.equals(...)? https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:104: public void testNewDocIdVirtualUncPaths() throws Exception { Since we're using getTempRootAsUncPath(), you might as well use it to confirm these with valid data and that folders end with a "/" and files don't. https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:145: // Cannot access local administrative share. Shouldn't the test fail here since the tests are depending on a valid UNC path? https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:161: Thread.sleep(100L); // Let the MonitorThread start up. We should probably include a callback/signal from the thread after it starts watching the path. You need guarantee that the thread started and that the watch point has been setup.
Sign in to reply to this message.
I see absolutely no way to wait for changes, other than what is done in checkForChanges(). This CL looks very reasonable. https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java (right): https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:161: Thread.sleep(100L); // Let the MonitorThread start up. Why let it start up? I'm not sure what it changes. It seems it would work identically without the sleep. https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:166: public void testMonitorNotFindExistingFiles() throws Exception { I feel like this would be a better test if it was combined with AddFile, since then you feel like you had a chance of waiting long enough for the modification event. https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:190: checkForChanges(Sets.<Path>newHashSet(tempRoot, file)); Specifying <Path> here and the two tests below doesn't seem like it should be necessary. Huh. https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:248: Thread.sleep(50L); // Let the MonitorThread start up. Ah, this is an interesting situation. It seems reasonable to make startMonitorPath() block until the watch is actually started, and I think that that semantic would be feasible to keep even on the Linux change detection. That could be achieved with a CountDownLatch. Call countDown() between ReadDirectoryChangesW and WaitForSingleObjectEx, and have an await() in startMonitorPath() after the start(). The implementation of countDown() is very fast if the count is already zero. https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:249: assertTrue(queue.isEmpty()); This doesn't seem necessary. It is already tested in testMonitorNotFindExistingFiles(). If it can be removed as well, then we would just do delegate.startMonitorPath() in each of the tests instead of having the startMonitor helper.
Sign in to reply to this message.
https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java (right): https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:92: assertTrue(id.contains(root.toString().replace('\\', '/'))); On 2014/02/11 01:30:45, mifern wrote: > Shouldn't this be id.startsWith(...)? Done. https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:93: assertTrue(id.contains(dir.toString().replace('\\', '/'))); On 2014/02/11 01:30:45, mifern wrote: > Shouldn't this be id.equals(...)? Again, trailing slash added. changed to startsWith. https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:97: assertTrue(id.contains(root.toString().replace('\\', '/'))); On 2014/02/11 01:30:45, mifern wrote: > Shouldn't this and the next line be id.startsWith(...)? Done. https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:99: assertTrue(id.contains(file.toString().replace('\\', '/'))); On 2014/02/11 01:30:45, mifern wrote: > Shouldn't this be id.equals(...)? Done. https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:104: public void testNewDocIdVirtualUncPaths() throws Exception { On 2014/02/11 01:30:45, mifern wrote: > Since we're using getTempRootAsUncPath(), you might as well use it to confirm > these with valid data and that folders end with a "/" and files don't. These are virtual paths of non-existent files. isDirectory() and isRegularFile() both return false. https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:115: public void testNewDocIdLocalUncPaths() throws Exception { On 2014/02/11 00:01:44, ejona wrote: > Does this require a special setup on the machine you are running on? Not for me. But conceivably, the logged in user might not be able to see administrative shares. https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:145: // Cannot access local administrative share. On 2014/02/11 01:30:45, mifern wrote: > Shouldn't the test fail here since the tests are depending on a valid UNC path? I'm using Assume to skip the the test if the user does not have sufficient permission to see administrative shares. https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:161: Thread.sleep(100L); // Let the MonitorThread start up. On 2014/02/11 01:30:45, mifern wrote: > We should probably include a callback/signal from the thread after it starts > watching the path. You need guarantee that the thread started and that the watch > point has been setup. Done. https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:166: public void testMonitorNotFindExistingFiles() throws Exception { On 2014/02/11 01:35:39, ejona wrote: > I feel like this would be a better test if it was combined with AddFile, since > then you feel like you had a chance of waiting long enough for the modification > event. Done. https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:190: checkForChanges(Sets.<Path>newHashSet(tempRoot, file)); On 2014/02/11 01:35:39, ejona wrote: > Specifying <Path> here and the two tests below doesn't seem like it should be > necessary. Huh. Done. https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:248: Thread.sleep(50L); // Let the MonitorThread start up. On 2014/02/11 01:35:39, ejona wrote: > Ah, this is an interesting situation. It seems reasonable to make > startMonitorPath() block until the watch is actually started, and I think that > that semantic would be feasible to keep even on the Linux change detection. > > That could be achieved with a CountDownLatch. Call countDown() between > ReadDirectoryChangesW and WaitForSingleObjectEx, and have an await() in > startMonitorPath() after the start(). The implementation of countDown() is very > fast if the count is already zero. Done. https://codereview.appspot.com/57170048/diff/20001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:249: assertTrue(queue.isEmpty()); On 2014/02/11 01:35:39, ejona wrote: > This doesn't seem necessary. It is already tested in > testMonitorNotFindExistingFiles(). If it can be removed as well, then we would > just do delegate.startMonitorPath() in each of the tests instead of having the > startMonitor helper. I kept the helper method because the CountDownLatch added a couple of more lines. I removed the isEmpty() check, because if it isn't empty any test will fail to match the expected results.
Sign in to reply to this message.
Eric's and Miguel's Feedback.
Sign in to reply to this message.
Eric's and Miguel's Feedback.
Sign in to reply to this message.
A couple of additional tests are needed. https://codereview.appspot.com/57170048/diff/60001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java (right): https://codereview.appspot.com/57170048/diff/60001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:215: Need test for move and rename directory. https://codereview.appspot.com/57170048/diff/60001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:235: Need test for modify a file and folder Acl. That should give the same result modifying a file.
Sign in to reply to this message.
https://codereview.appspot.com/57170048/diff/60001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/WindowsFileDelegate.java (right): https://codereview.appspot.com/57170048/diff/60001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/WindowsFileDelegate.java:227: startMonitorPath(watchPath, queue, new CountDownLatch(0)); Why have the two versions of this method? It seems that CountDownLatch is internal, and you should always just have a CountDownLatch(1). Then as part of startMonitorPath, you do await(). It seems it doesn't hurt anything to always wait for the other thread to start. Did you do it this way because of the InterruptedException? If so, you can do the same pattern as done in MonitorThread.shutdown(). If you were concerned with run() failing, you could extend the try-catch around runMonitorLoop() to have a finally that does countDown().
Sign in to reply to this message.
Eric's and Miguel's Feedback
Sign in to reply to this message.
https://codereview.appspot.com/57170048/diff/60001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/WindowsFileDelegate.java (right): https://codereview.appspot.com/57170048/diff/60001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/WindowsFileDelegate.java:227: startMonitorPath(watchPath, queue, new CountDownLatch(0)); On 2014/02/11 03:21:38, ejona wrote: > Why have the two versions of this method? It seems that CountDownLatch is > internal, and you should always just have a CountDownLatch(1). > > Then as part of startMonitorPath, you do await(). It seems it doesn't hurt > anything to always wait for the other thread to start. > > Did you do it this way because of the InterruptedException? If so, you can do > the same pattern as done in MonitorThread.shutdown(). > > If you were concerned with run() failing, you could extend the try-catch around > runMonitorLoop() to have a finally that does countDown(). Done. https://codereview.appspot.com/57170048/diff/60001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java (right): https://codereview.appspot.com/57170048/diff/60001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:215: On 2014/02/11 03:17:33, mifern wrote: > Need test for move and rename directory. Done. https://codereview.appspot.com/57170048/diff/60001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:235: On 2014/02/11 03:17:33, mifern wrote: > Need test for modify a file and folder Acl. That should give the same result > modifying a file. ACL related tests (including detecting ACL changes) are still a TODO.
Sign in to reply to this message.
https://codereview.appspot.com/57170048/diff/80001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/WindowsFileDelegate.java (right): https://codereview.appspot.com/57170048/diff/80001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/WindowsFileDelegate.java:17: import com.google.common.annotations.VisibleForTesting; Unused. https://codereview.appspot.com/57170048/diff/80001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java (right): https://codereview.appspot.com/57170048/diff/80001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:43: import java.util.concurrent.CountDownLatch; Unused https://codereview.appspot.com/57170048/diff/80001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:55: private WindowsFileDelegate delegate = new WindowsFileDelegate(); Can be FileDelegate again, although it doesn't matter. https://codereview.appspot.com/57170048/diff/80001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:272: long maxLatencyMillis = 10000; With a per-test duration of 10 seconds, this gets to be a very expensive set of tests. Would it be good to combine tests together so we only have to checkForChanges a couple times? If we name the files based on what they are testing, then it wouldn't necessarily be all that much more difficult to see what broke if the test fails.
Sign in to reply to this message.
https://codereview.appspot.com/57170048/diff/80001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/WindowsFileDelegate.java (right): https://codereview.appspot.com/57170048/diff/80001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/WindowsFileDelegate.java:17: import com.google.common.annotations.VisibleForTesting; On 2014/02/13 19:22:10, ejona wrote: > Unused. Done. https://codereview.appspot.com/57170048/diff/80001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java (right): https://codereview.appspot.com/57170048/diff/80001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:55: private WindowsFileDelegate delegate = new WindowsFileDelegate(); On 2014/02/13 19:22:10, ejona wrote: > Can be FileDelegate again, although it doesn't matter. Done. I like to keep it as the Interface, if I can, since it makes it obvious that I am making no assumptions about the implementation. https://codereview.appspot.com/57170048/diff/80001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:272: long maxLatencyMillis = 10000; On 2014/02/13 19:22:10, ejona wrote: > With a per-test duration of 10 seconds, this gets to be a very expensive set of > tests. > > Would it be good to combine tests together so we only have to checkForChanges a > couple times? If we name the files based on what they are testing, then it > wouldn't necessarily be all that much more difficult to see what broke if the > test fails. The timeout is the *maximum* amount of time I will wait for notifications. Typically, each test completes in less than 1/3 second, with an occasional outlier on my slow VM (note testStartStopMonitor on my last run): [junit] Testcase: testStartMonitorBadPath took 0.015 sec [junit] Testcase: testStartStopMonitor took 6.724 sec [junit] Testcase: testMonitorAddFile took 0.125 sec [junit] Testcase: testMonitorDeleteFile took 0.078 sec [junit] Testcase: testMonitorModifyFileAttributes took 0.327 sec [junit] Testcase: testMonitorRenameFile took 0.078 sec [junit] Testcase: testMonitorMoveAccrossDirs took 0.094 sec [junit] Testcase: testMonitorModifyFile took 0.25 sec [junit] Testcase: testMonitorRenameDir took 0.047 sec [junit] Testcase: testMonitorMoveDir took 0 sec [junit] Testcase: testMonitorChangesInSubDirs took 0.109 sec
Sign in to reply to this message.
Eric's Feedback.
Sign in to reply to this message.
LGTM. Please wait for Miguel's LGTM https://codereview.appspot.com/57170048/diff/80001/test/com/google/enterprise... File test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java (right): https://codereview.appspot.com/57170048/diff/80001/test/com/google/enterprise... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:272: long maxLatencyMillis = 10000; On 2014/02/13 23:54:51, Brett wrote: > The timeout is the *maximum* amount of time I will wait for notifications. I was just thinking that it was behaving that slowly, since you kept having to bump the timeout. > Typically, each test completes in less than 1/3 second, with an occasional > outlier on my slow VM (note testStartStopMonitor on my last run): Ah, those numbers look pretty reasonable. > [junit] Testcase: testStartStopMonitor took 6.724 sec That looks like a strange amount of time for that test. Maybe it has to initialize some JNA stuff and so gets some costs assigned to it. https://codereview.appspot.com/57170048/diff/100001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java (right): https://codereview.appspot.com/57170048/diff/100001/test/com/google/enterpris... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:43: import java.util.concurrent.CountDownLatch; Still unused.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
Committed 14 February 2014 To https://code.google.com/p/plexi.fs/ 6c240cd..920b11a master -> master https://codereview.appspot.com/57170048/diff/100001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java (right): https://codereview.appspot.com/57170048/diff/100001/test/com/google/enterpris... test/com/google/enterprise/adaptor/fs/WindowsFileDelegateTest.java:43: import java.util.concurrent.CountDownLatch; On 2014/02/14 21:58:18, ejona wrote: > Still unused. Done.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
