Bug 321608

Comment 1 Markus Keller 2010-08-03 11:59:21 EDT

> 1) File -> New JUnit Test Case -> choose New JUnit 4 Test (say Test1) in the
> radio button
> => the super class field is uneditable 
That's bug 275574.


> 4) Open the Test1 class and add the superclass as junit.framework.TestCase
> and repeat step (3) it appears now in the list

That creates a mixed JUnit 3 and Unit 4 test, which is not recommended and can be hard to understand.


> 3) Open the New JUnit Test Suite wizard : File -> New JUnit Test Suite 
> => The new test Test1 case does not appear in the list of 'Test classes to
> include in suite' 

That's a design choice (flaw) of JUnit 4. A class is a JUnit 4 test as soon as it contains a JUnit-4-specific annotation (e.g. @Test). An empty JUnit 4 test case is just an empty class, and it's intended that non-test classes don't show up in the suite wizard.

Keeping this bug to improve that scenario. The New JUnit 4 Test Case wizard should create a first test method like this (unless it has a "Class under test" and already generates test methods):

package tests;

import static org.junit.Assert.*;

import org.junit.Test;

public class Test1 {
	@Test
	public void test1() throws Exception {
		fail();
	}
}

Comment 2 Raksha Vasisht 2010-11-16 03:13:56 EST

Created attachment 183194 [details]
Patch

Adds the first test method to JUnit4 test case when class under test is null thereby appears in the list of classes while creating a test suite.

Comment 3 Markus Keller 2010-12-05 11:45:58 EST

The generated test method should look like a generated test method for a class under test. The implementation should also be the same, so please extract the relevant parts of NewTestCaseWizardPageOne#createTestMethodStubs(..) so that you can reuse the implementation.

BTW: You can also fix #createSetupStubs(..) so that it also uses #appendTestMethodBody(..) all the time, but then it would still be wrong to call a method called create*Setup*Stubs to create a test method stub. The condition with the special method name TEST1 is a no-go. It looks like a hack, and it can even cause wrong behavior if you use the wizard to create a test class whose super class is this:

public class Aaa {
	protected int test1() throws Exception {
		return 42;
	}
}

Comment 4 Raksha Vasisht 2011-01-18 03:39:05 EST

Created attachment 186977 [details]
Patch_2

(In reply to comment #3)
> The generated test method should look like a generated test method for a class
> under test. The implementation should also be the same, so please extract the
> relevant parts of NewTestCaseWizardPageOne#createTestMethodStubs(..) so that
> you can reuse the implementation.
> 
> BTW: You can also fix #createSetupStubs(..) so that it also uses
> #appendTestMethodBody(..) all the time, but then it would still be wrong to
> call a method called create*Setup*Stubs to create a test method stub. The
> condition with the special method name TEST1 is a no-go. It looks like a hack,
> and it can even cause wrong behavior if you use the wizard to create a test
> class whose super class is this:
> 
> public class Aaa {
>     protected int test1() throws Exception {
>         return 42;
>     }
> }

Removed new implementation from createSetupStubs(..) and put it in  createTestMethod(...) and extracted more checking code from NewTestCaseWizardPageOne#createTestMethodStubs(..) which ensures the names from Super types or Class under test do not clash and are differentiated with appropriate prefixes or numbers.

Comment 5 Markus Keller 2011-01-24 12:52:27 EST

* createTestMethod():
- The if-else needs a block around the else statement, and the rest of the method body is wrongly indented.
- The Javadoc should say "Creates a test method.", since multiple test methods can be created.
- parameter "elementName" is unnecessary

* createTestMethodStubs():
- I see that you try to avoid name conflicts, but the analysis is incomplete (only considers one level of super types), and it wrongly checks the class under test (which is not used at all in this case, so it cannot produce a conflict). It's also strange to consider the superclass of the test only here, but not in the case with a class under test. Please remove that analysis and just leave:

	List<String> names= new ArrayList<String>();
	createTestMethod(type, imports, null, null, names);

In the ClassUnderTest case, conflicts would be tedious to fix manually, but here, we produce at most one problem.

BTW: "methds" is a non-speaking abbreviation that looks like a typo. If you don't use a speaking name, call it "method2" (so that it's clear that the reader has to check what it is).

Comment 6 Markus Keller 2011-01-24 12:54:10 EST

OK to release for M5 with the suggested changes.

Comment 7 Raksha Vasisht 2011-01-24 14:10:13 EST

(In reply to comment #5)

> * createTestMethodStubs():
> - I see that you try to avoid name conflicts, but the analysis is incomplete
> (only considers one level of super types),
True, it should ideally check for all inherited methods. 

> It's also strange to consider the superclass of the test only here,
> but not in the case with a class under test. Please remove that analysis and
> just leave:
> 
>     List<String> names= new ArrayList<String>();
>     createTestMethod(type, imports, null, null, names);
> 

I only considered the superclass of the test here since we create the additional test method only in this case and not when we have a class under test. 
> In the ClassUnderTest case, conflicts would be tedious to fix manually, but
> here, we produce at most one problem.

Agreed. In fact the problem is when the super class has a test method already with a different signature as mentioned in the example in comment #3 which is at most one problem. It does make sense to remove the complicated checks. Patch modified accordingly and committed to HEAD.
> 
> BTW: "methds" is a non-speaking abbreviation that looks like a typo. If you
> don't use a speaking name, call it "method2" (so that it's clear that the
> reader has to check what it is).
 Will keep that in mind. Did have a bad feeling while naming..

Comment 8 Raksha Vasisht 2011-01-24 14:11:10 EST

(In reply to comment #6)
> OK to release for M5 with the suggested changes.

Commmitted to HEAD.

Comment 9 Raksha Vasisht 2011-01-24 14:11:38 EST

.

Comment 10 Dani Megert 2011-01-25 06:58:33 EST

Verified in I20110124-1800 that scenario from comment 0 now works.

Comment 11 Rajesh 2011-01-25 13:52:03 EST

Verified for 3.7 M5 with I20110124-1800.