Intermittent build failure in NewUserPageTest

I don't know how to solve this one honestly, never managed to reproduce it locally. I also did not code that page, looked into it but could not easily guess where the problem was.

I just managed to reproduce it! In Eclipse, run NewUserPageTest.testFill() as JUnit test. In the JUnit runner, press the re-run button. Keep re-running the test and it will fail every now and then.

Got it. PropertyFileWatcher strikes again. What is happening is that the test creates a new user, this configuration option is written to a property file, and then the test triggers a reload of the state from the property file. However, there is a check in PropertyFileWatcher that it is never stale unless 1000 milliseconds have passed. When the test fails, only 997 milliseconds have passed, so the file is (according to PropertyFileWatcher) unchanged, and the user does not exist.

I propose inserting a 1000 millisecond sleep in the test, as the test-for-existence occurs unrealistically soon after the property file is written.

Doh, just rerun it 10 times in a row and could not reproduce.
But yeah, your analisys makes sense, adding a delay would work. The 1000ms trick is there to avoid the slowdown of continuously checking the file timestamp in benchmarks (it visibly hurts them)

Confirmed on trunk.

Attached patch (for trunk) adds a 1000 millisecond sleep to avoid the race condition. I would like to apply it to trunk and 2.0.x.

Go ahead!

Patch committed in r14436 on trunk and r14437 on 2.0.x.

Thanks, Andrea.

Still failing on trunk.

Assigned to Andrea for advice:

Oh, my. What a bag of bugs. Where do I start?
(1) My original patch is ineffective. This is not a race condition that causes the test to fail. It is the race condition that causes the saved users.properties to be read from disk and successfully reveal that the underlying code to save the file is broken. The test is broken and does not test anything unless the race condition occurs.
(2) GeoserverUserDao.reload() has no effect at all, because it nulls out userDefinitionsFile, but because userMap is already set, checkUserMap() never has any effect. Nulling out userMap in GeoserverUserDao.reload() causes NewUserMapTest to fail every time, removing the race condition by demonstrating just how broken the underlying code is.
I have a patch for GeoserverUserDao that fixes the reload() logic, but this method is only used in testFill().

The underlying problem is that the test user is written with no authorities, so in the rare (race condition) event that the user.properties file is read, that line is ignored, because an acegi UserAttribute with zero authorities is invalid, and the test fails.

Can we just remove this test method [NewUserPageTest.testFill()]? The comments suggest that the palette behaviour prevents authority testing. The test tests nothing because GeoserverUserDao.reload() is broken, and even if fixed, the palette behaviour makes NewUserPageTest.testFill() fail.

Attached a patch to fix GeoserverUserDao.checkUserMap() logic that causes GeoserverUserDao.reload() to work properly. This patch also causes NewUserPageTest.testFill() to fail reliably , so NewUserPageTest.testFill() should also be removed.

Attached patch removes the problematic NewUserPageTest.testFill() method.

Ben, I'm committing a patch that fixes both the test (without removing it) and the dao. Can you try it?

Andrea, that is excellent work. You have fixed the underlying authorities problem, resurrected the test, and also salvaged another test. Well done. I tried my hardest to break testFill() and could not. Please ensure your patches are applied to 2.0.x and trunk.

There is one remaining issue: GeoserverUserDao.reload() still has no effect because the logic error in checkUserMap() remains. Setting userDefinitionsFile to null does not cause the property file to be reloaded. The test is instead checking the user map that is already present in memory. To ensure the test actually tests reloading the user map, please apply the patch attached to this issue:
http://jira.codehaus.org/secure/attachment/49350/GEOS-3982.GeoserverUserDao.patch

With this patch, the property file is reloaded and can never be stale because a new PropertyFileWatcher instance is constructed, so the original race condition is gone.

Would you like me to commit this patch?

Sure, go ahead and commit, I'm about to leave for the day and I won't be able to until tomorrow. If you feel like it and want to backport too, be my guest

Committed GEOS-3982.GeoserverUserDao.patch to trunk as r14462.

Andrea, it would be a shame to not backport your patch, so I committed it to 2.0.x in r14463. I also committed GEOS-3982.GeoserverUserDao.patch to 2.0.x as r14464.