groovy
  1. groovy
  2. GROOVY-5139

GroovyTestCase's shouldFail {...} should return the throwable and not the enclosed message

    Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.8.4, 2.0-beta-1
    • Fix Version/s: 1.8.6, 2.0-beta-3
    • Component/s: None
    • Labels:
      None
    • Testcase included:
      yes
    • Patch Submitted:
      Yes
    • Number of attachments :
      1

      Description

      As a part of GroovyTestCase shouldFail should return the entire throwable and not just the enclosed message. There may be other items on the exception that bears inspection, assertion.

      The easy workaround to fix existing code is to tack a '?.message' on existing tests.

        Activity

        Show
        Colin Harrington added a comment - Pull request sent: https://github.com/groovy/groovy-core/pull/9 commit (mostly fixing Tests): https://github.com/ColinHarrington/groovy-core/commit/47328ab76ae620c41f74301bace5d899d65dd229
        Hide
        Colin Harrington added a comment -

        Also including a patch for those who want it in this form.

        Show
        Colin Harrington added a comment - Also including a patch for those who want it in this form.
        Hide
        CÚdric Champeau added a comment -

        While this looks useful, it's a breaking change. Not only Groovy unit tests need to be changed, but potentially tests written by users. Maybe we could provide an alternate "shouldFailWithError" method.

        Show
        CÚdric Champeau added a comment - While this looks useful, it's a breaking change. Not only Groovy unit tests need to be changed, but potentially tests written by users. Maybe we could provide an alternate "shouldFailWithError" method.
        Hide
        Colin Harrington added a comment -

        This is a breaking change. But if we are going to do this, it would make sense that it would be part of a 2.0 release.

        The workaround is simple for users - tack a '.message' on the end of your shouldFail closure.

        shouldFail returning a message made sense in the early days of Groovy and the simple cases. As soon as you need to do assertions against anything else on the exception besides the type and message using shouldFail breaks down and you have to resort to less groovy try-catch assertions.

        Show
        Colin Harrington added a comment - This is a breaking change. But if we are going to do this, it would make sense that it would be part of a 2.0 release. The workaround is simple for users - tack a '.message' on the end of your shouldFail closure. shouldFail returning a message made sense in the early days of Groovy and the simple cases. As soon as you need to do assertions against anything else on the exception besides the type and message using shouldFail breaks down and you have to resort to less groovy try-catch assertions.
        Hide
        blackdrag blackdrag added a comment -

        If I did not oversee something in the patch, then you don't need to do ?.message, just .message is enough, since if a Throwable is returned, there will be one, and if not, then the method will throw an exception, thus not reaching that point.

        One question Colin... what speaks against using another method and have less breaking code?

        Show
        blackdrag blackdrag added a comment - If I did not oversee something in the patch, then you don't need to do ?.message, just .message is enough, since if a Throwable is returned, there will be one, and if not, then the method will throw an exception, thus not reaching that point. One question Colin... what speaks against using another method and have less breaking code?
        Hide
        Guillaume Laforge added a comment -

        I would also prefer a new method rather than breaking code.
        It's not because we're going to release a new major version that it's okay to break people's code, except if it's a very valid reason (ie. a deep bug in Groovy, a bad behavior that needs fixing, etc)
        Here, we're speaking about convenience, and I think adding a new method is a nice convenience, but breaking code is more painful.

        Show
        Guillaume Laforge added a comment - I would also prefer a new method rather than breaking code. It's not because we're going to release a new major version that it's okay to break people's code, except if it's a very valid reason (ie. a deep bug in Groovy, a bad behavior that needs fixing, etc) Here, we're speaking about convenience, and I think adding a new method is a nice convenience, but breaking code is more painful.
        Hide
        CÚdric Champeau added a comment -

        Colin, could you provide another patch with an alternate method ?

        Show
        CÚdric Champeau added a comment - Colin, could you provide another patch with an alternate method ?
        Hide
        Paul King added a comment -

        Or if you are too busy, let us know. We'd love you to claim the credit for the new feature from a git commit point of view but if that isn't possible we can progress it ourselves.

        Show
        Paul King added a comment - Or if you are too busy, let us know. We'd love you to claim the credit for the new feature from a git commit point of view but if that isn't possible we can progress it ourselves.
        Hide
        Paul King added a comment -

        A slight variation of Colin's proposal would be to leave the methods in GroovyTestCase alone and provide the modified shouldFail methods in JUnit4 style static methods - providing the enhanced functionality and also making it more easily available to JUnit4+. E.g. usage would be like:

        import org.junit.Test
        import static groovy.util.GroovyAssert.shouldFail
        
        class MathTest {
          @Test
          void testDivision() {
            def e = shouldFail { 2 / 0 }
            assert e instanceof ArithmeticException
            assert e.message == 'Division by zero'
          }
        }
        

        This doesn't break legacy code and provides an easy upgrade path for people wanting the new functionality. The '?.message' hack is avoided for existing tests but could be documented as a strategy for converting between the two styles.

        Show
        Paul King added a comment - A slight variation of Colin's proposal would be to leave the methods in GroovyTestCase alone and provide the modified shouldFail methods in JUnit4 style static methods - providing the enhanced functionality and also making it more easily available to JUnit4+. E.g. usage would be like: import org.junit.Test import static groovy.util.GroovyAssert.shouldFail class MathTest { @Test void testDivision() { def e = shouldFail { 2 / 0 } assert e instanceof ArithmeticException assert e.message == 'Division by zero' } } This doesn't break legacy code and provides an easy upgrade path for people wanting the new functionality. The '?.message' hack is avoided for existing tests but could be documented as a strategy for converting between the two styles.
        Hide
        Colin Harrington added a comment -

        @Paul King I like this approach.

        Show
        Colin Harrington added a comment - @Paul King I like this approach.
        Hide
        blackdrag blackdrag added a comment -

        I resolved the issue for Paul, since he seemed to have forgotten

        Show
        blackdrag blackdrag added a comment - I resolved the issue for Paul, since he seemed to have forgotten

          People

          • Assignee:
            Paul King
            Reporter:
            Colin Harrington
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: