Bugzilla – Full Text Bug Listing
| Summary: | [JUnit] Superclass entry is unnecessarily disabled for new JUnit Test Case Wizard | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Justin Corpron <justincemail-eclipse> | ||||||
| Component: | UI | Assignee: | Raksha Vasisht <raksha.vasisht> | ||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||
| Severity: | trivial | ||||||||
| Priority: | P3 | CC: | daniel_megert, deepakazad, markus.kell.r, rthakkar | ||||||
| Version: | 3.4.2 | Flags: | markus.kell.r:
review-
|
||||||
| Target Milestone: | 3.7 M2 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Linux | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Justin Corpron
Raksha, could you please have a look? The superclass should be editable for both kinds of tests. When the user switches between 3 and 4, the superclass should be updated unless the user already modified it (if field is empty, also set the new default). Created attachment 176299 [details]
Patch
Enable the super class field for JUnit4 and sets the appropriate default values when toggled.
(In reply to comment #2) > Created an attachment (id=176299) [details] I don't like the new API NewTypeWizardPage#enableBrowseButtonForSuperClass(): - The method does not always enable the button, so it would need a better name (something with setEnablement or update, ...) - I don't think it's necessary. NewTestCaseWizardPageOne#superClassChanged() should respect the contract of the super method and not override that method, but extend it (i.e. call super.superClassChanged() in all cases). Behavior-wise, we should also set the default superclass in case the user left the superclass field empty. That way, the user can e.g. recover when he accidentally changed the superclass and then switches to JUnit 3. (In reply to comment #3) > - I don't think it's necessary. NewTestCaseWizardPageOne#superClassChanged() > should respect the contract of the super method and not override that method, > but extend it (i.e. call super.superClassChanged() in all cases). > > Behavior-wise, we should also set the default superclass in case the user left > the superclass field empty. That way, the user can e.g. recover when he > accidentally changed the superclass and then switches to JUnit 3. Done. Made the changes and committed to HEAD. Created attachment 178700 [details]
patch committed
(In reply to comment #5) > Created an attachment (id=178700) [details] [diff] The code flow in init(..) is too convoluted. setJUnit4(..) now calls setSuperClass(..) only sometimes, and init(..) calls setSuperClass(..) again. Setting the superclass once should be enough. I also don't get why fSuperClassExplicitlySet is necessary. You should avoid this redundant state field and just compare the current value with the defaults to find out whether it's the default or not. superClassChanged() should not throw away the result of the super.superClassChanged() call. If necessary, use MultiStatus to merge multiple problems (but you should just return the super status in case it is an error). (In reply to comment #6) > You should avoid this redundant state > field and just compare the current value with the defaults to find out whether > it's the default or not. > Yeah! Its slightly easier than setting the default only when the user has not modified the superclass field in anyway, since we also set the defaults when the string is empty. > superClassChanged() should not throw away the result of the > super.superClassChanged() call. If necessary, use MultiStatus to merge multiple > problems (but you should just return the super status in case it is an error). I was only using it to set the enablement of Browse button, but the checks could be reused as well. Thanks Markus! Fixed in HEAD. Verified in I20100914-0100. Verified for 3.7M2 on Linux with I20100914-0100. |