Using a single aggregator on the method level

[akruijff]

The problem

When not all parameters consume the same number of arguments, then it is hard to consume the right arguments.

I have discusses the problem here: #2777

I currently do this with a static variable and a call from @BeforeEach setup method to reset method, but I do not like this solution.

Example

@ParameterizedTest
@CsvSource({
    "1, 2, 3, 4, 5"
})
public void test1(@AggregateWith(XA.class) X x, @AggregateWith(YA.class) Y y) {}
public void test2(@AggregateWith(YA.class) Y y, @AggregateWith(YA.class) X x) {}

public class XA implements ArgumentsAggregator {
    public Object aggregateArguments(ArgumentsAccessor accessor, ParameterContext context)
            throws ArgumentsAggregationException {
        int index = context.getIndex();
        return new Y(accessor.getInt(index + 0), accessor.getInt(index + 1));
    }
}
public class YA implements ArgumentsAggregator {
    public Object aggregateArguments(ArgumentsAccessor accessor, ParameterContext context)
            throws ArgumentsAggregationException {
        int index = context.getIndex();
        return new Y(accessor.getInt(index + 0), accessor.getInt(index + 1), accessor.getInt(index + 2));
    }
}

Solution

I would like the option to use @AggregateWith on the method level. The aggregator that is referred to should be instanced once per method call and called for every parameter. This way, the consumed arguments can be tracked.

Example

@ParameterizedTest
@CsvSource({
    "1, 2, 3, 4, 5"
})
@AggregateWith(MyAggregator .class)
public void test1(X x, y y) {}
public void test2(Y y, X x) {}

public class MyAggregator  implements ArgumentsAggregator {
    private int offset = -1;
    @Override
    public Object aggregateArguments(ArgumentsAccessor accessor, ParameterContext context)
            throws ArgumentsAggregationException {
        Class<?> type = context.getParameter().getType();
        if (type.equals(X.class))
            return new X(accessor.getInt(++offset), accessor.getInt(++offset));
        if (type.equals(Y.class))
            return new Y(accessor.getInt(++offset), accessor.getInt(++offset), accessor.getInt(++offset));
        throw new UnsupportedOperationException();
    }
}

Future improvement

It might also be nice to use a default aggregator and register convertor classes with it. I think this will lead to more reuse being possible

Example

@ParameterizedTest
@CsvSource({
    "1, 2, 3, 4, 5"
})
@ConvertWith(XC .class)
@ConvertWith(YC .class)
public void test1(X x, y y) {}
public void test2(Y y, X x) {}

public class DefaultAggregator  implements ArgumentsAggregator {
    private int offset = -1;
    private Map<Class<?>, SomeFunction> map;

    public void register(Class<?> type, SomeFunction func> { map.put(type, func); }

    public Object aggregateArguments(ArgumentsAccessor accessor, ParameterContext context) {
        Class<?> type = context.getParameter().getType();
        if (map.contains(type)) {
            SomeFunction func = map.get(type);
            Object obj = func.function(offset, accessor, context);
            offset += func.consumes();
            return obj;
        }
        throw new UnsupportedOperationException();
    }
}

public class XC implements SomeFunction {
    public Object function(int offset, ArgumentsAccessor accessor, ParameterContext context) {
        return new X(accessor.getInt(++offset), accessor.getInt(++offset));
    }
    public int consumes() { return 2; }
}

public class YC implements SomeFunction {
    public Object function(int offset, ArgumentsAccessor accessor, ParameterContext context) {
        return new Y(accessor.getInt(++offset), accessor.getInt(++offset), accessor.getInt(++offset));
    }
    public int consumes() { return 3; }
}

[marcphilipp]

@akruijff Thanks for raising the issue! ArgumentsAggregator are currently instantiated once per test method hence the proposed solution wouldn't work since index would be out of bounds after the first invocation. Moreover, supporting different ways of using ArgumentsAggregator would be confusing.

Instead, I think we could add a startIndex attribute to the @AggregateWith annotation or add support for using custom annotations in ArgumentsAggregator implementations by adding support for AnnotationConsumer.

If we decide for the former, ArgumentsAccessor would expose a sublist of arguments if startIndex > 0 so you could write your example as:

@ParameterizedTest
@CsvSource({
    "1, 2, 3, 4, 5"
})
public void test1(@AggregateWith(XA.class) X x, @AggregateWith(value = YA.class, startIndex = 2) Y y) {}

public class XA implements ArgumentsAggregator {
    public Object aggregateArguments(ArgumentsAccessor accessor, ParameterContext context)
            throws ArgumentsAggregationException {
        int index = context.getIndex();
        return new Y(accessor.getInt(0), accessor.getInt(1));
    }
}
public class YA implements ArgumentsAggregator {
    public Object aggregateArguments(ArgumentsAccessor accessor, ParameterContext context)
            throws ArgumentsAggregationException {
        int index = context.getIndex();
        return new Y(accessor.getInt(0), accessor.getInt(1), accessor.getInt(2));
    }
}

Would that work in your use case?

[akruijff]

@marcphilipp That solution seems very error prone, because it requires that, when the parameters change, that change is administered two places. This would deter me from using it.

Why do you think that my solution would be confusing?

The rule is: the more specific ArgumentsAggregator overrides the less specific ArgumentsAggregator i.e. when an ArgumentsAggregator is specified at both the method as the parameter level, then the ArgumentsAggregator at paramter level is used. To me, this feels intuitive. It is what I could expect. This works the same way for method in classes. The method in the more specific class is called.

My solution does require the ParameterContext to return the parameter count. With this, I could mod the index to fix the out of bounds problem.

@AggregateWith(A.class)
public void test1(@AggregateWith(XA.class) X x, @AggregateWith(value = YA.class) Y y) {}

I read this as A is never used, because a ArgumentsAggregator is specified for both x and y.

[marcphilipp]

Thanks for sharing your perspective! We'll discuss this in the team.

[marcphilipp]

Team decision: We think this is a corner case and adding support for method-level aggregators would add considerable complexity to the codebase. We're open to reconsider this decision in case there's more interest from the community.