Implemented partial parameter definitions

[l0rinc]

Until now the parameters either included all the data table header elements or none.

Their order wasn't enforced, but was replaced with a method call, based on the header,
causing parameter values to be mixed up.

This is changed now to variable number parameter definition, where only the order is enforced:

• first you can define the type of any variable from the data table, followed by arbitrary
  number of extra parameters (called with null, no default values supported yet)
  • the data table header order is compile-time checked
  • the extra parameters can serve as type-safety declarations outside the method body
  • provided default parameter values are compile-time rejected (not supported yet)
  • primitives are auto-wrapped to nullable types (they need default values)

---

This change is 

[leonard84]

please don't change the ordering of the imports and also avoid whitespace changes, where nothing really happens, this makes this commit just harder to read.

[l0rinc]

The imports are done by Idea automatically.

The end of line trimming is an .editorconfig setting, provided with the project: trim_trailing_whitespace = true

Would it be possible to configure these setting the way you would like to receive them, and reformat the whole codebase with it (and commit the config also).

It shouldn't take more than an hour, and would save all these reviews about formatting.

[leonard84]

Unfortunately .editorconfig is too limited to really cover the whole spock formatting and not all code really has the correct one, however if you are using IntelliJ you can configure the formatting to only apply to your changed code.

[l0rinc]

I would gladly unify the styles and provide a config for Idea and .editorconfig and reformat the whole codebase to avoid this confusion (so that new people have the correct settings from the start - it's not ok that everyone has to configure the styles on their own, and it's also not ok that the code is not uniform, and it's also not ok that I have to manually revert the imports and the trailing spaces every time I modify something)

[leonard84]

I don't quite understand the use of this, why would I want to declare extra parameters in the method when they are not passed? IMHO most method declarations are too large anyway so I think its a good to declare local variables in given/setup blocks.

I could see however the use to declare less parameters than, e.g. when you are using one for the description. I always disliked that I had to include it in the parameter definitions although I only used it in the @Unroll String.

Here a contrived example:

@Unroll("a is #resultDescription")
def "a even or uneven"(int a, boolean uneven) {
expect: 
((a & 1) == 1) == uneven
where:
a || uneven
1 || true
2 || false

resultDescription = uneven ? 'uneven' : 'even'
}

[robfletcher]

What's the impact on ordering of having assignments in addition to a data table? Is it clear what the expected order is in that case?

I personally haven't ever used parameters in this way. I share the confusion around the point of being able to declare arbitrary extra parameters that will be null.

[l0rinc]

The additional parameters are strictly at the end of existing data params (there's a test for that).

Sometimes you have to reassign a variable because you're testing it in different states, i.e.

when:   a = [1,2,3]
then:    ...

when:   a = [4,5,6]
then:    ...

If you can define a in the parameter list, the two whens can be symmetric (i.e. no def a in the first case; you can delete one, without the other).

Also, sometimes the def won't be enough, you want to declare the concrete type, which can reduce readability (i.e. lower S/R) - unless we hide it in the param list, which is not meant to be read in these cases anyway.

The given/when blocks are for assigning values, not necessarily for declaring types - but the param list IS.

[leonard84]

The given/when blocks are for assigning values, not necessarily for declaring types - but the param list IS.

I disagree, some code style checkers even consider reassigning parameters a code smell. The only reason I leave the final modifier from the parameter declarations in most of my code is that it generates too much noise. IMHO the given/setup block is the right place to define additional values.

[l0rinc]

some code style checkers even consider reassigning parameters a code smell

I consider it a smell too: the production code is probably too stateful. But I think the testing framework should be flexible enough to accommodate to that.

We used Spock for testing our message sending framework and some of the tests looked something like:

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

i.e. we reused the variable, because it plays the same role: it's the message that we're sending, but only the first one is defined.

If however there is a consensus that params should only be used for data tables, I will remove it from my PR :)

[l0rinc]

Please advise on what I should change for this to be merged!

[Fuud]

1. Why we reject invalid parameter order instead of map them by name?

2. With big data tables we always adds additional parameter 'n' equeal to data-row num. This parameter is used only in test name. May be it will be better if we can drop it from parameter list.

3. With Data table cells can reference previous ones #526 this PR became "must have".

[Fuud]

4. I also want an ability to omit some parameters from data table and set them in interceptor/extension (in runtime). It will be helpful in case of some complicated creation logic. For example: in our integration tests we have up to ten components connected to each other. Before the test verification starts all components should became up, running and all connections should be established. Copy from one of test:

    @JvmCount(4)
    @Unroll
    def "test #n process SupervisedGroupMembers (#comment) from=#before to=#after"(int n, SupervisedGroupMembers[] before, SupervisedGroupMembers[] after, String comment) {
        def rabbitMqFuture = base.startRabbitMq()
        def cnsFuture = base.startCns()
        def csgFuture = base.startOneLocationCsg()
        def adtFuture = base.startAdt()

        base.waitForAll()

        def adt = adtFuture.get()
        rabbitMqFuture.get()
        cnsFuture.get()
        def csg = csgFuture.get()

  .... // main test logic starts here

Such preamble (with different number of components) included in all tests. It will be better to reduce this code to

    @Unroll
    def "test #n process SupervisedGroupMembers (#comment) from=#before to=#after"(RabbitMq rabbitMq, Cns cns, Csg csg, Adt adt, SupervisedGroupMembers[] before, SupervisedGroupMembers[] after, String comment) {
  .... // main test logic starts here

[l0rinc]

Hi @Fuud,

1. Why reject invalid parameter order

Mostly because it was buggy until now (it ignored the order and tried to call it anyway), and because it's counterintuitive (e.g. in your example you wouldn't want to confuse injected parameters with data providers).

2. we can drop it from parameter list

In the current PR data providers are optional already (only the order is enforced).

3. this PR became "must have"

+1.

4. I also want to omit parameters [...] and set them in runtime

I suppose you mean some kind of dependency injector (Spring/Guice?).
I don't know if that's possible or not with Spock (until now it couldn't have additional params, so I guess not), but I think it's a good idea (could even be combined with default parameters, instead of nulling them out).

@leonard84, @robfletcher, opinions?

[Fuud]

Please take a look at Fuud@16739b3

I solved this problem with resolving parameters by name. It is just a prototype because derived parametrization was broken. But IMHO it is better way to implement this. And interceptors can change IterationInfo.dataValues providing additional parameters.

[l0rinc]

@Fuud, I still object to making the parameter orders permutable, for the above reasons.
However, it seems to be independent of the current PR.

[Fuud]

Please take a look at #566

[l0rinc]

Hey @leonard84, would it make sense for me to rebase this PR or should we close it?

[leonard84]

Hey @paplorinc this feature is now already in Spock 2.0 so I'll close this PR