New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

assert with message disables power assertion output #922

Closed
kriegaex opened this Issue Sep 27, 2018 · 9 comments

Comments

Projects
None yet
3 participants
@kriegaex

kriegaex commented Sep 27, 2018

With a feature method like this

def one() {
	expect:
	("one".capitalize() * 3).toUpperCase() == "OneOneOne"
}

we get an error message like that:

Condition not satisfied:

("one".capitalize() * 3).toUpperCase() == "OneOneOne"
       |            |    |             |
       One          |    ONEONEONE     false
                    OneOneOne          6 differences (33% similarity)
                                       O(NE)O(NE)O(NE)
                                       O(ne)O(ne)O(ne)

Expected :OneOneOne

Actual   :ONEONEONE

The same is true if we explicitly use assert at the beginning of the condition. So far, so good.

Now this is what happens when using assert with an additional error message:

def one() {
	expect:
	assert ("one".capitalize() * 3).toUpperCase() == "OneOneOne", "My custom message"
}

The very helpful power assert output vanishes.

Condition not satisfied:

("one".capitalize() * 3).toUpperCase() == "OneOneOne"

My custom message

What I would like to see instead is the custom message prepended to the power assert output like this:

My custom message

Condition not satisfied:

("one".capitalize() * 3).toUpperCase() == "OneOneOne"
       |            |    |             |
       One          |    ONEONEONE     false
                    OneOneOne          6 differences (33% similarity)
                                       O(NE)O(NE)O(NE)
                                       O(ne)O(ne)O(ne)

Expected :OneOneOne

Actual   :ONEONEONE

Would it be possible to add a corresponding AST transformation for that kind of syntax?

kriegaex pushed a commit to kriegaex/GebSpockSamples that referenced this issue Sep 27, 2018

@leonard84

This comment has been minimized.

Show comment
Hide comment
@leonard84

leonard84 Sep 27, 2018

Member

Yes it is possible, take a look at:

  • org.spockframework.compiler.ConditionRewriter#rewriteExplicitCondition
  • org.spockframework.compiler.ConditionRewriter#rewriteOtherCondition
  • org.spockframework.compiler.ConditionRewriter#rewriteMethodCondition
  • org.spockframework.compiler.ConditionRewriter#rewriteStaticMethodCondition

FYI the syntax is slightly different assert expression [ : message ]

The question should be if it should?
Adding a custom message currently completely removes the recording of arguments.
This behavior might be helpful. Furthermore, if will break tests that rely on the specific message (although they should be few)


Member

leonard84 commented Sep 27, 2018

Yes it is possible, take a look at:

  • org.spockframework.compiler.ConditionRewriter#rewriteExplicitCondition
  • org.spockframework.compiler.ConditionRewriter#rewriteOtherCondition
  • org.spockframework.compiler.ConditionRewriter#rewriteMethodCondition
  • org.spockframework.compiler.ConditionRewriter#rewriteStaticMethodCondition

FYI the syntax is slightly different assert expression [ : message ]

The question should be if it should?
Adding a custom message currently completely removes the recording of arguments.
This behavior might be helpful. Furthermore, if will break tests that rely on the specific message (although they should be few)


@kriegaex

This comment has been minimized.

Show comment
Hide comment
@kriegaex

kriegaex Sep 28, 2018

As for the syntax assert expression [ : message ], yes, it works like that. But the syntax assert expression [, message ] seems to be equivalent. At least it yields the exact same result in my test case. I was also surprised.

My vote is for a breaking change because I think it is what a Spock user probably expects: Spock power assertion features, no matter whether he parametrises the assert or not. Two different behaviours are rather unexpected IMO.

kriegaex commented Sep 28, 2018

As for the syntax assert expression [ : message ], yes, it works like that. But the syntax assert expression [, message ] seems to be equivalent. At least it yields the exact same result in my test case. I was also surprised.

My vote is for a breaking change because I think it is what a Spock user probably expects: Spock power assertion features, no matter whether he parametrises the assert or not. Two different behaviours are rather unexpected IMO.

@paulk-asert

This comment has been minimized.

Show comment
Hide comment
@paulk-asert

paulk-asert Sep 28, 2018

Contributor

Spock behavior doesn't have to mirror vanilla Groovy, but in vanilla Groovy when adding our simplified power assert we wanted a way to disable the generated message. You might have expressions that contain objects with sensitive data or self-recursive data-structures or believe you have a better way to present the information. We decided that if the optional additional message was supplied, that would disable the generated content. We tried thinking of a way to make the generated message available to be incorporated manually, e.g. if the supplied additional message was a GString but never found a solution we were happy with.

Contributor

paulk-asert commented Sep 28, 2018

Spock behavior doesn't have to mirror vanilla Groovy, but in vanilla Groovy when adding our simplified power assert we wanted a way to disable the generated message. You might have expressions that contain objects with sensitive data or self-recursive data-structures or believe you have a better way to present the information. We decided that if the optional additional message was supplied, that would disable the generated content. We tried thinking of a way to make the generated message available to be incorporated manually, e.g. if the supplied additional message was a GString but never found a solution we were happy with.

@leonard84

This comment has been minimized.

Show comment
Hide comment
@leonard84

leonard84 Sep 28, 2018

Member

@kriegaex what do you think about #923, would this be a viable alternative for you?

@paulk-asert thanks for the information, I was thinking along the same lines.

Member

leonard84 commented Sep 28, 2018

@kriegaex what do you think about #923, would this be a viable alternative for you?

@paulk-asert thanks for the information, I was thinking along the same lines.

@kriegaex

This comment has been minimized.

Show comment
Hide comment
@kriegaex

kriegaex Sep 29, 2018

@paulk-asert, I cannot follow your reasoning because if this was the intention and important enough to implement the "feature" that way, why is it undocumented? I am sure there must be a way to make it configurable to not have power assert output if someone does not want it. An annotation-driven extension, SpockConfig.groovy or a combination of both, default config plus override per spec or feature method. Maybe even an override per (block of) condition(s) via with(SomeCategory) would be possible, but I am not the implementation expert here.

Apart from the technology discussion, why would test data ever be sensitive enough not to print it? And if it was self-recursive, the printing problem would also apply to normal power asserts and should be solved within the framework, not by forcing users to disable power assert with a rather strange trick, documented or not.

@leonard84, #923 is not a solution for me, even though label printing in general is a frequently requested optional feature which I also would like to have, not just for failing assertions. Of course I also came up with a workaround for the missing power output long ago, but it is ugly. FYI:

package de.scrum_master.testing

import org.spockframework.runtime.ConditionNotSatisfiedError
import spock.lang.Specification

/**
 * This is an ugly workaround for https://github.com/spockframework/spock/issues/922
 */
class EnhancedPowerAssertionTest extends Specification {
  static class PowerAssertionEnhancer {
    private static final String DASHED_LINE = "=" * 60

    static boolean specialAssert(String extraMessage, Closure assertion) {
      try { assertion() }
      catch(ConditionNotSatisfiedError e) {
        System.err.println DASHED_LINE
        System.err.println extraMessage
        System.err.println DASHED_LINE
        throw e
      }
      true
    }
  }

  def setupSpec() {
    getClass().mixin PowerAssertionEnhancer
  }

  def "Power-assert output for unparametrised 'assert'"() {
    expect:
    assert ("one".capitalize() * 3).toUpperCase() == "OneOneOne"
  }

  def "Power-assert output for implicit Spock-style 'assert'"() {
    expect:
    ("one".capitalize() * 3).toUpperCase() == "OneOneOne"
  }

  def "No power-assert output for parametrised 'assert'"() {
    expect:
    assert ("one".capitalize() * 3).toUpperCase() == "OneOneOne" :
      "This is some additional info about the error"
  }

  def "Combine power-assert output with extra failure log output"() {
    expect:
    specialAssert "This is some additional info about the error", {
      assert ("one".capitalize() * 3).toUpperCase() == "OneOneOne"
    }
  }
}

Oh, BTW, one example of what we used my workaround for in a real-life project was an end-to-end integration test which often failed due to shortcomings in availability in a business partner's third-party system. We could detect and discern availability problems, configuration problems and certificate problems and print comprehensive messages for each about the reason and whom to contact in order to get it fixed. So nobody needed to analyse the call stacks or read the source code in order to react in the right way and "fix the build" (in reality, get the 3rd party server fixed). Admittedly, this is a special case. But so is using assert : "message" in general. I use it quite sparingly, but if I do, I want power assert output because I always like to have a maximum of information about why a test might have failed. In the example above, certificate problems were likely to be caused by an expired server certificate, but in theory the reason for a failed SSL handshake could also have been another one, such as an outdated trust store on the client side.

To make a long story short: The requested feature would be a really helpful thing to have in some situations. Please consider implementing it and making it the default behaviour - with an option to deactivate it if you think it is necessary.

kriegaex commented Sep 29, 2018

@paulk-asert, I cannot follow your reasoning because if this was the intention and important enough to implement the "feature" that way, why is it undocumented? I am sure there must be a way to make it configurable to not have power assert output if someone does not want it. An annotation-driven extension, SpockConfig.groovy or a combination of both, default config plus override per spec or feature method. Maybe even an override per (block of) condition(s) via with(SomeCategory) would be possible, but I am not the implementation expert here.

Apart from the technology discussion, why would test data ever be sensitive enough not to print it? And if it was self-recursive, the printing problem would also apply to normal power asserts and should be solved within the framework, not by forcing users to disable power assert with a rather strange trick, documented or not.

@leonard84, #923 is not a solution for me, even though label printing in general is a frequently requested optional feature which I also would like to have, not just for failing assertions. Of course I also came up with a workaround for the missing power output long ago, but it is ugly. FYI:

package de.scrum_master.testing

import org.spockframework.runtime.ConditionNotSatisfiedError
import spock.lang.Specification

/**
 * This is an ugly workaround for https://github.com/spockframework/spock/issues/922
 */
class EnhancedPowerAssertionTest extends Specification {
  static class PowerAssertionEnhancer {
    private static final String DASHED_LINE = "=" * 60

    static boolean specialAssert(String extraMessage, Closure assertion) {
      try { assertion() }
      catch(ConditionNotSatisfiedError e) {
        System.err.println DASHED_LINE
        System.err.println extraMessage
        System.err.println DASHED_LINE
        throw e
      }
      true
    }
  }

  def setupSpec() {
    getClass().mixin PowerAssertionEnhancer
  }

  def "Power-assert output for unparametrised 'assert'"() {
    expect:
    assert ("one".capitalize() * 3).toUpperCase() == "OneOneOne"
  }

  def "Power-assert output for implicit Spock-style 'assert'"() {
    expect:
    ("one".capitalize() * 3).toUpperCase() == "OneOneOne"
  }

  def "No power-assert output for parametrised 'assert'"() {
    expect:
    assert ("one".capitalize() * 3).toUpperCase() == "OneOneOne" :
      "This is some additional info about the error"
  }

  def "Combine power-assert output with extra failure log output"() {
    expect:
    specialAssert "This is some additional info about the error", {
      assert ("one".capitalize() * 3).toUpperCase() == "OneOneOne"
    }
  }
}

Oh, BTW, one example of what we used my workaround for in a real-life project was an end-to-end integration test which often failed due to shortcomings in availability in a business partner's third-party system. We could detect and discern availability problems, configuration problems and certificate problems and print comprehensive messages for each about the reason and whom to contact in order to get it fixed. So nobody needed to analyse the call stacks or read the source code in order to react in the right way and "fix the build" (in reality, get the 3rd party server fixed). Admittedly, this is a special case. But so is using assert : "message" in general. I use it quite sparingly, but if I do, I want power assert output because I always like to have a maximum of information about why a test might have failed. In the example above, certificate problems were likely to be caused by an expired server certificate, but in theory the reason for a failed SSL handshake could also have been another one, such as an outdated trust store on the client side.

To make a long story short: The requested feature would be a really helpful thing to have in some situations. Please consider implementing it and making it the default behaviour - with an option to deactivate it if you think it is necessary.

@leonard84

This comment has been minimized.

Show comment
Hide comment
@leonard84

leonard84 Oct 2, 2018

Member

I still see the need to be able to suppress a message, especially if we are using a third-party library, that provides its own exception messages.

 resultActions.andExpect(status().isOk())
                .andExpect(view().name('login'))
                .andExpect(content().string(
                CoreMatchers.allOf(
                        containsString('<input type="hidden" name="service" value="foo" />'),
                        containsString('<input type="hidden" name="successURL" value="https:/login/success?kmsi=false" />'),
                        containsString('<input type="hidden" name="loginFailedURL" value="https:/login?failed=true&amp;login_hint=username@web.de" />'),
                        containsString('<input type="hidden" name="loginErrorURL" value="https:/login/error" />'),
                        containsString('<input type="hidden" name="statistics" value="foo" />'),
                        containsString('<input type="hidden" name="username" value="username@web.de" />'),
                        matcher
                )))

renders as

Condition failed with Exception:

resultActions.andExpect(status().isOk()) .andExpect(view().name('login')) .andExpect(content().string( CoreMatchers.allOf( containsString('<input type="hidden" name="service" value="foo" />'), containsString('<input type="hidden" name="successURL" value="https:/login/success?kmsi=false" />'), containsString('<input type="hidden" name="loginFailedURL" value="https:/login?failed=true&amp;login_hint=username@web.de" />'), containsString('<input type="hidden" name="loginErrorURL" value="https:/login/error" />'), containsString('<input type="hidden" name="statistics" value="foo" />'), containsString('<input type="hidden" name="username" value="username@web.de" />'), matcher )))
|             |         |        |        |         |      |
|             |         |        |        |         |      <org.springframework.test.web.servlet.result.ViewResultMatchers$$Lambda$382/219130824@77f4038c arg$1=login>
|             |         |        |        |         <org.springframework.test.web.servlet.result.ViewResultMatchers@27a90ce5>
|             |         |        |        java.lang.AssertionError: No ModelAndView found
|             |         |        <org.springframework.test.web.servlet.result.StatusResultMatchers$$Lambda$381/1314050802@51869ad6 arg$1=200>
|             |         <org.springframework.test.web.servlet.result.StatusResultMatchers@36511772>
|             <org.springframework.test.web.servlet.MockMvc$1@795faad val$mvcResult=org.springframework.test.web.servlet.DefaultMvcResult@1eb9d69a this$0=org.springframework.test.web.servlet.MockMvc@73385d3f>
<org.springframework.test.web.servlet.MockMvc$1@795faad val$mvcResult=org.springframework.test.web.servlet.DefaultMvcResult@1eb9d69a this$0=org.springframework.test.web.servlet.MockMvc@73385d3f>

The spring exception message is listed as cause.

with assert <expectation> : 'MVC Expectation Failed' we get

Condition failed with Exception:

resultActions.andExpect(status().isOk()) .andExpect(view().name('login')) .andExpect(content().string( CoreMatchers.allOf( containsString('<input type="hidden" name="service" value="foo" />'), containsString('<input type="hidden" name="successURL" value="https:/login/success?kmsi=false" />'), containsString('<input type="hidden" name="loginFailedURL" value="https:/login?failed=true&amp;login_hint=username@web.de" />'), containsString('<input type="hidden" name="loginErrorURL" value="https:/login/error" />'), containsString('<input type="hidden" name="statistics" value="foo" />'), containsString('<input type="hidden" name="username" value="username@web.de" />'), matcher )))

MVC Expectation Failed

I would argue it would be event better to hide the complete expression. So if we change assert exp : message then we need something for this, any ideas?

Member

leonard84 commented Oct 2, 2018

I still see the need to be able to suppress a message, especially if we are using a third-party library, that provides its own exception messages.

 resultActions.andExpect(status().isOk())
                .andExpect(view().name('login'))
                .andExpect(content().string(
                CoreMatchers.allOf(
                        containsString('<input type="hidden" name="service" value="foo" />'),
                        containsString('<input type="hidden" name="successURL" value="https:/login/success?kmsi=false" />'),
                        containsString('<input type="hidden" name="loginFailedURL" value="https:/login?failed=true&amp;login_hint=username@web.de" />'),
                        containsString('<input type="hidden" name="loginErrorURL" value="https:/login/error" />'),
                        containsString('<input type="hidden" name="statistics" value="foo" />'),
                        containsString('<input type="hidden" name="username" value="username@web.de" />'),
                        matcher
                )))

renders as

Condition failed with Exception:

resultActions.andExpect(status().isOk()) .andExpect(view().name('login')) .andExpect(content().string( CoreMatchers.allOf( containsString('<input type="hidden" name="service" value="foo" />'), containsString('<input type="hidden" name="successURL" value="https:/login/success?kmsi=false" />'), containsString('<input type="hidden" name="loginFailedURL" value="https:/login?failed=true&amp;login_hint=username@web.de" />'), containsString('<input type="hidden" name="loginErrorURL" value="https:/login/error" />'), containsString('<input type="hidden" name="statistics" value="foo" />'), containsString('<input type="hidden" name="username" value="username@web.de" />'), matcher )))
|             |         |        |        |         |      |
|             |         |        |        |         |      <org.springframework.test.web.servlet.result.ViewResultMatchers$$Lambda$382/219130824@77f4038c arg$1=login>
|             |         |        |        |         <org.springframework.test.web.servlet.result.ViewResultMatchers@27a90ce5>
|             |         |        |        java.lang.AssertionError: No ModelAndView found
|             |         |        <org.springframework.test.web.servlet.result.StatusResultMatchers$$Lambda$381/1314050802@51869ad6 arg$1=200>
|             |         <org.springframework.test.web.servlet.result.StatusResultMatchers@36511772>
|             <org.springframework.test.web.servlet.MockMvc$1@795faad val$mvcResult=org.springframework.test.web.servlet.DefaultMvcResult@1eb9d69a this$0=org.springframework.test.web.servlet.MockMvc@73385d3f>
<org.springframework.test.web.servlet.MockMvc$1@795faad val$mvcResult=org.springframework.test.web.servlet.DefaultMvcResult@1eb9d69a this$0=org.springframework.test.web.servlet.MockMvc@73385d3f>

The spring exception message is listed as cause.

with assert <expectation> : 'MVC Expectation Failed' we get

Condition failed with Exception:

resultActions.andExpect(status().isOk()) .andExpect(view().name('login')) .andExpect(content().string( CoreMatchers.allOf( containsString('<input type="hidden" name="service" value="foo" />'), containsString('<input type="hidden" name="successURL" value="https:/login/success?kmsi=false" />'), containsString('<input type="hidden" name="loginFailedURL" value="https:/login?failed=true&amp;login_hint=username@web.de" />'), containsString('<input type="hidden" name="loginErrorURL" value="https:/login/error" />'), containsString('<input type="hidden" name="statistics" value="foo" />'), containsString('<input type="hidden" name="username" value="username@web.de" />'), matcher )))

MVC Expectation Failed

I would argue it would be event better to hide the complete expression. So if we change assert exp : message then we need something for this, any ideas?

@kriegaex

This comment has been minimized.

Show comment
Hide comment
@kriegaex

kriegaex Oct 4, 2018

Well, that's why my last sentence was:

Please consider implementing it and making it the default behaviour - with an option to deactivate it if you think it is necessary.

So providing an option to users to override the default behaviour (power-assert output) is, as I said, completely fine with me - not that you need my approval. ;-)

The sample log output you just showed might be a case in which a user would want to avoid power assert output, even though I think that it is rather exotic to use extra libraries for assertions even though Spock provides its nice condition auto-assert which is no less expressive and easy to use. BTW, I would still like to have the power assert output if I could have the additional text I want. Why? The first line is identical anyway, but looks way uglier than it would using Spock conditions. Now we cannot even clearly see which of the many assertions failed. The power assert output at least helps a bit here, indicating that .andExpect(view().name('login')) failed with java.lang.AssertionError: No ModelAndView found. So actually it is a case for wanting power assert, not for avoiding it. It helps me more than the general information "MVC expectation failed". Of course it did, the power assert output tells me already.

As for how to implement the feature, I wrote something about it in a previous comment, too:

I am sure there must be a way to make it configurable to not have power assert output if someone does not want it. An annotation-driven extension, SpockConfig.groovy or a combination of both, default config plus override per spec or feature method. Maybe even an override per (block of) condition(s) via with(SomeCategory) would be possible, but I am not the implementation expert here.

The idea that was mentioned by @paulk-asert about different behaviour for String vs. GString would be quite counter-intuitive, I think we should not go that way. Another similar option might be a syntax like assert condition : ["Additional message"] which could be used like assert condition : ["First paragraph", "Second paragraph"]. I.e. if there was some kind of collection or array as an argument, Spock could use power assert output. It feels a bit ugly and definitely would need explicit documentation, but so would all other options.

I am just thinking aloud here and am really open to other ways of implementing or using this. Even if it was just a workaround like powerAssert condition , "message" it would be better than now because at least I could use it. I still would be happier with making power assert the default and providing ways to disable it, see my quote above.

kriegaex commented Oct 4, 2018

Well, that's why my last sentence was:

Please consider implementing it and making it the default behaviour - with an option to deactivate it if you think it is necessary.

So providing an option to users to override the default behaviour (power-assert output) is, as I said, completely fine with me - not that you need my approval. ;-)

The sample log output you just showed might be a case in which a user would want to avoid power assert output, even though I think that it is rather exotic to use extra libraries for assertions even though Spock provides its nice condition auto-assert which is no less expressive and easy to use. BTW, I would still like to have the power assert output if I could have the additional text I want. Why? The first line is identical anyway, but looks way uglier than it would using Spock conditions. Now we cannot even clearly see which of the many assertions failed. The power assert output at least helps a bit here, indicating that .andExpect(view().name('login')) failed with java.lang.AssertionError: No ModelAndView found. So actually it is a case for wanting power assert, not for avoiding it. It helps me more than the general information "MVC expectation failed". Of course it did, the power assert output tells me already.

As for how to implement the feature, I wrote something about it in a previous comment, too:

I am sure there must be a way to make it configurable to not have power assert output if someone does not want it. An annotation-driven extension, SpockConfig.groovy or a combination of both, default config plus override per spec or feature method. Maybe even an override per (block of) condition(s) via with(SomeCategory) would be possible, but I am not the implementation expert here.

The idea that was mentioned by @paulk-asert about different behaviour for String vs. GString would be quite counter-intuitive, I think we should not go that way. Another similar option might be a syntax like assert condition : ["Additional message"] which could be used like assert condition : ["First paragraph", "Second paragraph"]. I.e. if there was some kind of collection or array as an argument, Spock could use power assert output. It feels a bit ugly and definitely would need explicit documentation, but so would all other options.

I am just thinking aloud here and am really open to other ways of implementing or using this. Even if it was just a workaround like powerAssert condition , "message" it would be better than now because at least I could use it. I still would be happier with making power assert the default and providing ways to disable it, see my quote above.

@leonard84

This comment has been minimized.

Show comment
Hide comment
@leonard84

leonard84 Oct 4, 2018

Member

I don't want a config switch. First, it adds hidden information to the behavior of Spock, so a reviewer would also have to know about that. Second, it would be global so you couldn't mix it. Third, this needs to be known at compile time not test runtime, which is also confusing for the users.

even though I think that it is rather exotic to use an extra libraries for assertions even though Spock provides its nice condition auto-assert which is no less expressive and easy to use.

That depends, I agree it doesn't make sense to add standalone assertion libraries. However, if the framework you are working with (e.g., Spring MVC) provides test support then it is a valid scenario.

Spring MockMVC does output more than a page on system.out with additional information. So I would actually remove the whole condition rendering as well as the power assertions. It does not have to be assert at all, maybe we could use something better to indicate that the method we are calling provides its own diagnostics.

Member

leonard84 commented Oct 4, 2018

I don't want a config switch. First, it adds hidden information to the behavior of Spock, so a reviewer would also have to know about that. Second, it would be global so you couldn't mix it. Third, this needs to be known at compile time not test runtime, which is also confusing for the users.

even though I think that it is rather exotic to use an extra libraries for assertions even though Spock provides its nice condition auto-assert which is no less expressive and easy to use.

That depends, I agree it doesn't make sense to add standalone assertion libraries. However, if the framework you are working with (e.g., Spring MVC) provides test support then it is a valid scenario.

Spring MockMVC does output more than a page on system.out with additional information. So I would actually remove the whole condition rendering as well as the power assertions. It does not have to be assert at all, maybe we could use something better to indicate that the method we are calling provides its own diagnostics.

@kriegaex

This comment has been minimized.

Show comment
Hide comment
@kriegaex

kriegaex Oct 8, 2018

I don't want a config switch. First, it adds hidden information to the behavior of Spock, so a reviewer would also have to know about that. Second, it would be global so you couldn't mix it. Third, this needs to be known at compile time not test runtime, which is also confusing for the users.

I suggested a combination of global config + annotation-driven extension. The former would have the advantage that I could set the desired default behaviour for a whole project and not just class by class or method by method. The latter would enable developers to override on a case-by-case basis. But if you dislike the idea of a global switch, I can also live with the annotation-driven extension only or any of the other approaches I suggested.

It does not have to be assert at all, maybe we could use something better to indicate that the method we are calling provides its own diagnostics.

Yes, see above. It would be fantastic if assert + parameter would use power-assert output but the user would still have the ability to indicate she does not want it in certain cases.

@leonard84, to me is looks as though we are talking "past each other". Most of the issues you keep mentioning I have already addressed in my previous suggestions. I am really trying to be constructive here. Please tell me if you need more input. :-)

kriegaex commented Oct 8, 2018

I don't want a config switch. First, it adds hidden information to the behavior of Spock, so a reviewer would also have to know about that. Second, it would be global so you couldn't mix it. Third, this needs to be known at compile time not test runtime, which is also confusing for the users.

I suggested a combination of global config + annotation-driven extension. The former would have the advantage that I could set the desired default behaviour for a whole project and not just class by class or method by method. The latter would enable developers to override on a case-by-case basis. But if you dislike the idea of a global switch, I can also live with the annotation-driven extension only or any of the other approaches I suggested.

It does not have to be assert at all, maybe we could use something better to indicate that the method we are calling provides its own diagnostics.

Yes, see above. It would be fantastic if assert + parameter would use power-assert output but the user would still have the ability to indicate she does not want it in certain cases.

@leonard84, to me is looks as though we are talking "past each other". Most of the issues you keep mentioning I have already addressed in my previous suggestions. I am really trying to be constructive here. Please tell me if you need more input. :-)

leonard84 added a commit to leonard84/spock that referenced this issue Oct 9, 2018

leonard84 added a commit to leonard84/spock that referenced this issue Oct 14, 2018

leonard84 added a commit to leonard84/spock that referenced this issue Oct 14, 2018

leonard84 added a commit to leonard84/spock that referenced this issue Oct 14, 2018

leonard84 added a commit that referenced this issue Oct 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment