Groovy
  1. Groovy
  2. GROOVY-4075

shouldFailWithCause no longer works for unchecked exceptions

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.7.0
    • Fix Version/s: 1.7.2, 1.8-beta-1
    • Component/s: None
    • Labels:
      None

      Description

      So we upgraded from 1.6.3 to Groovy 1.7.0 and realized that one of our tests started failing... it seems that in 1.7.0 unchecked exceptions are simply rethrown (groovy.lang.Closure#throwRuntimeException(Throwable) called from #call(Object[])). Which in groovy.util.GroovyTestCase#shouldFailWithCause(Class,Closure), the exception falls to the catch Throwable branch and the cause is not attempted to be determined beyond that, which causes the test to fail.

      Example test showing a successful checked and unsuccessful unchecked exception:

      package dummy;
      
      import groovy.util.GroovyTestCase;
      
      import java.io.IOException;
      
      import org.junit.Test;
      
      /**
       * Demonstrates cases of should fail with cause.
       * @author Charlie Huggard-Lee 
       */
      class DummyTest extends GroovyTestCase {
          
          public static void failChecked() throws Exception {
              throw new Exception(new IOException());
          }
          
          public static void failUnchecked() {
              throw new RuntimeException(new IOException());
          }
          
          
          @Test
          public void testcheckedFailure() {
              //Hurray!
              shouldFailWithCause(IOException.class, {DummyTest.failChecked()})    
          }
          
          @Test
          public void testuncheckedFailure() {
              //BOO!!!
              shouldFailWithCause(IOException.class, {DummyTest.failUnchecked()})    
          }
          
      }
      

        Activity

        Hide
        Roshan Dawrani added a comment -

        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

        Show
        Roshan Dawrani added a comment - 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
        Hide
        Roshan Dawrani added a comment -

        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.

        Show
        Roshan Dawrani added a comment - 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.
        Hide
        Roshan Dawrani added a comment -

        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.

        Show
        Roshan Dawrani added a comment - 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 .
        Hide
        Paul King added a comment -

        I suspect so.

        Show
        Paul King added a comment - I suspect so.
        Hide
        Charlie Huggard-Lee added a comment -

        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();
            }
        
        Show
        Charlie Huggard-Lee added a comment - 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(); }
        Hide
        Roshan Dawrani added a comment -

        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 {}
        
        Show
        Roshan Dawrani added a comment - 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 {}
        Hide
        Charlie Huggard-Lee added a comment -

        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())
                }
            }
        
        Show
        Charlie Huggard-Lee added a comment - 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()) } }
        Hide
        Roshan Dawrani added a comment -

        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.

        Show
        Roshan Dawrani added a comment - 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.
        Hide
        Roshan Dawrani added a comment -

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

        Show
        Roshan Dawrani added a comment - And since it has not been having any problem handling exception cases with cause = null, making it so will cause another regression.
        Hide
        Charlie Huggard-Lee added a comment -

        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.

        Show
        Charlie Huggard-Lee added a comment - 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.
        Hide
        Charlie Huggard-Lee added a comment -

        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)
            }
        
        Show
        Charlie Huggard-Lee added a comment - 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) }
        Hide
        Charlie Huggard-Lee added a comment -

        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)
            }
        
        Show
        Charlie Huggard-Lee added a comment - 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) }
        Hide
        Charlie Huggard-Lee added a comment -

        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();
            }
        
        Show
        Charlie Huggard-Lee added a comment - 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(); }
        Hide
        Roshan Dawrani added a comment -

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

        Show
        Roshan Dawrani added a comment - The last patch looked ok to me and I have applied it. Thanks.
        Hide
        Paul King added a comment -

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

        Show
        Paul King added a comment - I am not sure the logic is correct here as patched. Pondering ...
        Hide
        Paul King added a comment -

        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.

        Show
        Paul King added a comment - 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.
        Hide
        Paul King added a comment -

        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.

        Show
        Paul King added a comment - 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.
        Hide
        Paul King added a comment -

        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.

        Show
        Paul King added a comment - 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.
        Hide
        Roshan Dawrani added a comment -

        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.

        Show
        Roshan Dawrani added a comment - 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.
        Hide
        Paul King added a comment -

        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.

        Show
        Paul King added a comment - 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.
        Hide
        Roshan Dawrani added a comment -

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

        Show
        Roshan Dawrani added a comment - Ok, let's do it - with a few lines of javadoc this time
        Hide
        Paul King added a comment -

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

        Show
        Paul King added a comment - Wasn't sure whether you wanted me to do it or not - so I went ahead and did it.
        Hide
        Roshan Dawrani added a comment -

        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.

        Show
        Roshan Dawrani added a comment - 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.
        Hide
        Paul King added a comment - - edited

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

        Show
        Paul King added a comment - - edited No worries at all. Now just to work out why tests pass locally and on Bamboo JDK1.6 but fail on Bamboo JDK1.5?
        Hide
        Roshan Dawrani added a comment -

        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.

        Show
        Roshan Dawrani added a comment - 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.
        Hide
        Roshan Dawrani added a comment -

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

        Show
        Roshan Dawrani added a comment - Here is a suggestion for IOException replacement in tests : java.util.concurrent.ExecutionException - has both the needed constructors.
        Hide
        Paul King added a comment -

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

        Show
        Paul King added a comment - Yes, I ran the tests locally on 1.5 and spotted that too. Just fixing now ...
        Hide
        Paul King added a comment -

        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.

        Show
        Paul King added a comment - 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.
        Hide
        Paul King added a comment -

        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.

        Show
        Paul King added a comment - 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.

          People

          • Assignee:
            Roshan Dawrani
            Reporter:
            Charlie Huggard-Lee
          • Votes:
            2 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development