Bugzilla – Full Text Bug Listing
| Summary: | InstallWizardTest#testInstallWizardUnresolved is failing | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | Pascal Rapicault <pascal> | ||||||
| Component: | p2 | Assignee: | Susan McCourt <susan> | ||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | john.arthorne | ||||||
| Version: | 3.6 | Flags: | john.arthorne:
review+
pascal: review+ |
||||||
| Target Milestone: | 3.6 RC2 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Mac OS X - Carbon (unsup.) | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Pascal Rapicault
This has been introduced by the fix to bug #310667. Created attachment 168561 [details]
Attempt at fixing the problem
The issue is that when we go and check for the status of the operation (wizard.recomputePlan(dialog); line 159 of InstallWizardtest), the call to ProvisioningOperation.getCurrentStatus takes into the account the value of installHandlerStatus (defined in InstallWizard) that was from the previous run of the operation.
This patch, forces installHandlerStatus to be reset to null when I recompute the plan. I'm not sure if there is a better place.
Do we need to fix this in rc1 or can it wait for rc2?
Pascal pointed out that this test failure actually indicates a real problem in the code. A regression introduced by the fix for bug 310667. Here's the scenario. - Help->Install New Software... - select Helios - check Collaboration->Mylyn Task List - Next (resolves) OK - Back - Now also check the category Linux Tools - Next !! This should fail to resolve, but it does not. If you were to start over in a new wizard and check both up front, it would fail to resolve. The problem here is that the "installHandlerStatus" is only supposed to be consulted when it is not OK. In Line 194 of the InstallWizard we do this: if (!installHandlerStatus.isOK()) { -----> couldNotResolveStatus = installHandlerStatus; The intention of the method statusOverridesOperation() is that only if we had encountered a non-OK installHandler status would we have set this into the other variable. So the check for whether this status mattered used identify (couldNotResolveStatus == installHandlerStatus). It seemed so safe at the time. The status would have to have been explicitly set during the current resolution for it to matter. What is going wrong here is that if the install handler status is ok, we use Status.OK_STATUS. So we don't set it into the variable. But the wizard itself will also have a Status.OK_STATUS, so in the end, the comparison matches on an OK status. Created attachment 168830 [details]
patch
The first patch from Pascal set the installHandlerStatus to null to ensure that a past install handler status wasn't consulted on a subsequent check. This solved the problem in both the test case and the bug described here.
However, this makes the solution timing-dependent, because the wizard can still get into a state *after* resolution whereby the
noChangeRequest status is Status.OK and the
installHandler status is Status.OK,
so a caller would get a wizard status of OK, when in fact the operation had a resolution problem that should be consulted.
This patch makes the intention of
statusOverridesOperation()
more explicit. Note that the comment was trying to describe on the knowledge being used about when the noChangeRequest status was being reset. But it didn't consider that when both are OK, they are identical.
More importantly, though, it didn't consider that if the installHandlerStatus is OK, it should *never* be consulted because it is not more important than the operation's resolution status.
However, I like this patch better because it
Pascal & John - can you please review. With this patch, I tested that: - InstallWizardTest#testInstallWizardUnresolved is passing - the scenario described in comment #3 is fixed - the scenario in bug 310667 is still working (the bug that introduced the regression) - the scenarios in bug 300441 are still working (further sanity checking) - I tried scenarios like bug 300441 that ensured that the feature with the install handler was in the selections. For example: - add the site http://www.developer.com/img/2007/03/upd-install-site.xml - choose "all available sites" in the install wizard - ensure that Feature Feature 1.0 and something else (I used releng tools) is installed - next - fails due to install handler pb (good) - back - uncheck releng tools - still fails due to install handler pb (and prompts again, which one could argue is a bug, but that is the intention). - back - check releng tools, uncheck Feature Feature 1.0 - next - success In other words, I was verifying that the non-OK status of an install handler (when an actual conflict does occur) is still working in addition to the cases that were broken in this bug I've applied the patch and went through the problematic scenario I found today and it was fine. Released. |