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

Allow additional parameters in feature methods #566

Conversation

Fuud
Copy link
Contributor

@Fuud Fuud commented Jan 22, 2016

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

@@ -16,8 +16,10 @@

package org.spockframework.compiler;

import java.io.PrintWriter;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is PrintWriter used in the current version of your PR?

@szpak
Copy link
Member

szpak commented Jan 22, 2016

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.

@l0rinc
Copy link
Contributor

l0rinc commented Jan 23, 2016

As I detailed my arguments in #525 (comment) 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.

@Fuud Fuud force-pushed the additional_parameters__over_spock_master branch from 50dd091 to 475b564 Compare January 24, 2016 11:28
@Fuud
Copy link
Contributor Author

Fuud commented Jan 24, 2016

@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.

@l0rinc
Copy link
Contributor

l0rinc commented Jan 24, 2016

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

@Fuud
Copy link
Contributor Author

Fuud commented Jan 24, 2016

@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.

@l0rinc
Copy link
Contributor

l0rinc commented Jan 24, 2016

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?)

@Fuud
Copy link
Contributor Author

Fuud commented Jan 24, 2016

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 
}

@l0rinc
Copy link
Contributor

l0rinc commented Jan 24, 2016

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)

@Fuud
Copy link
Contributor Author

Fuud commented Jan 24, 2016

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

@leonard84 leonard84 closed this Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants