Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 261931

Summary: [tests] UI Tests hang waiting for input
Product: [Eclipse Project] Equinox Reporter: Ian Bull <irbull>
Component: p2Assignee: Susan McCourt <susan>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: john.arthorne, pascal, simon_kaegi, susan
Version: unspecified   
Target Milestone: 3.5 M6   
Hardware: PC   
OS: Windows XP   
Whiteboard:

Description Ian Bull CLA 2009-01-21 18:10:10 EST
ui.dialgos.InstallWizardTest hangs waiting for input (restart Eclipse dialog). If you hit "apply" it finishes and passes.
Comment 1 Susan McCourt CLA 2009-01-21 19:12:39 EST
This is new, but I can't say exactly when it appeared because I was out most of last week and I did not run the ui tests from HEAD until today.

In debugging this, I can see that it's not anything that's happening in the InstallWizardTest per se.  There is a ProfileEvent (CHANGED) for the self profile on the event bus, and this event is serviced during the test, which causes an async request restart.

I can fix this by ignoring restart requests while the tests are running.  

But I'm wondering why this event has suddenly appeared, and why.
The UI tests generally don't change the SELF profile, so I'm kind of surprised.

cc'ing Simon.  Can you think of any changes in profiles or the engine that might cause ProfileEvents to be triggered whereas they weren't before?  Or any changes that could cause profile changes to the test workbench's SELF profile?  And why haven't we seen this in builds?
Comment 2 Susan McCourt CLA 2009-01-21 19:22:07 EST
(In reply to comment #1)
> And why haven't we seen this in builds?

Actually, in the N20090120-2000 the ui tests DNF, presumably for this reason.
They have been finishing in the prior night's build and before.  So something has been introduced since N20090119-2000 that causes a profile change event on the SELF profile of the test instance.

This is reproducible by only running the ui tests, so it's not something introduced by the core tests.  It must be a change in the UI tests where the self profile is being hammered, or else a bogus event being published.  

Comment 3 Susan McCourt CLA 2009-01-21 19:30:18 EST
found it.
A new test was added, IUPropertyUtilsTest.testLocalizedLicense, which installs a bunch of stuff into the self profile.  I would rather see us use a test profile here, unless it's somehow important to the test that the self profile is used (I don't think it is).

I'd rather not modify the UI to have a "test mode" for the restart dialog until we determine that it's important to test modifications to the self profile.

It seems to me that using the self profile in tests could introduce subtle timing bugs and differences in the tests based on which tests are being run, esp if a certain state of the self profile is assumed.

Pascal, is it possible to use a newly created test profile for this case?
Comment 4 Ian Bull CLA 2009-01-21 19:40:07 EST
(In reply to comment #3)
> found it.
> A new test was added, IUPropertyUtilsTest.testLocalizedLicense, which installs
> a bunch of stuff into the self profile.  I would rather see us use a test
> profile here, unless it's somehow important to the test that the self profile
> is used (I don't think it is).

Sorry that would have been me :(.  The reason I did that was because the actual code that I was testing only looks in the Self profile.  The localization stuff searches around the Self profile:

This line of doc in IUPropertyUtils explains it:
//Due to performance problems we restrict locale lookup to the current profile (see bug 233958)

The test case can be disabled, however there were no other test cases for this stuff.
Comment 5 Ian Bull CLA 2009-01-21 19:41:35 EST
in came in as part of Bug 261278
Comment 6 Susan McCourt CLA 2009-01-21 19:50:40 EST
I've disabled the test for now and marked this for 3.5 (not M5).
It sounds like the SELF profile is needed given what you've said and I think having the localization tests is good.  I just need to set up something in ProvisioningOperationRunner so that it can be set into a test mode which doesn't prompt for restart.  
Comment 7 Ian Bull CLA 2009-01-21 19:54:58 EST
(In reply to comment #6)
> I've disabled the test for now and marked this for 3.5 (not M5).
> It sounds like the SELF profile is needed given what you've said and I think
> having the localization tests is good.  I just need to set up something in
> ProvisioningOperationRunner so that it can be set into a test mode which
> doesn't prompt for restart.  
> 
+1.  thanks. Sorry about causing this. I guess I saw the problem last week, but didn't equate it with my test case.  (I just assumed there was a problem with my Eclipse Install).

At least the bug (that the test cases demonstrates) is fixed, and that's the important thing :-).

Comment 8 Ian Bull CLA 2009-01-21 19:56:37 EST
sorry, I didn't mean to remove you from the assignee.  

The Reassign radio button is too close to the submit button :-). I'm causing too many problems today, time to head home.
Comment 9 Susan McCourt CLA 2009-01-21 20:01:16 EST
No worries, Ian.  More tests are good, this was a subtle one.  
Comment 10 Simon Kaegi CLA 2009-01-21 21:16:51 EST
I'm trying to figure out if there is still something to do here. There indeed was a change done in last night's build in the profile registry that made it so that we would wait for a profile lock instead of just failing immediately. While an engine operation is going on the profile is locked and any pending change requests are blocked until the original request is complete. Is there anything like that going on in this test scenario or has it been resolved in some other way?
Comment 11 Ian Bull CLA 2009-01-21 22:10:47 EST
Simon,

What appears to have happened was that I wrote a test case that added a bunch of stuff to the Self profile.  Because of this, the "restart eclipse dialog" was presented (in a separate test case), when the UI Test case was not expecting it.  Since the dialog was not expected, it was not handled properly, and the test suite hung.

I'm unsure why my test case would cause this, but it does.  

So in short, it doesn't appear that the change you're talking about has any bearing on this.  The correct path forward is to fix the test suite (to handle the restart dialog), but that may take some time.  In the meantime, my problematic test case has been removed.
Comment 12 Pascal Rapicault CLA 2009-01-21 23:01:59 EST
I think I had seen this behavior independently of the engine lock change. No worry Simon

Comment 13 Susan McCourt CLA 2009-01-22 00:06:27 EST
(In reply to comment #11)
> Simon,
> 
> What appears to have happened was that I wrote a test case that added a bunch
> of stuff to the Self profile.  Because of this, the "restart eclipse dialog"
> was presented (in a separate test case), when the UI Test case was not
> expecting it.  Since the dialog was not expected, it was not handled properly,
> and the test suite hung.
> 
> I'm unsure why my test case would cause this, but it does. 

The new test case introduced the restart dialog because it modified the self profile.  It is the only test case that does this.  Changes to the self profile are what trigger the restart dialog in the p2 UI.  Since the profile change events are dispatched async (and the restart dialog is then opened async in the UI thread), it's just a matter of timing...in some later test case, the event(s) will be processed and the restart dialog opened as a result of the event.

It's not so much that we don't handle the dialog properly, it's just that we don't want a modal dialog launched in the middle of testing.  So we need to set up a test mode in the p2 UI whereby self profile changes don't prompt for restart.  I had never worried about this before because no other tests modify the self profile of the test launch.
Comment 14 Susan McCourt CLA 2009-01-31 13:10:10 EST
fixed in HEAD >20090131.
Added a "suppressRestart" flag to ProvisioningOperationRunner and this flag is now set by the UI tests.  This will prevent any self profile changes from triggering restart in the test runs.  Reenabled the license test.