groovy

I don't think it is related to groovy.lang.Closure#throwRuntimeException(Throwable) simply rethrowing unchecked exceptions. Even in 1.6.3, it does the same thing - https://svn.codehaus.org/groovy/tags/GROOVY_1_6_3/src/main/groovy/lang/Closure.java

I think it is possibly related to - http://fisheye.codehaus.org/browse/groovy/trunk/groovy/groovy-core/src/main/org/codehaus/groovy/reflection/CachedMethod.java?r1=12759&r2=17769

I am pretty sure that above mentioned change in CachedMethod exception handling is causing it.

The above change was done on 1.7-beta-2. Upto 1.7-beta-1, the reported issue is not there. It's also not there on current 1.6.9 because this change was not done on 1.6.x line.

Should we try to revert a part of changes from the above revision to fix the regression?

The changes were done under "faster exception handling" thread - http://marc.info/?l=groovy-dev&m=125412236814260&w=2.

I suspect so.

I'll readily admit I personally am relatively new to Groovy and this could indeed be a semantic issue requiring the partial reversion of the changes you cite.

But the simplest solution to me seems to be to update shouldFailWithCause to reflect this change by making it closer to the shouldFail code and just evaluate cases based upon the cause instead of the throwable itself) somewhat like the following:

protected String shouldFailWithCause(Class clazz, Closure code) {
        Throwable th = null;
        try {
            code.call();
        } catch (GroovyRuntimeException gre) {
            th = ScriptBytecodeAdapter.unwrap(gre);
        } catch (Throwable e) {
            th = e;
        }
        
        if (th==null) {
            fail("Closure " + code + " should have failed");
        } else {
            th = th.getCause();
            if (th==null) {
                fail("Closure " + code + " should have failed with an exception cause of type " + clazz.getName());
            } else if (! clazz.isInstance(th)) {
                fail("Closure " + code + " should have failed with an exception cause of type " + clazz.getName() + ", instead got Exception " + th);
            }
        }
        return th.getMessage();
    }

I looked into the patch and it fails to handle MissingProperyException / MissingMethodException, as shown in the code below

class PatchTest extends GroovyTestCase {
    public void testMissingProperty() {
        def foo = new Foo()
        shouldFailWithCause(MissingPropertyException) {
            foo.bar
        }
    }
    
    public void testMissingMethod() {
        def foo = new Foo()
        shouldFailWithCause(MissingMethodException) {
            foo.roshan()
        }
    }
}

class Foo {}

Re: the Missing*Exceptions...

I thought those cases are supposed covered under shouldFail(Class,Closure) instead of shouldFailWithCause(Class,Closure) since those are the exceptions that are directly thrown:

E.g. new Foo().roshan() throws an exception without a cause:

@Test
    public void testMissingMethodException() {
        try {
            new Foo().roshan()
            throw new Error("no failure")
        } catch(MissingMethodException e) {
            assertNull(e.getCause())
        }
    }

There are unit test cases in groovy code base that explicitly test MissingPropertyException with shouldFailWithCause. I think the current implementation of shouldFailWithCause() has no problem if getCause() is null.

And since it has not been having any problem handling exception cases with cause = null, making it so will cause another regression.

I guess the root of this issue then is what is 'shouldFailWithCause' supposed to assert about the passed Class and Closure and how is it different from shouldFail(Class,Closure)?

Unfortunately we're running into having no javadoc on the method, the only other bug am currently finding that mentions shouldFailWithCause is GROOVY-1247 (which seems to be related to return values), and fisheye unfortunately isn't helping find when it was added given that HEAD of GroovyTestCase currently indexed as r6778 where the method does not exist yet...

Based upon the naming convention, I interpreted shouldFailWithCause(Class,Closure) to mean that I expect a particular closure to throw any Throwable that has a cause of the specified class, thus distinguishing it from shouldFail(Class,Closure) which seems to asserts that the closure throws a Throwable of the specified class.

The issue I have with shouldFailWithCause also handling the case where getCause() == null is that then we have a state where this one method sometimes makes an assertion about the thrown Throwable, and sometimes makes an assertion about the thrown Throwable's cause, making our tests unclear as to which is the case.

If my interpretation of purpose behind shouldFailWithCause is true, then the tests were written incorrectly in the first place, and we should deliberately break them. If I'm wrong, then we need to make sure to change shouldFailWithCause so that it only asserts one state since philosophically having a method that asserts multiple different states makes any test utilizing it ambiguous to begin with. (Since no matter what we'll wind up breaking someone with any change, we probably should slate it for groovy 1.8).

And of course this is a separate discussion from whether or not the change to let RuntimeExceptions fall through as is was indeed correct.

My Last argument in code... currently these two tests both succeed, and I assert that only one of them should since they are different states:

public static class HomerSimpson { 
    }
    
    @Test
    public void testMissingMethodManual() {
        Closure code = {
            throw new MissingMethodException("eatDonut", HomerSimpson, new Object[0] )
        }
        shouldFailWithCause(MissingMethodException.class, code)
    }
    
    @Test
    public void testMissingMethodSecond() {
        Closure code = {
            throw new Exception(new MissingMethodException("preventMeltdown", HomerSimpson, new Object[0] ))
        }
        shouldFailWithCause(MissingMethodException.class, code)
    }

Wait... I'm an idiot... shouldFailWithCause tests that the last cause is of the specified type... (is this correct?)

@Test
    public void testIOExceptionFirst() {
        Closure code = { throw new IOException() }
        shouldFailWithCause(IOException.class, code)
    }
    
    @Test
    public void testIOExceptionSecond() {
        Closure code = {throw new Exception(new IOException()) }
        shouldFailWithCause(IOException.class, code)
    }
    
    @Test
    public void testIOExceptionThird() {
        Closure code = { throw new Exception(new Exception(new IOException())) }
        shouldFailWithCause(IOException.class, code)
    }
    
    @Test
    public void testIOExceptionFourth() {
        Closure code = { throw new Exception(new Exception(new Exception(new IOException()))) }
        shouldFailWithCause(IOException.class, code)
    }

Considering my last comment how about this then:

protected String shouldFailWithCause(Class clazz, Closure code) {
        Throwable th = null;
        try {
            code.call();
        } catch (Throwable e) {
            th = e;
            while(th.getCause()!=null) {
                th = th.getCause();
            }
        }
        
        if (th==null) {
            fail("Closure " + code + " should have failed with an exception caused by type " + clazz.getName());
        } else if (! clazz.isInstance(th)) {
            fail("Closure " + code + " should have failed with an exception caused by type " + clazz.getName() + ", instead got Exception " + th);
        }
        return th.getMessage();
    }

The last patch looked ok to me and I have applied it. Thanks.

I am not sure the logic is correct here as patched. Pondering ...

The following test used to work but now fails with the latest patch:

class MyTest extends GroovyTestCase {
  def throwWrappedException() {
    throw new GroovyRuntimeException(
      new IllegalArgumentException(
        new NullPointerException()
      )
    )
  }
  void testBefore() {
    shouldFailWithCause(IllegalArgumentException) {
      throwWrappedException()
    }
  }
}

I think the original intention of this method was to strip away any layers of Groovy wrapping and proceed from there but changes in the way we do things make shouldFail have that characteristic now. I think the best semantics would now be to detect the cause anywhere in the hierarchy.

My suggested additional fix is attached (though needs javadoc!). All tests pass. But I think PropertyTest should just be shouldFail anyway (which also passes). I suspect PropertyTest was only the other way because of legacy problems with shouldFail which no longer exist.

One aspect that I wasn't sure which way to go was whether to check the top-level exception against the cause we are looking for. I ended up going that way because the first exception we see might be further wrapped as the cause for some GroovyRuntimeException and hence might be the cause without us knowing it.

Another option would be to revert back all changes to shouldFailWithCause() that we have made here and solve it by reverting part of the "faster exception handling" change, as earlier discussed here.

Or, we can stick to the new patch and define the behavior at this point with a proper javadoc so that the intended behavior does not need to be guessed in future and stick with it.

I am +1 for the second of these:

We can stick to the new patch and define the behavior at this point with a proper javadoc so that the intended behavior does not need to be guessed in future and stick with it.

I don't think the original version was offering any significant value given the other changes that have taken place.

Ok, let's do it - with a few lines of javadoc this time

Wasn't sure whether you wanted me to do it or not - so I went ahead and did it.

I was looking for some help there due to the javadoc involved. I thought you were in a better position to explain it there due to the last issue you found there and the changes that you made.

Thanks.

No worries at all. Now just to work out why tests pass locally and on Bamboo JDK1.6 but fail on Bamboo JDK1.5?

Paul, the JDK 1.5 builds are failing because in the tests you have used the following 2 constructors of IOException and the JDK 1.5 version of IOException does not have both of these.

IOException(Throwable)

IOException(String, Throwable)

The tests need to use some other exception that has such suitable constructors in both JDK 1.5 and 1.6.

Here is a suggestion for IOException replacement in tests : java.util.concurrent.ExecutionException - has both the needed constructors.

Yes, I ran the tests locally on 1.5 and spotted that too. Just fixing now ...

I ended up going with IllegalArgumentException. The other lesson learned from the CI logs was that we weren't seeing the full error message. It made sense to unwrap GroovyRuntimeExceptions and print out all of the exception messages in the error message. It also no longer made sense to compare against the top level exception as that is now the same as shouldFail. So I updated the logic and the javadoc.

Just chatting with Jochen. Apparently there is a use case that we have encountered in the past where an exception has itself as its own cause. Sounds bad style but we should handle this case gracefully in any case. I'll add in a check for this case too to avoid the possibility of an infinite loop.