Saturday, January 28, 2006

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

0 Comments:

Post a Comment

<< Home