Skip to content
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

Add support for chaining sequences of closures #979

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jff
Copy link

@jff jff commented Mar 14, 2019

This enables the use of closures in the lists passed to the triple-right-shift (>>>) operator.

Examples

  1. The expression

queue.poll() >> { throw new UnsupportedOperationException() } >> { throw new UnsupportedOperationException() } >> 0

can now be written as

queue.poll() >>> [{ throw new UnsupportedOperationException() }] * 2 >> 0

  1. The lists can be heterogeneous, containing both closures and values:

queue.poll() >>> [{ throw new UnsupportedOperationException() }, 0, { count++ }, 2, [3, 4]]

  1. More examples can be found in the test file spock-specs/src/test/groovy/org/spockframework/smoke/mock/ChainedResponseGenerators.groovy

This change is Reviewable

@codecov
Copy link

codecov bot commented Mar 14, 2019

Codecov Report

Merging #979 into master will decrease coverage by <.01%.
The diff coverage is 76.92%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #979      +/-   ##
============================================
- Coverage     75.99%   75.99%   -0.01%     
- Complexity     3544     3546       +2     
============================================
  Files           377      377              
  Lines         10788    10800      +12     
  Branches       1374     1377       +3     
============================================
+ Hits           8198     8207       +9     
- Misses         2109     2110       +1     
- Partials        481      483       +2
Impacted Files Coverage Δ Complexity Δ
...ework/mock/response/IterableResponseGenerator.java 83.33% <76.92%> (-16.67%) 7 <1> (+2)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed38b97...1da3126. Read the comment docs.

@leonard84
Copy link
Member

Hi @jff, thanks for implementing this feature. When changing something on the spock-core language level, we have to ask ourselves, does this new feature improve the readability/clarity of the test without adding too much variation. Having different ways of accomplishing the same goal might be nicer, but it also increases the cognitive load on those who have to read an comprehend your tests.

This feature does not add something that couldn't be achieved before. Yes

queue.poll() >>> [{ throw new UnsupportedOperationException() }] * 2 >> 0

is a bit shorter than

queue.poll() >> { throw new UnsupportedOperationException() } >> { throw new UnsupportedOperationException() } >> 0

but now the reader has to calculate to see what happens. See DRY vs. DAMP.

I'm not totally against it, but it does seem to be for some edge cases. How often would you actually use this new feature?

@jff
Copy link
Author

jff commented Mar 18, 2019

Hi @leonard84, thanks for the quick and pertinent reply. Please find below a few additional points:

  • I would say that your comment on increasing the cognitive load could also be applied to the existing operator >>>. Why provide the operator >>> and write

    subscriber.receive(_) >>> ["ok", "error", "error", "ok"]

    when one could simply do

    subscriber.receive(_) >> "ok" >> "error" >> "error" >> "ok"
    ?

  • One could see the operator >>> as a generalization of >>. However, from a type-oriented perspective this generalization breaks down, because whilst >> supports closures as operands, >>> does not. I don't see a good reason to have this inconsistency.

  • Regarding the DRY vs DAMP argument, I agree that duplication is more acceptable in tests than in software. However, in general, if we can avoid duplication without affecting the readability of the tests, I believe that we should avoid it. Spock's documentation mentions duplication as something to avoid. In fact, one of my favorite features in Spock, data tables, are an excellent mechanism to avoid duplication without hindering readability. My initial motivation to extend the operator >>> was to allow the use of sequences of closures in data tables.

    For example, I don't think it is far-fetched to have the need to test the internal state of a certain module that can throw a sequence of 3 or more exceptions (e.g. I am writing mock tests for a module that books "activities" involving hotels, cars, etc. and we are interested in testing certain sequences of exceptions).

    If we are in a context where we want to test a certain state for the permutations of three possible exceptions, we would probably have to write 6 test methods, one for each sequence:

    m() >> { throw new E1() } >> { throw new E2() } >> {thrown new E3()}
    ...
    when:
    x.process()
    then:
    x.getState() == STATE1
    
    m() >> { throw new E1() } >> { throw new E3() } >> {thrown new E2()}
    ...
    when:
    x.process()
    then:
    x.getState() == STATE2
    
    m() >> { throw new E2() } >> { throw new E1() } >> {thrown new E3()}
    ...
    when:
    x.process()
    then:
    x.getState() == STATE3
    
    m() >> { throw new E2() } >> { throw new E3() } >> {thrown new E1()}
    ...
    when:
    x.process()
    then:
    x.getState() == STATE3
    
    m() >> { throw new E3() } >> { throw new E1() } >> {thrown new E2()}
    ...
    when:
    x.process()
    then:
    x.getState() == STATE2
    
    m() >> { throw new E3() } >> { throw new E2() } >> {thrown new E1()}
    ...
    when:
    x.process()
    then:
    x.getState() == STATE1
    

    Using data tables and the new operator >>>, one could write a single test method:

    m() >>> sequence_exceptions
    
    when:
    x.process()
    then:
    x.getState() == state
    
    
    where:
    sequence_exceptions                                     | state
    [{throw new E1()}, {throw new E2()}, {thrown new E3()}] | STATE1
    [{throw new E1()}, {throw new E3()}, {thrown new E2()}] | STATE2
    [{throw new E2()}, {throw new E1()}, {thrown new E3()}] | STATE3
    [{throw new E2()}, {throw new E3()}, {thrown new E1()}] | STATE3
    [{throw new E3()}, {throw new E1()}, {thrown new E2()}] | STATE2
    [{throw new E3()}, {throw new E2()}, {thrown new E1()}] | STATE1
    

    This is, in my view, more readable, mainly because I can see all the cases together without jumping
    from method to method.

    Perhaps this feature will not be used in every project, but I believe that it improves the type consistency of >>> (when seen as a generalization of >>). I also believe that there are projects that could benefit from having it, for it would improve the readability of tests that involve sequences of closures.

@leonard84
Copy link
Member

Another problem we have, is that using >>> [{ code }] is currently the only easy way to actually return a closure. Which makes this a breaking change. As a workaround you could do >> {{ code }} but that looks strange.

@leonard84 leonard84 added this to the 2.0 milestone Mar 21, 2019
@leonard84 leonard84 removed this from the 2.0 milestone Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants