Finishing, making the patch, and waiting...

This contribution was different from my last as far as me keeping in sync with the latest Eclipse builds. I spent a lot of time last time loading new versions of Eclipse and keeping the changes in sync. It was a royal pain in the butt, so this time I got one version and did all my changes without re-syncing.

This time, I got all the changes done and tested on one revision of Eclipse. Then, I downloaded the latest integration build and compared my files to the latest. There were virtually no changes to the files than mine. Eclipse had been going through a stabilization period for a milestone release (3.2 M5). I also compared to the head revision using the functions in the Eclipse CVS Repository perspective. I created a patch of all the files by control selecting all the packages I had altered in the package explorer and selecting the "Create Patch..." function from the CVS menu. (Note the package explorer doesn't come out-of-the-box in the CVS perspective, so I always add it for convenience.) I don't think I realized you could do patches for all packages by multi-selecting them last time I did this because I sent in two files.

Here are my comments for the patch:

- - - - - - - - - - -

Markus - Here is a patch that implements the fix from comment 4 as discussed.

Notes:

There's a new class "AssertionVMArg" that contains all the functionality as
static methods. Changes to existing classes are (for the most part) one line
calls to this class. This allowed me to keep the functionality encapsulated to
one place, and made changes to existing files less repetitive and messy.

You should check the changes to the UI on the JUnitPreferencePage. I took my
best guess as to the way to put the new checkbox on the page and adjust the
wording. I'll have no hard feelings if you want to change it.

Added a new test class with tests for default preference setting, the UI
hooking up correctly with the preference, and the jdt junit class creating a
config correctly. Automated tests are missing for the pde classes I had to
change, but I manually verified junit plug-in test configs work as well as
junit test configs. It seems I would have to put tests in the pde plug-in to
get to these classes, as it wouldn't let me import the pde classes into the jdt
test plug-in. I am willing to do this if you want, but I need to gather my
energies and make another run at it.

Let me know if you want any changes to the changes.
- John

- - - - - - - - -

Marcus took a bit to return this e-mail, but seemed positive.

- - - - - - - - - -

John: Thanks for the patch. I'll release it just after the milestone (sorry, I
was rather busy last week, and now we're already in the lockdown).

- - - - - - - - - -

That was from February 13th, and there's been no update since then (today is March 5). I'm concerned my patch is getting stale in all this time, but am assuming Markus will ask me for a fresh re-update if he has trouble with it.

EclipseCon is coming up. As I said long ago in an earlier post, my talk on doing things like this (making Eclipse contributions, I mean) was accepted, and I got slides done by their Feb 17 deadline. In the course of submitting the talk to the Eclipse folks (you do it as a bug report in Eclipsezilla - mine was submission 77 - http://eclipsezilla.eclipsecon.org/show_bug.cgi?id=77 ) I met another guy, Philippe Ombredanne, who had submitted basically the same talk. We decided to join forces and are giving the talk together. The talk is on Thursday afternoon, March 23rd. I hope this patch goes in before then so I can show the functionality in an official version of Eclipse.

Next, I dunno, hopefully a happy ending about how the patch went in...

I unsuccessfully try to expand the scope again...

When I was running my tests, I realized something about my new solution that I didn't like. It is this: a user will go to the preferences window and check a checkbox that says "enable assertions for JUnit tests." Then the user will rerun a JUnit test that already has a launch configuration setup, and assertions will not be enabled. That's because the code only enables assertions on new launch configurations as they are created, not on existing launch configurations.

So, I started looking into what it would take to enable or disable unit tests for all existing launch configurations when the checkbox is OK'ed. This turns out to be a fair pain-in-the-butt. You have to read all the launch configurations, filter the ones that are JUnit or JUnit plug-in tests (OK, that part's not so hard) but then you have to dig through all their JVM arguments and match the ones that enable assertions to see if they are already enabled or not. Since I don't know very well the constraints and variations among VMs on entering these args, I'm not confident I can detect them and frame their beginnings and endings in all cases. So I start implementing it, hoping I can work through the issue. But alsoI sense I am expanding the scope of my solution in a possibly controversial way, so I send off an e-mail to Markus explaining my dilemma.

- - - - - - - - - -

Markus - I have a preliminary solution done that adds a checkbox to the
JUnitPreferencesPage, uses that to set a preference, and then uses the
preference to default new JUnitLaunchConfiguration objects to have their VM
args defaulted to blank or to the enable assertions argument. It all works, and
I have an automated test for the page and the preference, but not yet the
launch configurations.

I ran into something, however, that seems like a bug in this solution. When you
set the preference, it only signals the system to add the VM arg to
subsequently created launch configurations. All the old launch configurations
are still sitting without the VM arg. So it seems to the user they just enabled
assertions, but when they rerun a unit test, assertions still aren't enabled
(and vice-versa with disabling).

So, I figured I should go through all the JUnitLaunchConfiguration objects, and
re-sync their arguments to however the user set this preference. I'm working my
way through the code of this, and almost have something to try.

Thought I should bring this up to you for comment before I spring the whole
thing on you at once. Will have a complete patch soon.
- John

- - - - - - - - - -

As I might have guessed Markus wants none of this idea, and writes back with a simplifying change instead.

- - - - - - - - - -

We should not fiddle around with the user's exisiting launch configurations.
The user may have set up some launch configurations with -ea and some without.
If the checkbox would change all existing configs, that would destroy the
user's configuration and would not allow him to enable -ea for new tests
without destroying his work.

You should rather call the checkbox 'Enable assertions for new JUnit launch
configurations' to avoid any confusion.

- - - - - - - - - -

As usual, I cave in immediately and get on with the fix. I changed the text, and commented out my half-implemented configuration-surfing-parsing-and-changing code. (You never know when you may need it later...)

- - - - - - - - - -

OK - I can see that - it's definitely the safer way to go.

I was almost done with the code to do it too - the only part I was having a
hard time with was how to reliably take an argument out of the VM args - I
imagine that's even worse from your perspective.

So, the only part I have left to do to finish is to figure out how to call the
right things programatically to create new launch configurations through
JUnitLaunchShortcut, JUnitTabGroup, and AbstractPDELaunchConfigurationTabGroup,
and then I can finish the automated tests.

- - - - - - - - - -

Next, the patch and the wait....

Making JUnit tests

The first rule if you're making or running JUnit tests for Eclipse code is you have to download the source for the org.eclipse.test.performance package. This magically enables JUnit plug-in tests to work, and without it, you get a bunch of inexplicable errors that you will waste your time trying to understand and fix.

Since this was the first time I ever made up a new JUnit plug-in test, I looked at other examples in the org.eclipse.jdt.ui.tests package. I had added tests to the NewTypeWizardTest before, I poked around in there for ideas, and it helped to review familiar territory.

The main issues with testing Eclipse code are having the right interfaces to test what you need, and getting the classes you want to test into the right state. Since these classes normally execute inside complex Eclipse runtime contexts, you have to do some experiments with setting up scaffolding and calling initialization methods. When you need test projects to work with, you can create them inside the setUp() method of the JUnit test. Since Markus told me he didn't want me actually running a JUnit test and checking to see if it actually asserted, but only checking if the launch configuration was initialized correctly, I didn't have to bother with setting up a test project to run. I could test each unit on a lower level.

The units of functionality I had to test were:
That the preference for enabling assertions was initially false
That enabling and disabling the checkbox on the preferences page properly set and cleared the enable assertions preference
That the preference caused new launch configurations to be initialized properly (JUnitTabGroup) with the assertion VM arg or not
The automated tests for default configuration creation (JUnitWorkbenchShortcut) and plug-in test initialization (AbstractPDELaunchConfigurationTabGroup) I left out because they looked like more trouble, so I hope I don't burn in hell too long for that.

The first test was a no-brainer, just put an assertion on the utility function for reading the preference to make sure it's off.

The second test for the preferences page had two issues. First, since you don't test by means of a UI robot, but test by means of calling utility methods on the page, I had to create a utility method to set and clear the check box. Second, you have to create a page and initialize it to test it. In the case of a JUnitPreferencesPage, I had to call the createControl() method to initialize the SWT widgets so I could set and clear the checkbox. The createControl() method needs an SWT UI context as the parent of the new page. Luckily, the JUnit plug-in scaffolding provides a DialogCheck class that has a getShell() method you can use as a parent context. After creating a new page and initializing it this way, all I had to do was call my checkbox enable/disable method, call the page's performOk() method, and assert that the preference had been set correctly.

The third test for the launch configuration initialization took more experimenting. First, you can’t change a launch configuration, so you have to use a launch configuration working copy. And you can’t instantiate one; you have to go through a 4-line song-and-dance that I put into a utility method. Then, you have to create a tab group and initialize it using createTabs(). Then you can call the setDefaults() method on the tab group, and then test to see if the launch configuration has the assertion enabling argument on or not as expected.

Finding the solution for this problem took a lot of setting breakpoints and tracing through the code to find the right bottlenecks. The key area, as I had discovered in my surfing last time, was the launch configuration, which was initialized with default VM arguments when it was created.

Since there are several similar operations users can go through that cause Eclipse to create new launch configurations, I had to check each one to see how the launch configurations were defaulted. New launch configurations can be created manually, when the user selects the "Run.." or "Debug.." menu items under "Run as" or "Debug as" in the right-click menu when launching. New launch configurations are also automatically created by Eclipse when the user selects "JUnit test" or "JUnit Plug-in test" from the "Run as" or "Debug as" menu if there is no previously created launch configuration for the selected item.

My first inclination was to add a constructor to the JUnitLaunchConfiguration class to set the defaults there, but the class didn't have an explicitly declared default constructor, and I didn't want to complicate the semantics of new object creation with searching properties and dealing with exceptions when something goes wrong. It seemed like I should look around for a better place.

Ultimately, the six use cases outlined above resolve to 3 methods in 3 different classes.
JUnitWorkbenchShortcut.createConfiguration() handles the cases where the user invokes the test directly and there is no default launch configuration.
JUnitTabGroup.setDefaults() handles manually created launch configurations for base JUnit tests.
AbstractPDELaunchConfigurationTabGroup.setDefaults() handles manually created launch configurations for JUnit plug-in tests.

Finding these spots was where I had to do my breakpoint setting and tracing. Launching a JUnit test involves sending an event to an event handler, so you have two potential threads to trace through, and once you find the right one, the setDefaults() protocols have fairly deep call stacks. At first I thought I could set the configuration in the common base of these two classes (AbstractLaunchConfigurationTabGroup) but unfortunately, they set defaults differently, and the PDE version monkeys with the VM args that somehow may be there already. So I had to create separate but similar implementations for the two classes.
The JUnitWorkbenchShortcut.createConfiguration() case was clearer to nail down, and didn't take so much headscratching. It turns out that case was the same as far as what I had to do to default the VM argument as the JUnitTabGroup.setDefaults().

So, the issue was I had 3 similar cases that shared some code - 2 of the 3 were exactly the same, and I needed to invoke them from 3 different spots in Eclipse code. So I made up a utility class that contained all the code to set and get the preference, and to initialize the launch config value, and exposed the functionality as static methods. Then I added invocations to these methods as one-line changes in the 3 different spots. I tested them with constants instead of real preferences and the launch configurations were as I expected and the JUnit tests ignored or failed assertions as I expected.

The next thing to do was add a checkbox to the Window Preferences JUnit page. I had never done this kind of thing before, so I looked at a lot of examples. I had to make changes to the JUnitPreferencePage class to create the checkbox and add a new composite so this checkbox could share the window with the stack trace filter pattern editing stuff that was already there. Then adding the preference turned out to be quite easy. Manual tests were working, so I was pretty close.

All in all, the surfing and tracing took the most time. I did the back-end parts with the new AssertionVMArg class and inserting its changes in two evenings home from work, and I did the preferences page in a weekend afternoon.

Next - writing JUnit tests.

Email exchanges from the bug...

In these comments, Markus says he doesn't really like the design behind the bug, but is willing to use it as a fallback if they can't get the solution they want done in time. I believe his point is that the problem is setting VM arguments properly in launch configurations will solve the assertion enabling problem along with many others. If you go read the other related bugs, you see a lot of people complaining about different aspects of VM argument setting.

------- Comment #7 From Markus Keller 2006-01-09 05:13 [reply] -------

This is another case that should be solved by a general launch templates mechanism rather than a JUnit-specific solution (see bug 41353).

If the Debug team decides they won't fix this for 3.2, then we would have to go for the solution from comment 4.

------- Comment #8 From Markus Keller 2006-01-09 11:59 [reply] -------

(In reply to comment #5)
> Is anyone on fixing this one?
> It bothers me enough I'm willing to work on a fix.

John, if you could provide a patch for the solution from comment 4, that would be nice.

I don't think we should add a checkbox on the individual launch configuration pages to enable/disable the -ea feature. It would clutter up the UI and it's hard to motivate why there should be special UI for the -ea switch but not for all the other vm arguments.

- - - - - - - - - - - - - -

My point is that many users won't necessarily understand that enabling assertions is related to VM arguments, so they'll want a feature that is more direct and higher level. I may be over-reaching at this point, but I make one more pitch for having a higher-level feature, like a checkbox on the launch configuration box.

------- Comment #9 From John Kaplan 2006-01-10 23:41 [reply] -------

OK - I will start looking into it.
I also like the solution from comment 4, but I do have an issue with only having the option in workspace preferences and not in the launch configuration pages.

The issue is that the workspace preferences page takes a fair amount of navigation to get to, and is far away from what the user sees when they start a run.

So for usability, it's likely the option will be hidden and a lot of users won't see it.

I think the fact that enabling assertions is a VM argument is an implementation detail. I didn't look at VM arguments as I was trying to figure out how to enable assertions, and from the chatter about this on the web (if you google "enable assertions eclipse" you'll see it) shows there are plenty of other users that also think this is counter-intuitive.

So, I would prefer to have a check box for defaulting the behavior per project on the preferences panel, but then have another check box on the run.../debug... dialog panels to override it per launch configuration, and to make it more manifest to the users for better usability.

But, as I always end up saying in these discussions, it ain't my code base, so I'll go with the committers decision.

- John

- - - - - - - - - - - -

At this point I do my surfing and analysis of the code that's there. Looks like it will be a fairly straightforward fix, and I'm not convinced I was right about the check boxes on the launch configurations anymore, so I'm willing to let that part go and just do the check box on the JUnit window preferences dialog.

------- Comment #10 From John Kaplan 2006-01-21 22:57 [reply] -------

Been looking at this trying to figure out a solution. I have one for the basic use case we agreed to.

I think the best design would be to initialize new launch configurations related to junit tests to be created with a default VM arg that enables assertions if the user has set the preference in the Window > Preferences... dialog. This way the VM arg is where users can see it if they look and there's no invisible creation of VM args in the background of a launch.

To implement this solution, I found 2 possible paths to create launch configurations for junit tests. One is in JUnitLaunchShortcut.createConfiguration() that is called when the user directly launches a new test. The other is in JUnitTabGroup.setDefaults() that is called when the user starts with the "Run..." or "Debug..." dialogs to create new configurations before they run something for the first time.

The JUnitLaunchShortcut.createConfiguration() change would be easy, but the JUnitTabGroup.setDefaults() looks like it will need one other workaround because the JUnit plug-in test doesn't jump through the same implementation as the JUnitTabGroup. It has a different implementation of setDefaults in AbstractPDELaunchConfigurationTabGroup, and it already messes with the VM args, so the implementation here will have to be more intelligent to avoid double-entering the VM arg.

So that's the implementation I'd go with if you guys agree. Anyone know of other issues I haven't covered? Thoughts? Yeas or Nays?

- - - - - - - - - -

In his response, Markus comes through with some good advice. I didn't know about the DebugPlugin.parseArguments method, and it came in handy.

------- Comment #11 From Markus Keller 2006-01-23 12:07 [reply] -------

John, that sounds great. I guess in the PDE JUnitTabGroup, you have to parse the existing IJavaLaunchConfigurationConstants.ATTR_VM_ARGUMENTS. That's probably done easiest and safest by calling DebugPlugin.parseArguments(..) and then searching for arguments that start with "-enableassertions" or "-ea".

------- Comment #12 From John Kaplan 2006-01-24 00:18 [reply] -------

Thanks for the tip Markus.

One more question - Since it looks like I'll be making changes in 3 packages for this, I wanted to check on which test suites to run and add new tests to.

I've already worked with jdt.ui.tests, so I know the drill there. Are tests for the junit preferences tab in the UI there, or in another package? I don't see a jdt.junit.tests, but there are a few test packages in jdt.debug.

To get the junit functionality completely tested, I'd have to set the preference to both enable and disable assertions, then run a test and either check the VM environment somehow or set up a test that throws an assertion and check that it did/didn't assert as expected.

Can you point me to any similar kind of test junit test I could use as a model, and let me know where the best place to put new tests would be? (If the test junit plug-in tests are in a different place, let me know that too since I have to make the unique changes we discussed above.)

Thanks,
- John

------- Comment #13 From Markus Keller 2006-01-24 05:16 [reply] -------

We don't have many tests for the junit plugins. The existing tests are indeed in org.eclipse.jdt.junit.tests in the jdt.ui.tests plugin.

If you want to write a test for this bug, thats great, but it's also OK without a test. I would not try to run a generated JUnit launch configuration from within the test. Just check that when the preference is enabled, the new lauch config has the -ea set.

- - - - - - - - - -

Far be it for me to judge - but did I just hear the committer for JUnit support in eclipse tell me I don't have to write tests? I am too much of a TDD programmer to let this slide, will have to rib him about this at some point.

------- Comment #14 From John Kaplan 2006-01-24 14:51 [reply] -------

OK - good enough.
I'll move ahead on this info and see how far I get.
Thanks,
- John

OK - Much time has passed, and many things have happened I need to catch up on. Have asked some questions on the bug mail and gotten good guidance from Markus. Also, my proposal for giving a talk at EclipseCon about doing contributions like this has been accepted. I'll catch up on those things in later entries.

For this one, it's the nuts and bolts of the fix. I have done a lot of surfing the eclipse code to find what I'm going to change and how. I downloaded a new integ of eclipse to work with, and got the source straight out of the PDE to do the source surfing. Grabbed a bunch of packages just to make sure I had it covered.

To avoid more of my bad experiences on my last contribution, I want to keep track of which packages I need and which files I intend to change, and get no more than that once I start messing with CVS. Note you can work with the PDE to get source to mess with, but the test packages are missing, and you need to get them directly from CVS if you're going to alter tests. So, you might as well get everything from CVS so you can keep it coordinated.

I won't go through the boring details of all the surfing and tracing I did to find the spots in the code I will need to change. Suffice it to say you have to do a lot of this when making eclipse changes. It is (as we say in Colorado) a big honkin' bunch of code, and it is factored into thousands of tiny specialized method calls. The "Search" menu and pop-up menu "Open Declaration" commands are your best friends.

For UI classes, do a text file search for text on the display. That may lead you straight to a utility class with the text as a comment, or to a preferences file with the text. From there, you can search for the preferences file constant ID or for a class that references the utility class.

For internal classes, you need to do more digging. Since assertions will be enabled through a VM arg, I searched for "vm arg" in a file text search, and got a haystack full of hits. I knew this would also have something to do with a launch configuration, so that narrowed it down quite a bit.

Once you've got a good suspect target class, set a breakpoint in the spot you think is the most likely, and fire up eclipse in a debugger to step through the code and watch it do its thing. That's the way I narrowed down what classes were involved in the changes I'll have to make. Here's the list I eventually got to:

Packages/files needed:
org.eclipse.jdt.junit
JUnitTabGroup
JUnitLaunchShortcut
JUnitPreferencePage

org.eclipse.pde.ui
AbstractPDELaunchConfigurationTabGroup
JUnitWorkbenchShortcut

(For testing)
org.eclipse.jdt.ui.tests
org.eclipse.test.performance

OK - so I grit my teeth and do a search for a bug about enabling assertions in the Eclipse bugzilla. I grit my teeth because I've done this before and it is often an exercise in frustration. The bug database is so big it hides everything like a haystack full of needles, and everybody uses obscure tribal languages in the descriptions that you hope are related to the thing you're searching for. Invariably, they're not, at least they're probably not but you're not quite sure. But then maybe that one from two searches back was really the one you wanted after all... If you try to get back to that one you can't find it because you forgot the exact keyword combination you used before and you see a bunch that look similar but not quite the same and none of them are really related to what you want so you give up.

Well, by some miracle of violated expectations, I found the exact bug I was going to enter right near the top of the query results. It had been sitting for a couple of years and no one had fixed it. It was bug #45408, titled "Enable assertions during unit tests [JUnit]", opened by Kevin Klinemeier. Here is the stack of discussion from the bug, with my offer to fix it second from the end:

Description: [reply] Opened: 2003-10-22 14:49

As I understand it, Java 1.4 assertions are disabled by default unless the vm
argument "-ea" exists. I would like very much for Eclipse to have an option to
always enable assertions while running my unit tests from the "run as... JUnit
test" menu item.

I realize that I can define a runtime configuration and specify the parameters
there, but if this were the "default", that would be very helpful.


------- Comment #1 From Dan Allen 2005-06-03 00:53 [reply] -------

Additionally, I would like to see a switch somewhere in the run dialog titled
"Enable Assertions". Having to type the -ea switch in the JVM arguments is far
too low-level for eclipse and not very user friendly. I would almost even argue
adding a "Run Java Application with Assertions" in the Run menu.


------- Comment #2 From Edward Willink 2005-06-23 13:56 [reply] -------

Surely there should be a Compiler preference?


------- Comment #3 From Andrew Scherpbier 2005-06-23 14:28 [reply] -------

(In reply to comment #2)
> Surely there should be a Compiler preference?

This is not a compiler issue so much since java 1.4 assertions are always
compiled into the class files. The issue is the (default) enabling of
assertions at runtime.


------- Comment #4 From Edward Willink 2005-10-07 09:46 [reply] -------

Indeed, not a Compiler Preference.

There should be a Workspace->Preferences->JUnit config to set -ea as a default
for all auto-generated launch configurations, and possibly for manual ones too.


------- Comment #5 From John Kaplan 2005-12-14 10:58 [reply] -------

Is anyone on fixing this one?
It bothers me enough I'm willing to work on a fix.


------- Comment #6 From Edward Willink 2005-12-14 12:05 [reply] -------

There is a slightly clumsy workaround which works fine once you've learnt it.

If you put -ea as a VM argument in a Test All launch configuration, it
works, and is inherited when you select some test(s) to redo from the
subsequent
JUnit window.

It's only when you do a Run As->JUnit Test on an IResource selection that you
lose the -ea, and get deceived into thinking everything's good till the next
Test All.

- - - - - - - - - -

This bug was assigned to Erich Gamma. You gotta figure the dude's pretty busy, so I didn't think flaming him to get on it and get his bugs fixed would be appropriate. Instead, a diplomatic e-mail might do the trick, so I wrote the following:

Fixing eclipse bugzilla #45408 (enable assertions)

Hello Erich - I am interested in submitting a fix for this bug #45408 (Enable assertions during unit tests [JUnit]).
I see it is assigned to you and didn't want to step on toes if you were actively working on it, or planning to do so in the near future.

If it's OK with you, I'd start by making sure there was a consensus for the user interaction.

My first proposal would be to have a new panel in the Window > Preferences... > Java > JUnit section that had a checkbox for "enable assertions by default for all JUnit test configurations." Then have a checkbox in each of the Run... and Debug... JUnit configuration dialog panels that says "enable assertions." That's the basics, but also it seems this check box should remain in 2-way sync with the -ea VM argument.

I'd go with the consensus if there are other opinions as to how to do it, but I'd like to start a discussion on the bug list.

Once there was a consensus I'd like to develop and submit a fix.

Hope to hear from you soon,

Thanks,
- John

- - - - - - - - - -

Erich replied:

Hi John,

Markus Keller is now taking care of the JUnit integration. I've reassigned
the bug to Markus and I agree that the discussion should continue in
bugzilla

--erich

- - - - - - - - - -

So onwards we go...