spock icon indicating copy to clipboard operation
spock copied to clipboard

Allow additional parameters in feature methods

Open Fuud opened this issue 9 years ago • 9 comments

With this PR it will be possible to declare additional parameters (that is not specified in where block) in feature methods. Values can be set via interceptors.

Main usage scenario of this PR is to allow some kind of dependency injection: some parameters are set from where block and others set by interceptor (for example Spring beans can be injected or other contexts)

Additionally this PR allows to omit some of data variables from parameters list (it is usefull with #526 - intermediate parameters can be dropped)


This change is Reviewable

Fuud avatar Jan 22 '16 12:01 Fuud

This PR looks like a building block for some bigger task (like externalization of data providers). It would be probably good to describe (at least in the PR itself description for a while) the idea of its practical usage.

szpak avatar Jan 22 '16 19:01 szpak

As I detailed my arguments in https://github.com/spockframework/spock/pull/525#issuecomment-165036759 already, since parameters were thought to be interchangeable (there were even wrong tests, asserting it), but weren't in reality (int a, int b with values b=2, a=1 were simply switched, i.e. a was 2 and b was 1), and because non-data provider parameters could end up in the parameter list also (making it unobvious which is which), I strongly disagree with the order of parameters being random.

l0rinc avatar Jan 23 '16 10:01 l0rinc

@paplorinc About order: Main idea is to allow random parameter order and map parameters by name only. For example test1 and test2 are same and both will pass:

def test1(int a, int b) {
  expect:
    a < b
  where:
    a | b
    1 | 2
    3 | 4
}

def test2(int a, int b) {
  expect:
    a < b
  where:
    b | a
    2 | 1
    4 | 3
}

@szpak Fixed, PR was updated.

Fuud avatar Jan 24 '16 11:01 Fuud

@Fuud, I understood what the intention was, but it's the exact thing I prohibited in my PR, because I consider it confusing.

l0rinc avatar Jan 24 '16 12:01 l0rinc

@paplorinc I can not understand what can confuse. Just forget about order and all will become clear. Nobody rely on parameter order when invoking method (if it is not reflection case) - only on names.

Fuud avatar Jan 24 '16 13:01 Fuud

Please take a look at https://github.com/spockframework/spock/pull/525/files#diff-eb35d0e75f7f2a418c977d88fde43275R97, where I have data provider parameters and type specifying parameters mixed up. Wouldn't you find it confusing, if one data provider param would be at the beginning and the other at the end - especially if some parameters were optional?

I simply don't see any value in having multiple sorting criteria for the same thing (i.e. what's the value in having a | b | c in the test, but b, a, c in the parameter list?)

l0rinc avatar Jan 24 '16 14:01 l0rinc

May be something like this (better to place source value at the first place):

def "test decryption"(byte[] encrypted, byte[] key, byte[] expectedDecrypted){
  when:
    byte[] decrypted = new Decryptor().decrypt(key, encrypted)
  then:
    arrayEquals(expectedDecrypted, decrypted)
  where: // encrypted is calculated => should be placed after its dependencies
    expectedDecrypted                  | key                                            | encrypted
    "sadfasdfasdfasd".getBytes()   | "56454sdfasdfasdf".getBytes() | encrypt(key, expectedDecrypted)
}

My PR forbids optional parameters. All parameters should be set before method is invoked (it can be done by data-provider or by interceptor).

Wouldn't you find it confusing, if one data provider param would be at the beginning and the other at the end [...]

No. May be in this case it is confusing - it is just because this test tests strange thing. With dependency injection I can imagine tests like this:

def "test cache subsystem"(
  UserService userService,
  UserCache userCache, List<User> cachedUsers,
  PermissionCache permissionCache, Map<User, Permission> cachedPermissions,
  int requestedUserId,
  UserData expectedUser,
){
  setup:
    userCache.putAll(cachedUsers)
    permissionCache.putAll(cachedPermissions)
  when:
    UserData user = userService.getUserData(requestedUserId)
  then:
    expectedUser == user
  where:
    requestedUserId | cachedUsers | cachedPermissions | expectedUser
    123             |  []         | []                | null 
}

Fuud avatar Jan 24 '16 14:01 Fuud

Sorry, I really don't know what the advantage of

byte[] encrypted, byte[] key, byte[] expectedDecrypted

vs

expectedDecrypted | key | encrypted

is. Why not order them the same way?

My PR forbids optional parameters

Then it conflicts with my PR

With dependency injection

DI would still work with a more consistent parameter order also (i.e. the same as its usage)

l0rinc avatar Jan 24 '16 15:01 l0rinc

My PR forbids optional parameters

Then it conflicts with my PR

Yes. But if you want default values for parameters, you can write interceptor that provide null for all undefined parameters.

Regarding your usecase, you can move declarations to method body from parameter list:

Message message
when:   message = 'new'
then:   ...
when:   message = 'update'
then:   ...
when:   message = 'delete'
then:   ...
when:   message = 'update'
then:   exception

Fuud avatar Jan 24 '16 15:01 Fuud