Bug 341323

Comment 1 Szymon Brandys 2011-03-30 05:00:09 EDT

File not out of sync: /buildtest/I20110329-0800/eclipse-testing/test-eclipse/eclipse/core_resources_sniff_folder/openProject/1/2/3/3 failure indicates that this may be related to the recent fix in refreshing. James will take a look at it.

Comment 2 James Blackburn 2011-03-30 05:38:31 EDT

This is interesting.  The Assertion is going off in ensureOutOfSync(IFile). This method does two things:
  
 1) Ensure that the modified stamp of the file != the cached mod time in core.resources
 2) Append to the file using java.io.File....  

Given that (1) succeeded, (2) likely failed because the File I/O caused the modification stamp to revert -- perhaps File I/O has a lower granularity than the #setLastModifiedTime API.  The alternative is that between (1) & (2) something caused the file to come back into sync (which I think is unlikely).

A number of the failures were a knock-on of the first failure.

The simple fix is to swap the order of (1) & (2) in ResourceTest.

Comment 3 Dani Megert 2011-03-30 05:59:09 EDT

Are we sure that this can't hit someone in real code? Also, why does the disabled lightweight auto-refresh affect the tests?

Comment 4 Szymon Brandys 2011-03-30 06:20:07 EDT

We have to make sure that our tests, real scenarios etc. are not affected when lightweight auto-refresh is disabled. So if the failure is caused but the recent fix somehow, I would expect to fix it in the code, not in the test.

Comment 5 James Blackburn 2011-03-30 06:41:34 EDT

(In reply to comment #3)
> Are we sure that this can't hit someone in real code? 

Behaviour should be entirely unchanged when the preference is off.  If you find a case where it isn't then do let us know!

> Also, why does the
> disabled lightweight auto-refresh affect the tests?

It shouldn't.  I've just confirmed that on reverting org.eclipse.core.tests.resources to v20110321-2120 (before the patch went in), the tests still pass against new core.resources.


To be clear the changes that were made to the tests as part of bug 303517 (  https://bugs.eclipse.org/bugs/attachment.cgi?id=191910):
 1) Push #touchInFilesystem(...) up to ResourceTest + unify with #ensureOutOfSync(...) to share implementation.  All tests which require out-of-sync now use the common method.  (An ordering issue caused this particular failure.)
 2) Test for Bug 186984 was beefed up to ensure we test the bug you reported
 3) Regression testsuite added for Bug 303517 
 4) IResourceTest added join on RefreshJob in setupBeforeState to ensure that tests continue to work even when lightweight refresh is switched on.  The change ensures that asynchronous refresh from one test doesn't interfere with subsequent tests -- the tests re-use the same set of IResources.

In summary the changes made were: a refactoring to share the ensure-out-of-sync method; add a new testsuite; a couple tweaks to ensure that the tests which don't care about refresh, continue to pass when refresh is enabled by default.

I'll commit a fix for the potential test regression introduced due to (1).

Comment 6 James Blackburn 2011-03-30 06:44:08 EDT

I've put the one-line fix into ResourceTest. 

Will continue to monitor tests for any other failures.

Comment 7 Szymon Brandys 2011-03-30 08:10:57 EDT

(In reply to comment #2)
> Given that (1) succeeded, (2) likely failed because the File I/O caused the
> modification stamp to revert -- perhaps File I/O has a lower granularity than
> the #setLastModifiedTime API.  The alternative is that between (1) & (2)
> something caused the file to come back into sync (which I think is unlikely).

That's interesting indeed. I can confirm that this is a problem with ResourceTest#ensureOutOfSync which did not make the resource out of sync for some reason. The patch looks good.