Bug 511443

Comment 1 Noopur Gupta 2017-02-01 04:57:34 EST

See bug 287688 comment #2 also. Fix it along with this bug by removing existing quick fixes and adding the following to the favorites list:

- org.junit.Assert.*
- org.junit.Assume.*

Comment 2 Noopur Gupta 2017-02-01 06:12:56 EST

Created attachment 266558 [details]
Patch

Markus, could you please provide your comments on the following?

From bug 287688:

> The only problem is that there's currently no official way for plug-ins like
> jdt.junit to contribute default favorite static classes.

Can we use PreferenceConstants.getPreferenceStore().setValue(PreferenceConstants.CODEASSIST_FAVORITE_STATIC_MEMBERS, favorites) for this?

> jdt.junit should set default static imports the first time the plug-in is 
> loaded. 

Should this be done in JUnitPlugin.start(BundleContext context)?

> Set another (internal) preference to avoid adding these types 
> again and again.

I didn't understand this properly. These types will be added only once when the plug-in is loaded for the first time in #start. So, where should we add the internal preference and what should it do?

Attaching the patch from BETA_JUNIT5 branch which implements the above. Please have a look.

Comment 3 Markus Keller 2017-02-06 11:11:58 EST

We should not assume that every user always wants all these static import favorites in their workspace.

It's OK to add the proposed classes to the preferences by default, but if a user removes a static import in the preferences, it must never be added back automatically.

For new workspaces, we can just use PreferenceConstants.getPreferenceStore() and call set*Default*Value(..) to add the JUnit imports as defaults to CODEASSIST_FAVORITE_STATIC_MEMBERS.

However, if users already modified the preference, they won't see the new defaults unless they manually click Restore Defaults. To fix that, we can check whether the preference is still set to the default value. If it's not, we can run some migration code that adds the new favorites using setValue(..). But we must only perform that check/add a single time per workspace. To record whether we already performed the migration, we can add an internal preference to jdt.junit and set it to "true" when we run the migration code. If the preference is already "true", then we don't touch the favorites any more.

Please re-check if we really don't lose functionality with the changes in JUnitQuickFixProcessor. I think we should keep the Quick Fixes for setups where the Content Assist favorites are not set and hence JDT UI doesn't offer the generic Quick Fixes based on the favorites.

Comment 4 Noopur Gupta 2017-03-01 09:56:13 EST

Created attachment 267055 [details]
Patch

Attaching the patch implemented based on comment #3. 

(In reply to Markus Keller from comment #3)
> we can add an internal preference to jdt.junit

The internal preference is added in JUnitUIPreferencesConstants. But I am storing/reading it from org.eclipse.jdt.junit.core.prefs instead of org.eclipse.jdt.ui.prefs. 

This internal preference is first saved from the JUnitPlugin.start(BundleContext) method. Then the migration code in JunitPreferenceInitializer is executed. If the preference is saved in org.eclipse.jdt.ui.prefs then this migration code moves it to org.eclipse.jdt.junit.core.prefs for the first time and hence reading it from org.eclipse.jdt.ui.prefs fails. See bug 337209 also.

Hence, I am using org.eclipse.jdt.junit.core.prefs for this preference. Please let me know if this could be handled differently.


> Please re-check if we really don't lose functionality 

The quick fixes for an undefined method provided by JUnitQuickFixProcessor are: 
- Add static import 'org.junit.Assert.*'
- Add static import 'org.junit.Assert.fail'

The quick fix for an undefined method provided by Content Assist favorites is: 
- Add static import for 'Assert.fail'

Hence, we lose one quick fix for 'Assert.*' and the qualified type name in the quick fix ('org.junit.Assert.fail' vs 'Assert.fail)'. 


> we should keep the Quick Fixes for setups where the 
> Content Assist favorites are not set

Added a check in JUnitQuickFixProcessor to add the quick fixes if the current value of the favorites preference does not contain org.junit.Assert.*.

Markus, please have a look at the patch and above comments.

Comment 5 Noopur Gupta 2017-03-02 05:19:43 EST

(In reply to Noopur Gupta from comment #4)
> Hence, I am using org.eclipse.jdt.junit.core.prefs for this preference.
> Please let me know if this could be handled differently.

We could also remove the old migration code now which was added in bug 278844.

Comment 6 Markus Keller 2017-03-16 07:33:27 EDT

(In reply to Noopur Gupta from comment #5)
> We could also remove the old migration code now which was added in bug 278844.

Yes, please remove that migration code. None of those preferences are so important that it's worth the hassle trying to fix that migration code.

The new preference should be stored in the UI plug-in (JUnitPlugin).

Please change all the HashSets in JUnitPlugin#setCodeassistFavoriteStaticMembers() to LinkedHashSets. That keeps the order of the entries consistent and avoids useless little changes in the file that stores this preference.

I would also add a short explanation that tells what we intend to do, and why. E.g.: Add the new default static import favorites in old workspaces that already have non-default favorites. Only do this once, so that users have a way to op-out if they don't want the new favorites.

Writing such a comment takes a minute now, but can save hours of investigation later.

Comment 7 Noopur Gupta 2017-03-16 09:06:10 EDT

Thanks, Markus. Released the updated patch:

http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?h=BETA_JUNIT5&id=b0675f3a1449aa34e74ef66f86af7a061f5182cf