Bug 338529 - [working sets] Clean up WorkingSetDropAdapterTest
Summary: [working sets] Clean up WorkingSetDropAdapterTest
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.7   Edit
Hardware: All All
: P4 normal (vote)
Target Milestone: 3.7 RC1   Edit
Assignee: Raksha Vasisht CLA
QA Contact:
URL:
Whiteboard:
Keywords: test
Depends on:
Blocks:
 
Reported: 2011-03-01 08:12 EST by Dani Megert CLA
Modified: 2011-05-16 06:20 EDT (History)
1 user (show)

See Also:
daniel_megert: review+

Attachments
Patch (7.97 KB, patch)
2011-05-05 01:48 EDT, Raksha Vasisht CLA 		daniel_megert: review-
Details | Diff
Patch_2 (11.67 KB, patch)
2011-05-05 05:18 EDT, Raksha Vasisht CLA 		no flags Details | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Dani Megert CLA 2011-03-01 08:12:06 EST 
I20110301-0800.

WorkingSetDropAdapterTest is ugly and does not correctly create the working sets. Besides that I'd like to remove PackageExplorerPart.internalTestShowWorkingSets(IWorkingSet[]). Use the Accessor to access non public stuff.
Comment 1 Dani Megert CLA 2011-03-01 08:53:47 EST 
IMPORTANT: First fix bug 338531 and then this one.
Comment 2 Raksha Vasisht CLA 2011-05-05 01:48:00 EDT 
Created attachment 194790 [details]
Patch

Test cleaned up 
creation of working sets and code removed from PackageExplorerPart.java to the test using Accessor.
Comment 3 Raksha Vasisht CLA 2011-05-05 01:48:24 EDT 
Dani, could you pls review?
Comment 4 Dani Megert CLA 2011-05-05 04:46:20 EDT 
Comment on attachment 194790 [details]
Patch

- 'packageExplorerPart' is not a valid field name.
- I don't see the need for the second argument in 'createJavaWorkingSets'.
- The code is still ugly: AFAICS we always only put one element into the
  selection hence creating an array in all those test methods seems overkill.
  In addition we then loop over the 1 element array in 'createSelection'.
Comment 5 Raksha Vasisht CLA 2011-05-05 05:18:05 EDT 
Created attachment 194806 [details]
Patch_2

(In reply to comment #4)
> Comment on attachment 194790 [details] [diff]
> Patch

> - I don't see the need for the second argument in 'createJavaWorkingSets'.

I added it so that you can pass other arguments as well if needed. As it is now, it is not required.

> - The code is still ugly: AFAICS we always only put one element into the
>   selection hence creating an array in all those test methods seems overkill.
>   In addition we then loop over the 1 element array in 'createSelection'.

Oh didn't notice it was like that for all methods, removed the list. 

Patch_2 committed to HEAD.
Comment 6 Raksha Vasisht CLA 2011-05-05 05:33:07 EDT 
.
Comment 7 Dani Megert CLA 2011-05-05 05:34:12 EDT 
> Patch_2 committed to HEAD.
Take a look in HEAD for an improved version.
Comment 8 Dani Megert CLA 2011-05-16 06:20:00 EDT 
Verified in I20110512-2000 that the test is cleaned up and released into the map files.