Comment 1 Markus Keller 2011-03-04 08:35:35 EST

Fix is to replace code like this:

	public static Test suite() {
		if (true) {
			return allTests();
		} else {
			TestSuite suite= new TestSuite();
			suite.addTest(new MarkerResolutionTest("testQuickFixAfterModification"));
			return suite;
		}
	}

with this:

	public static Test suite() {
		return allTests();
	}
	
	public static Test setUpTest(Test test) {
		return new ProjectTestSetup(test);
	}

Comment 2 Deepak Azad 2011-03-04 08:41:30 EST

(In reply to comment #1)
> Fix is to replace code like this:
> 
>     public static Test suite() {
>         if (true) {
>             return allTests();
>         } else {
>             TestSuite suite= new TestSuite();
>             suite.addTest(new
> MarkerResolutionTest("testQuickFixAfterModification"));
>             return suite;
>         }
>     }
Just curious, why was such a code written in the first place ?

Comment 3 Markus Keller 2011-03-04 08:57:25 EST

It was written at a time when setUpTest(Test) was not available. The goal was an easy way to run a single test.

Deepak: If you have time, you can fix this in HEAD. You can also remove the allTests() methods. Their whole reason for being there was to avoid that the parent suites become incomplete (allTests() always returns all tests, but suite() only does that if the flag is true).

The super-clean solution is this:

    public static Test suite() {
        return setUpTest(new TestSuite(THIS));
    }

    public static Test setUpTest(Test test) {
        return new ProjectTestSetup(test);
    }

Comment 4 Deepak Azad 2011-03-04 15:01:43 EST

Created attachment 190440 [details]
fix

- Removed dead code warning in suite() methods. (Note that there are still some dead code warnings because of disabled tests)
- Removed allTests() methods.

Comment 5 Deepak Azad 2011-03-04 15:27:10 EST

Committed to HEAD.

Markus, I noticed that ASTProviderTest is not part of any Test suite. This is a mistake, right ?

Comment 6 Markus Keller 2011-03-05 19:00:48 EST

> Markus, I noticed that ASTProviderTest is not part of any Test suite. This is a
> mistake, right ?

Yes. But unfortunately, the test fails when I run it. I filed bug 339022.

Comment 7 Dani Megert 2011-03-15 10:22:29 EDT

Deepak, maybe you got tricked by Markus's hint on how to fix some of the warnings. However, there are still several dead code warnings left.

NOTE: You only need to fix those with the following pattern:

if (true)...
or
if (false)...

The ones which use a constant should be fixed by fixing bug 256796.

Comment 8 Deepak Azad 2011-03-15 12:16:16 EDT

So, you want to replace the true/false with a constant initialized to true/false - the dead code will warning still remain but will go away when bug 256796 is fixed ?

Comment 9 Dani Megert 2011-03-15 12:17:22 EDT

(In reply to comment #8)
> So, you want to replace the true/false with a constant initialized to
> true/false - the dead code will warning still remain but will go away when bug
> 256796 is fixed ?
True that is.

Comment 10 Deepak Azad 2011-04-06 01:48:58 EDT

Created attachment 192613 [details]
additional fix

Comment 11 Deepak Azad 2011-04-06 01:49:20 EDT

Fixed in HEAD.