Junit5: Support aggregation of arguments into a single object for parameterized tests

[klassounski-daitan]

Overview

Hello there Java testers.

I started working with JUnit 5 a few days ago because I loved the new way of creating parameterized tests.

There's @ParameterizedTest that allows a test to run in a parameterized fashion and @CsvFileSource that loads the parameters from a CSV file.

The thing is that I have too many columns in my CSV and don't want to have a huge method signature in my unit test. Let me give you an example:

    @ParameterizedTest
    @CsvFileSource(resources = "/person-data.csv")
    void myTest(String p1, String p2, String p3, String p4, String p5, String p6) {
        // test using parameters
    }

It would be awesome to have some kind of annotation that would allow us to do something like this:

    @ParameterizedTest
    @CsvFileSource(resources = "/person-data.csv")
    void myTest(@ConvertWith(PersonConverter.class) Person person) {
        // test using parameters
    }

    static class PersonConverter implements AggregatingConverter {
        public Person convert(Object ... params){
           // a simple method that creates the Person object and inserts the params in it
        }
    }

I believe that this improvement would be much welcomed by our community.

Happy coding and stay awesome!

Deliverables

• [x] Decide what semantics are supported in terms of mixing indexed arguments, aggregators, and arguments resolved by other ParameterResolver implementations (e.g., TestInfo).
• [x] Introduce ArgumentsAccessor API.
• [x] Introduce ArgumentsAggregator API along with @AggregateWith annotation.
  
  
  
  • [x] Allow @AggregateWith to be used as a meta-annotation.
  
  
  • [x] Ensure that additional arguments (e.g., TestInfo) can be supplied to a parameterized test method that makes use of these aggregation features.
  
  
• ❌ Consider introducing a setIndexOffset(int) method in ArgumentsAccessor that makes it easier to reuse existing position-based logic for input sets that are occasionally shifted to the right by a given _offset_ (i.e., n indexes).
• [x] Document in Release Notes.
  
  
  
  • [x] Documentation exists but can be improved -- for example, cross referencing sections from the User Guide.
  
  
• [x] Document in User Guide.
  
  
  
  • [x] Documentation exists but can be improved.
  
  

[sbrannen]

Assigned to 5.1 backlog for team discussion.

[sbrannen]

FYI: I updated the title of this issue to reflect its more generic nature.

[hglasgow]

I like the idea of being able to encapsulate row fields. Pull request #1290 is an attempt at implementing this issue, with unit tests and documentation updates included. If this is an acceptable approach, I would be happy to work through any suggestions for improvement.

[sbrannen]

Hi @hglasgow,

Thanks for submitting PR #1290.

Unfortunately that is not the direction we would want to go.

If we do anything along these lines, it would have to be a general-purpose ArgumentConverter that can be applied to the arguments for any @ParameterizedTest. In other words, we would not limit the support to just @CsvFileSource. Similarly, we would not bake the support into a single ArgumentsProvider implementation.

For example, I could imagine introducing a sub-interface of ArgumentConverter (perhaps something like an AggregatingArgumentConverter) that is handled consistently across all ArgumentsProvider implementations (i.e., something more along the lines of what @klassounski-daitan proposed in this issue's description).

Regards,

Sam

[geo-m]

Greetings @sbrannen

Is this request still being considered? I could also use this feature and would like to contribute to the project. The implementation i would go for would be an ArgumentConverter as you mentioned.

[deleted old approach] (see edit)

EDIT: After some source code digging i understand that by default each converter is called only once, for a specific parameter only, without knowledge of the total argument context. In which case i believe the extension also needs to modify ParameterizedTestParameterResolver.java. , and is not possible with a clean ArgumentConverter implementation. Please correct me if i missed anything.

[sbrannen]

Is this request still being considered? I could also use this feature and would like to contribute to the project. The implementation i would go for would be an ArgumentConverter as you mentioned.

Yes, we are still considering introducing such a converter in the 5.2.x time frame.

EDIT: After some source code digging i understand that by default each converter is called only once, for a specific parameter only, without knowledge of the total argument context.

Yes, that adequately describes the status quo.

In which case i believe the extension also needs to modify ParameterizedTestParameterResolver.java. ,

Yes, ParameterizedTestParameterResolver would have to be modified to support both the existing ArgumentConverter API and the new AggregatingArgumentConverter API, depending on the concrete type of converter configured via @ConvertWith.

and is not possible with a clean ArgumentConverter implementation.

I'm not quite sure what you mean by "clean". However, this can definitely not be achieved via the existing ArgumentConverter API alone, since an AggregatingArgumentConverter would have to convert _multiple_ objects into a single object.

Based on this discussion, hopefully it'll be relatively straightforward to put together the PR. 😉

Have fun!

p.s. if you don't have time to put together a PR, please let us know so that somebody else can have a go at it (e.g., yours truly).

[sbrannen]

In response to the proposal in #1325, I'd like to outline what I think we should be achieving in terms of the programming model for end users, setting aside implementation details for the moment.

I envision a new AggregatingArgumentConverter API similar to the following.

public interface AggregatingArgumentConverter {

    Object convert(Object[] sourceArguments, ParameterContext context)
            throws ArgumentConversionException;

}

As for whether or not AggregatingArgumentConverter should extend ArgumentConverter remains up for discussion. Having it as a sub-interface makes it considerably easier to register such a converter via @ConvertWith; however, if it were to extend ArgumentConverter, it would then need to provide a default implementation of ArgumentConverter.convert(Object, ParameterContext) that throws an UnsupportedOperationException.

A user could then implement such a converter to aggregate multiple arguments into a single Person object -- for example, from a single line in a CSV file.

That might look something like this:

public class PersonArgumentConverter implements AggregatingArgumentConverter {

    @Override
    public Object convert(Object[] sourceArguments, ParameterContext context) {
        return new Person(
            String.valueOf(sourceArguments[0]),
            String.valueOf(sourceArguments[1])
        );
    }

}

And could then be used as follows:

@ParameterizedTest
@CsvSource({ "Jane, Smith", "John, Smith" })
void aggregatingTest(@ConvertWith(PersonArgumentConverter.class) Person person) {
    assertTrue(person.getFirstName().startsWith("J");
    assertEquals("Smith", person.getLastName());
}

[sbrannen]

@klassounski-daitan, @hglasgow, @geo-m, and @junit-team/junit-lambda,

What do you all think about my above proposal?

[marcphilipp]

It would be more flexible if PersonArgumentConverter wouldn't have to hard-code the indexes. For example, there might be multiple CSV files. In one the first two columns might be first and last name, in another it might be the fifth and sixth etc.

How about a new annotation to register an AggregatingArgumentConverter (or ArgumentAggregator?), e.g. @AggregateWith(aggregator = PersonAggregator.class, indexes = {4, 5})?

[sbrannen]

To create an aggregating converter that converts multiple arguments into a collection, we would could consider providing some built-in implementations, but even if we don't... the implementations are rather straightforward.

For example, an aggregating converter for a List target type:

public class ListArgumentConverter implements AggregatingArgumentConverter {

    @Override
    public Object convert(Object[] sourceArguments, ParameterContext context) {
        return Arrays.asList(sourceArguments);
    }

}

[sbrannen]

@marcphilipp, I was also pondering what to do about the indices... but I intentionally left out that detail until we got a bit further in the design discussions.

But... since you've brought it up.... 😉

Yes, I do think we will need to provide a means for an aggregating converter to convert a specific number of arguments, potentially by index or range.

[sbrannen]

Actually, I can foresee the need for multiple aggregating converters for a single parameterized test method. For example, if a CSV line contains information for a Person and an Address (and the Person object does not _own_ the Address), one could imagine wanting to write a test like this:

@ParameterizedTest
@CsvSource({ "Jane, Smith, 1010 Hotdog Street, Anyville" })
void aggregatingTest(
        @ConvertWith(aggregator = PersonArgumentConverter.class, indices = {1, 2}) Person person,
        @ConvertWith(aggregator = AddressArgumentConverter.class, indices = {3, 4}) Address address) {

    // assertions ...
}

[sbrannen]

Regarding AggregatingArgumentConverter vs. ArgumentAggregator, the latter might in fact be a better choice since the primary role is aggregation instead of conversion.

Plus having a different name logically results in a different annotation for registering it and avoids the aforementioned issues with trying to treat it as a sub-interface of ArgumentConverter.

In other words, I'm basically agreeing with your suggestions:

• ArgumentAggregator interface
• @AggregateWith annotation

[sbrannen]

Regarding indices vs indexes, that's a whole other can of worms. 😝

[geo-m]

@sbrannen

For example, an aggregating converter for a List target type:

The reason the example collection implementation i provided was a bit more verbose was the impression that the user should choose the list implementation he wants or use custom collection types, for example provide a set for unique arguments, or a treemap for sorting.

Regarding the indices, is there anything inherently different in an Aggregator apart from access to all arguments at once by the resolver, similar to the TestData annotation proposal? Asking out of genuine curiosity due to inexperience.

[sbrannen]

The reason the example collection implementation i provided was a bit more verbose was the impression that the user should choose the list implementation he wants or use custom collection types, for example provide a set for unique arguments, or a treemap for sorting.

Yep, I understood what you were doing.

We could potentially consider providing a built-in aggregator that explicitly supports aggregation via collection target types, but we should get the rest of the programming model right before adding on.

[sbrannen]

Regarding the indices, is there anything inherently different in an Aggregator apart from access to all arguments at once by the resolver, similar to the TestData annotation proposal? Asking out of genuine curiosity due to inexperience.

Well, as I mentioned in my Person/Address example, a user may wish to have more than one object aggregated from various elements of the source arguments array.

And as Marc pointed out, the order of the arguments may change based on the source used.

[sbrannen]

Tentatively slated for 5.2 M1

[sormuras]

Mh, the initial request escalated quickly. ;) From simple to over complicated?

I like this more:

@ParameterizedTest
@CsvSource({ "Jane, Smith, 1010 Hotdog Street, Anyville" })
void aggregatingTest(String... values) {
    // aggregate data ...
    var person = Person.of(values[0], values[1]);
    var address = Address.of(values[2], values[3]);

    // assertions ...
}

[sbrannen]

Mh, the initial request escalated quickly.

I don't see it like that.

Any such feature should be easy to use for simple use cases but still _usable_ for more advanced use cases (without having to introducing breaking changes in the published API at a later date). So, based on my experience in API design for OSS, if we know (or have a very strong hunch) that people will request the advanced features later, it's generally a good idea to include them in the initial design.

FWIW, with the current proposal, the initial request would go from this:

@ParameterizedTest
@CsvFileSource(resources = "/test-data.csv")
void myTest(@ConvertWith(TestDataConverter.class) TestData testData) {
    // test using parameters
}

... to:

@ParameterizedTest
@CsvFileSource(resources = "/test-data.csv")
void myTest(@AggregateWith(TestDataAggregator.class) TestData testData) {
    // test using parameters
}

The only difference for that use case are the names of the annotation and the implementation. 😉

[sbrannen]

I like this more:

@ParameterizedTest
@CsvSource({ "Jane, Smith, 1010 Hotdog Street, Anyville" })
void aggregatingTest(String... values) {
    // aggregate data ...
    var person = Person.of(values[0], values[1]);
    var address = Address.of(values[2], values[3]);

    // assertions ...
}

We could of course consider supporting something like that as well, but I don't know if we'd want to support auto-conversion to array component types other than Object.

Also, although I certainly appreciate the simplicity in the convention there, I'm not so sure it's really better from a user's perspective in terms of having to pull out the values from the array (and potentially converting them) vs. having pre-converted values passed in to explicit, type-safe arguments.

I'll have to ponder it a bit.

[bechte]

Actually, I am in favor of @sormuras' suggestion, because of two reasons:

1. I am not a big fan of "complex" annotations on parameters, because they tend to become a nightmare to read, especially on auto-formatted project code, as code formatters have troubles in setting thing the nice and smooth readable way as done manually by @sbrannen in the example above.
2. I share the concerns about complexity with @sormuras and don't see any advantages in having the aggregation done in the parameter line over having it in a nice way put at the very beginning of the test code.

Still, I do not agree in one point of the approach:

I like this more:

@ParameterizedTest
@CsvSource({ "Jane, Smith, 1010 Hotdog Street, Anyville" })
void aggregatingTest(String... values) {
    // aggregate data ...
    var person = Person.of(values[0], values[1]);
    var address = Address.of(values[2], values[3]);

    // assertions ...
}

While it clearly has advantages in point of readability as mentioned above, I am very unhappy in having to "pollute" the domain objects (i.e. Person and Address here). In some cases those will be part of the test data domain and this would be an okay solution, but I think we should also support to fill real data domain objects as in the example (I doubt Person and Address being only test data domain objects).

Having said that, I would add another ingredient to the approach of @sormuras to bring together the pros of both worlds, namely adding a smooth DSL for doing the exact magic the annotations would have done in the approach provided by @sbrannen:

import static org.junit.jupiter.params.converter.Converters.forValues;

@ParameterizedTest
@CsvSource({ "Jane, Smith, 1010 Hotdog Street, Anyville" })
void aggregatingTest(Object... values) {
    // aggregate data ...
    Person person = forValues(values).filterIndices(1, 2).convertWith(PersonArgumentConverter.class);
    Address address = forValues(values).filterIndices(3, 4).convertWith(AddressArgumentConverter.class);

    // assertions ...
}

This way we could have both, the flexibility of the the initial approach that allows extensibility in the future and a nice-to-read DSL that will not be split and tangled up by the code formatter. And (at least to me) it is more natural to use (in a Java 8+ way of doing things).

What do you guys think?

[sormuras]

@bechte Thanks for the detailed answer and you explained my concerns with the annotation-based program logic very well.

The simple factory methods I used, Person.of and Address.of, are just examples. If such constructors or factory methods don't exist in ones domain model, I'd resort to test-local factories, like:

static Person createPerson(Object preName, Object familyName) { return ... }

Usage in the aggregatingTest(Object... values) method:

Person person =  createPerson(values[0], values[1]);

The domain objects are not polluted and you may re-use that factory in your tests. No need for the JUnit framework to create and maintain a DSL.

[bechte]

The domain objects are not polluted and you may re-use that factory in your tests. No need for the JUnit framework to create and maintain a DSL.

@sormuras I'm with you, that this would be the trivial solution. At a first glimpse, the DSL idea also felt a bit wrong to me. But the longer I was thinking about the benefits of @sbrannen's approach, I think it is worth it. We could provide a very easy, yet powerful DSL, that leads to a reusable test pattern when dealing with parameterized tests:

• tests become easier to read
• tests follow a common (in the future: well-known) pattern
• people will clearly understand what is going on
• clear separation of concerns (domain vs. test data mapping)

Here is a draft for such an DSL, we might want to tweak it a bit more, but in the end it shouldn't need much more than this:

public final class Converters {
    public static Converters forValues(Object... values) {
        return new Converters(values != null ? Arrays.stream(convertToIndexed(values)) : Stream.empty());
    }

    public Converters filterIndices(Integer... indices) {
        List<Integer> indicesWhitelist = Arrays.asList(indices);
        return filterBy(item -> indicesWhitelist.contains(item.getIndex()));
    }

    public Converters filterBy(Predicate<Indexed<Object>> filter) {
        values = values.filter(filter);
        return this;
    }

    public <R> R convertsWith(Class<ArgumentConverter<R>> converterClass) {
        return converterClass.newInstance().convert(values.toArray());
    }

    // some hidden helper methods & the wrapper class Indexed
    // holding an index with a value in order to make filtering by index easier
}

[geo-m]

Would a combination of your suggestions be ok?

If a simple List or other collection as the default aggregator, with @Aggregate being just shorthand for @AggregateWith(value = <default>) for clarity purposes you could do either this

@CsvSource({"firstname, lastname"})
public void personFromDefaultAggregator(@Aggregate List data){
   Person p = new Person (data.get(0), data.get(1));
}

or this

@CsvSource({"firstname1, lastname1, firstname2, lastname2"})
public void personsFromPersonAggregator(
     @AggregateWith(value = PersonAggregator.class , range = {0, 1}) Person person1,
     @AggregateWith(value = PersonAggregator.class , range = {2, 3}) Person person2
) {
    // assertions
}

or even specific indexes

@CsvSource({"firstname1, commonlastname, firstname2"})
public void personsFromPersonAggregatorWithIndex(
     @AggregateWith(value = PersonAggregator.class , indexes = {0, 1}) Person person1,
     @AggregateWith(value = PersonAggregator.class , indexes = {2, 1}) Person person2
) {
    // assertions
}

or, for ease of use, completely ignore the index parameters if you expect the converter to use all of them

@CsvSource({"firstname","lastname"})
public void shortHandPersonAggregation(
     @AggregateWith(PersonAggregator.class) Person person1
) {
    // assertions
}

Sounds easy to me as a novice user at least

[sbrannen]

Would a combination of your suggestions be ok?

Please hold off on implementing anything concrete for the time being.

The @junit-team/junit-lambda is still actively discussing where we want to take this feature.

We will be providing additional input here, and we would also appreciate input from the rest of you!

Speaking of which, @geo-m, thanks for providing the examples. Those will help guide the discussion.

[sbrannen]

@klassounski-daitan, I took the liberty to change the examples you provided to refer to Person objects instead of TestData so that the discussion here can focus on more concrete use cases.

I hope that's OK with you, and if my changes somehow affect your original intention, please let me know.

[sbrannen]

Actually, I can foresee the need for multiple aggregating converters for a single parameterized test method. For example, if a CSV line contains information for a Person and an Address (and the Person object does not _own_ the Address), one could imagine wanting to write a test like this:

Replying to myself here...

After having put a bit more thought into, I actually don't think it would be very common to parse multiple aggregated objects from a single set of inputs.

In fact, when I think about existing "mapping" frameworks out there, I can't really think of one that would support such a use case. For example, Spring Batch supports mapping a single CSV line onto a single domain object. Similarly, the uniVocity CSV parsing library used behind the scenes in Jupiter's parameterized test support also has a mapping API for mapping single rows onto single objects.

In summary, I think it should suffice to be able to aggregate all available arguments into a _single_ object.

[sbrannen]

I share the concerns about complexity with @sormuras and don't see any advantages in having the aggregation done in the parameter line over having it in a nice way put at the very beginning of the test code.

@bechte, one of the key benefits of being able to externalize the aggregation/conversion is that doing so makes the implementation reusable across multiple test methods or test classes, analogous to implementing one of Jupiter's Extension APIs and being able to reuse the extension.

[sbrannen]

@bechte, the notion of introducing a DSL for extracting and converting is definitely worth investigating further.

@marcphilipp also proposed a similar idea that would allow the existing auto-conversion infrastructure to be reused by developers, but I'll let him chime in with the details of his idea later. 😉

[bechte]

@bechte, one of the key benefits of being able to externalize the aggregation/conversion is that doing so makes the implementation reusable across multiple test methods or test classes, analogous to implementing one of Jupiter's Extension APIs and being able to reuse the extension.

@sbrannen, yes I agree with that. Was I was trying to say is, that to me it doesn’t matter if an externalized way to aggregate / convert those lines is placed in the parameter line by annotate the parameters or in the first lines of the method as an clean DSL. (While I prefer the latter ;-))

[marcphilipp]

Here's an idea that lets one reuse the existing auto-conversion infrastructure:

@ParameterizedTest
@CsvSource({ "Jane, Smith, 1010, Hotdog Street, Anyville" })
void aggregatingTest(ArgumentsAccessor arguments) {
    // aggregate data ...
    var person = new Person(arguments.getString(0), arguments.getString(1));
    var address = new Address(arguments.getInt(2), arguments.getString(3), arguments.getString(4));

    // assertions ...
}

In addition, there would be a get(Class<?>, int) method.

We could combine this with @bechte's DSL idea:

@ParameterizedTest
@CsvSource({ "Jane, Smith, 1010, Hotdog Street, Anyville" })
void aggregatingTest(ArgumentsAccessor arguments) {
    // aggregate data ...
    var person = arguments.filterIndices(0, 1).convertWith(new PersonArgumentsConverter());
    var address = arguments.filterIndices(2, 3, 4).convertWith(new AddressArgumentsConverter());

    // assertions ...
}

Each ArgumentsConverter would get an ArgumentsAccessor instance so it could also reuse the existing auto-conversion infrastructure.

[bechte]

@marcphilipp, cool. Like the idea.

[sbrannen]

Yep, I agree.

Great teamwork here..... I really like how the programming model is taking shape here! 👏

[AJNeufeld]

Is there any check to see if all the indices are used? How about if any indices are used more than once? If not, I fail to see how this "aggregation" is any different from "conversion". But instead of converting from simple strings, you have two ArgumentConverters, both which are converting from an ArgumentsAccessor object, ... one converting into a Person object, the other into an Address object.

Instead, why not add indices as an optional parameter to @ConvertWith, and when present creates a List<?> containing the elements to pass to the converter, and when absent just passes the next argument to the converter:

@ParameterizedTest
@CsvSource({ "1, Jane, Smith, false, 1010, Hotdog Street, Anyville" })
void myTest(
        int id, 
        @ConvertWith( PersonArgumentConverter.class, indices={1, 2} ) Person person,
        boolean married,
        @ConvertWith( AddressArgumentConverter.class, indices={4, 5, 6} ) Address address) {

    // assertions ...
}

Implicit conversion is used for the first argument, and third argument, and explicit converters for the second and fourth. Because index=2 was the last used index in the conversion to person, the married parameter would be converted from index=3.

Instead of indices, a number of arguments to consume could be given.

@ParameterizedTest
@CsvSource({ "1, Jane, Smith, false, 1010, Hotdog Street, Anyville" })
void myTest(
        int id, 
        @ConvertWith( PersonArgumentConverter.class, arguments=2) Person person,
        boolean married,
        @ConvertWith( AddressArgumentConverter.class, arguments=3) Address address) {

    // assertions ...
}

[bechte]

@AJNeufeld, thanks for sharing your insights.

Is there any check to see if all the indices are used? How about if any indices are used more than once?

Currently, this is not planned as far as I understood. We could probably add a verification method to the ArgumentsAccessor that can be called within the test, but I wouldn't expect that this verification needs to be done with every kind of test data. Often there is a field or two that are not necessary, or fields that might be used several times.

And yes, we might call it conversion, that would be absolutely fine with me.

Instead of indices, a number of arguments to consume could be given.

I doubt that this is a good idea. Test data in form of CSV, especially when provided by another system, does not always follow the perfect structure, i.e. partitioning etc. Therefore, I think the solution should be more flexible and I can foresee specifying the number of arguments causes troubles in reading of the code, because one would have to count themselves to know which column is picked.

@AJNeufeld, did you also review the current proposal by @marcphilipp three posts before yours? How do you think about that one?

[sormuras]

I like the ArgumentsAccessor proposal.

It could/should provide a read-only access to the raw String values, too. Something like:

@ParameterizedTest
@CsvSource({ "Jane, Smith, 1010 Hotdog Street, Anyville" })
void aggregatingTest(ArgumentsAccessor arguments) {
    // aggregate data ...
    var values = arguments.toStringArray();
    var person = Person.of(values[0], values[1]);
    var address = Address.of(values[2], Utils.stringToZip(values[3]));

    // assertions ...
}

[bechte]

@sormuras, with that in place, the test writer himself can decide which direction to go. I think that is valid and should be one of our targets. So, fine with me.

[AJNeufeld]

@bechte Yes, I did review the proposal by @marcphilipp and I both liked it and disliked it. What I disliked was the "ceremony" around passing multiple arguments to one converter:

var person = arguments.filterIndices(0, 1).convertWith(new PersonArgumentsConverter());

I immediately thought, "well, I could make a function that takes the converter and indices and produces the desired output with less typing on my part":

var person = convert( arguments, new PersonArgumentsConverter(), 0, 1);

I understand the desire to use a fluent DSL to process the arguments into the desired type. It is flexible and extendable. But ... is this really something that will be extended? Does it need to be flexible? We are passing n fields, representing m arguments to a test method. The specific converters are going to do the customizing of the conversion, but the plumbing to send the correct arguments to the specific converter shouldn't need much customization or flexibility.

Consider the existing ArgumentConverter:

@ParameterizedTest
@CsvSource({ "Jane Smith, '1010 Hotdog Street, Anyville'" })
void myTest(
        @ConvertWith( PersonArgumentConverter.class) Person person,
        @ConvertWith( AddressArgumentConverter.class) Address address) {
    // assertions ...
}

We have argument conversion with implicit 1-to-1 mapping. We want to expand this to implement n-to-1 mapping. Instead of converting one String into an Address, we want to convert List<String> into an Address. This is still one Object to one Object, and the ArgumentConverter interface seems like it still applies. Adding an optional indices parameter to @ConvertWith would allow 3 strings to be passed to the AddressArgumentConverter:

@ParameterizedTest
@CsvSource({ "Jane Smith, 1010, Hotdog Street, Anyville" })
void myTest(
        @ConvertWith( PersonArgumentConverter.class) Person person,
        @ConvertWith( AddressArgumentConverter.class, indices={1, 2, 3}) Address address) {
    // assertions ...
}

Equivalently, an optional arguments or fields or count could be used, with implicit index generation.

@ParameterizedTest
@CsvSource({ "Jane Smith, 1010, Hotdog Street, Anyville" })
void myTest(
        @ConvertWith( PersonArgumentConverter.class) Person person,
        @ConvertWith( AddressArgumentConverter.class, fields=3) Address address) {
    // assertions ...
}

Instead of indices, a number of arguments to consume could be given.

I doubt that this is a good idea. Test data in form of CSV, especially when provided by another system, does not always follow the perfect structure, i.e. partitioning etc. Therefore, I think the solution should be more flexible and I can foresee specifying the number of arguments causes troubles in reading of the code, because one would have to count themselves to know which column is picked.

I disagree. I think using a count may actually be simpler. My second last example, if I changed my test data from "Jane Smith, ..." to "Jane, Smith, ...", I'd have to specify either fields=2 or indices={0,1} to @ConvertWith(...), as well as change the converter implementation to handle the first and last names as separate arguments. The benefit comes from the Address parameters. With fields=3, nothing needs to change, but with indices={1,2,3}, it would have to be updated to indices={2,3,4}.

---

To summarize,

void aggregatingTest(ArgumentsAccessor arguments) {
    var person = arguments.filterIndices(0, 1).convertWith(new PersonArgumentsConverter());
    ...

is much more typing, and has an extra ArgumentAccessor type. Using the existing @ConvertWith(...) annotation, with an optional indices={...} or fields=# or range={#,#} parameter (all can be supported, but only one may be used at a time) is shorter:

void myTest(
    @ConvertWith( PersonArgumentConverter.class, fields=2) Person person
    ...

Using a fluent DSL is likely overkill.

[sbrannen]

It could/should provide a read-only access to the raw String values, too. Something like:

I was also thinking about providing direct access to the arguments as either an array or list:

List<Object> list = arguments.asList();
Object[] arr = arguments.asArray();

or:

List<Object> list = arguments.toList();
Object[] arr = arguments.toArray();

etc.

[marcphilipp]

@AJNeufeld Thanks for the feedback! We've discussed the issue in the team today and believe the ArgumentsAccessor approach might still be useful. However, we are going to postpone the decision on the fluent API.

The current ArgumentConverter API converts arguments one by one. In our opinion, we should not force users to downcast its Object parameter to List<Object>. In addition, it could then not reuse the existing conversion infrastructure. Instead, we've decided to go for a new ArgumentsAggregator interface that receives an ArgumentsAccessor. In addition, parameterized test methods can declare parameters of type ArgumentsAccessor.

---

Team decision:

• Implement ArgumentsAccessor with getInt(int), getString(int), get(Class, int) etc. and toList()/toArray() methods and resolve it for @ParameterizedTest methods
• Introduce ArgumentsAggregator interface with single method aggregate(ArgumentsAccessor, ParameterContext) : Object and call the aggregator to resolve the method parameter
• Introduce @AggregateWith(value: ArgumentsAggregator) annotation.

For the latter, we could ship a first version without indices, fields (offset?) or range parameter.

[sbrannen]

Correction: we actually agreed that the method signature would be aggregateArguments(ArgumentsAccessor, ParameterContext) -- right? 😉

[marcphilipp]

@sbrannen Good catch! I've changed it accordingly.

@AJNeufeld The ParameterContext describes the method parameter being resolved, e.g. a Person parameter with index 0 in the following example:

@ParameterizedTest
@CsvSource({ "Smith, Jane" })
void myTest(@AggregateWith(PersonAggregator.class) Person person) {
    // assertions ...
}

[marcphilipp]

@geo-m is working on a PR for this issue in #1325.

[sbrannen]

@AJNeufeld, I guess you deleted your comment that I just read via email notifications, since I don't see it here.

In any case, @marcphilipp apparently already answered you. 😉

[sbrannen]

The initial work on this issue has been pushed to master in commits e2295abd0069604852567a042acd5bc6fd3d576f and 3ec77993d46efe04cc5a3b62086bb3d5fd924e3d.

Thanks again for all of your hard work on these features, @geo-m!

[sbrannen]

FYI: this issue has not yet been closed in order to introduce additional tests for preconditions and error cases as well as to perform a final review of the new section in the User Guide.

[sbrannen]

Update: I have added a _Deliverables_ section to track tasks, lacking features, and bugs (e.g., inability to use TestInfo alongside ArgumentsAccessor).

[sbrannen]

Current error when including TestInfo or other values resolved by a ParameterResolver as arguments following an ArgumentsAccessor argument:

org.junit.jupiter.api.extension.ParameterResolutionException: Discovered multiple competing ParameterResolvers for parameter [org.junit.jupiter.api.TestInfo arg1] in executable [void org.junit.jupiter.params.aggregator.AggregatorTests.argumentsAccessorAsArgumentToMethod(org.junit.jupiter.params.aggregator.ArgumentsAccessor,org.junit.jupiter.api.TestInfo)]: org.junit.jupiter.engine.extension.TestInfoParameterResolver, org.junit.jupiter.params.ParameterizedTestParameterResolver
    at org.junit.jupiter.engine.execution.ExecutableInvoker.resolveParameter(ExecutableInvoker.java:201)
...

[sbrannen]

Update: I made changes to support the combination of _aggregators_ with TestInfo, etc.

AggregatorIntegrationTests demonstrates a subset of what's currently possible.

More fine tuning to come, along with documentation for what is actually supported.

[geo-m]

Should have seen the bug coming, caused by consuming all arguments... Now it's quite better but as you probably know the test doesn't pass if any other injectable like a TestInfo comes before any accessor or an aggregate value.

That might be easy to know if documented. Still, I was wondering what you would think about using the first accessor instead of the last as "terminating". Anything after that index is either an accessor / aggregator or needs another resolver.

[sbrannen]

as you probably know the test doesn't pass if any other injectable like a TestInfo comes before any accessor or an aggregate value.

Yep. We're aware of that.

It's essentially no different than how conventional parameterized tests (i.e., without aggregators) work. If you want the TestInfo, a Spring bean, a Mockito mock, etc., those parameters must come at the end of the formal parameter list.

[sbrannen]

That might be easy to know if documented. Still, I was wondering what you would think about using the first accessor instead of the last as "terminating".

Thanks for the suggestion. We are in fact already considering that.

Anything after that index is either an accessor / aggregator or needs another resolver.

That's certainly an option, but it's less _flexible_ than the current implementation.

However, we may decide that we do not wish to support such a "mixed mode" as is currently in master.

Once the team has made a decision, we will enforce, test, and document the semantics.

[sbrannen]

Should have seen the bug coming, caused by consuming all arguments...

No worries.

I'd say you did a great job with the PR! To the best of my knowledge, it only contained three bugs, and that's quite good for such a feature. 😉

To be honest, we should have requested additional tests before merging the PR. That would have helped discover bugs and find solutions while it was still a PR. So I partially take the blame for that.

[sbrannen]

Update

Here's what's up for discussion.

@ParameterizedTest(name = "Mixed Mode #1: {arguments}")
@CsvSource({
    "gh-11111111, Jane, Doe, 1980-04-16, F, 42 Peachtree Street, Atlanta, 30318, red",
    "gh-22222222, Jack, Smith, 2000-11-22, M, 99 Peachtree Road, Atlanta, 30318, blue"
})
void mixedMode1(String issueNumber, @CsvToPerson @StartIndex(1) Person person,
    @CsvToAddress @StartIndex(5) Address address, TestInfo testInfo) {

    // assertions
}

@ParameterizedTest(name = "Mixed Mode #2: {arguments}")
@CsvSource({
    "gh-11111111, Jane, Doe, 1980-04-16, F, 42 Peachtree Street, Atlanta, 30318, red",
    "gh-22222222, Jack, Smith, 2000-11-22, M, 99 Peachtree Road, Atlanta, 30318, blue"
})
void mixedMode2(String issueNumber, @CsvToPerson @StartIndex(1) Person person, TestInfo testInfo,
    @CsvToAddress @StartIndex(5) Address address) {

    // assertions
}

mixedMode1() currently passes against master (i.e., with "find last index of aggregator" semantics).

mixedMode2() currently _fails_ against master. To make it _pass_ , we would have to switch to "find first index of aggregator" semantics.

I am personally not convinced that putting something like TestInfo testInfo or @Autowired OrderService _before_ an argument aggregator is desirable.

Thoughts?

[sbrannen]

Food for thought:

As currently implemented in master, "find last index of aggregator" semantics also allow one to declare traditional parameters sourced from indexed arguments _after_ or _between_ aggregators.

That would look something like the following fictitious example, where issueNumber is declared between the person and address aggregators.

@ParameterizedTest
@CsvSource( /* ... */ )
void mixedModeX(@CsvToPerson Person person, String issueNumber,
    @CsvToAddress Address address, TestInfo testInfo) {

    // assertions
}

That would work with the current implementation in master, but... issueNumber would be pulled from index 1 in the provided arguments.

In other words, as soon as you start mixing the order of indexed arguments and aggregators, it gets ugly fast.

That's why in created @StartIndex for the tests.

[sbrannen]

In order to follow the _KISS_ principle, I'm starting to think we should perhaps just enforce the following strict rules for now.

1. 0 or more _indexed arguments_ come first.
2. 0 or more _aggregators_ come second.
3. 0 or more arguments supplied by other ParameterResolver implementations come last.

Thoughts?

[sbrannen]

New _Deliverables_ up for debate:

• Consider making the ArgumentsAggregator API generic (i.e., ArgumentsAggregator<T>) in order to make it easier to unit test an ArgumentsAggregator implementation.
• Consider introducing a setIndexOffset(int) method in ArgumentsAccessor that makes it easier to reuse existing position-based logic for input sets that are occasionally shifted to the right by a given _offset_ (i.e., n indexes).

[sbrannen]

Reintroduced the _team discussion_ label to clarify remaining open issues.

[sbrannen]

Revised User Guide content in 6306716afab6df2e77c8c72cf2987eb3d285fb4b.

[sbrannen]

Replying to myself...

• Consider making the ArgumentsAggregator API generic (i.e., ArgumentsAggregator<T>) in order to make it easier to unit test an ArgumentsAggregator implementation.

I suppose that's a moot point, since Java supports covariant return types: a PersonAggregator can already return Person from aggregateArguments(...) since Person _is-a_ Object.

In addition, the parameterized return type would effectively be ignored by JUnit. Any errors will occur at runtime in any case.

So I'll go ahead and remove the _Deliverable_.

[sbrannen]

Latest documentation can be viewed here:

https://junit.org/junit5/docs/snapshot/user-guide/#writing-tests-parameterized-tests-argument-aggregation

[marcphilipp]

Consider introducing a setIndexOffset(int) method in ArgumentsAccessor that makes it easier to reuse existing position-based logic for input sets that are occasionally shifted to the right by a given _offset_ (i.e., n indexes).

Is that only relevant for ArgumentsAggregator implementations? If so, I'd rather call them with an ArgumentsAccessor that already has that offset. Let's maybe hold off on this for now. We can revisit the topic when (if at all) we think about adding indices/range/offset like discussed above.

[marcphilipp]

Latest documentation can be viewed here:

https://junit.org/junit5/docs/snapshot/user-guide/#writing-tests-parameterized-tests-argument-aggregation

Looks great! 👍

[sbrannen]

Team Decision

1. Implement the aforementioned proposal following the _KISS_ principle.
2. Hold off on introducing support for additional configuration options such as offsets, ranges, and indices.

[sbrannen]

The work for this issue has now been completed in master.

Follow-up work to take place in #1369.

[geo-m]

That's great. If you ever plan on extending the feature i would appreciate a chance to work more on this. Thanks!