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

Fuzzy Truth: better support for simple transformations of the iterable's elements #325

Closed
birdayz opened this Issue Jun 7, 2017 · 7 comments

Comments

Projects
None yet
4 participants
@birdayz
Copy link

birdayz commented Jun 7, 2017

I am kind of missing a possibility to check if elements in an iterable contain fields with specific values. AssertJ offers this:

assertThat(fellowshipOfTheRing).extracting(TolkienCharacter::getName)
                               .doesNotContain("Sauron", "Elrond");

(http://joel-costigliola.github.io/assertj/)

I wonder, how do you do this without having a "full object" ready, which you pass into contains ? Related: https://stackoverflow.com/questions/38902503/making-assertions-about-elements-in-an-iterable

What i did, was writing a helper implementation of Correspondence:

public class FieldEqualsCorrespondence<T> extends Correspondence<T, Object> {

	public static <T> FieldEqualsCorrespondence<T> extractField(Function<T, Object> extractor) {
		return new FieldEqualsCorrespondence<>(extractor);
	}

	private Function<T, Object> fieldExtractor;

	public FieldEqualsCorrespondence(Function<T, Object> fieldExtractor) {
		this.fieldExtractor = fieldExtractor;
	}

	@Override
	public boolean compare(T target, Object expected) {
		Object actual = this.fieldExtractor.apply(target);
		return actual.equals(expected);
	}

	@Override
	public String toString() {
		return "is equal to";
	}
}

This can be used the following way:

import static xxx.FieldEqualsCorrespondence.extractField;
...

assertThat(actual).comparingElementsUsing(extractField(Person::getName)).contains("Frodo");

I wonder if this is in the spirit of this project and worth contributing in some form, or if there is a built-in method to achieve this. Thanks!

@dimo414

This comment has been minimized.

Copy link
Member

dimo414 commented Jun 7, 2017

More generally, correspondence could perhaps support arbitrary transformation functions, and construct a Correspondence similar to the one you shared. However the .toString() in your example would be confusing; I think it would read like:

Not true that [{name: Legolas, weapon: Bow}, ...] contains at least one element that is equal to <Frodo>

Which (to me) doesn't read as clearly as "... that has name ".

Perhaps we could add an overload to .comparingElementsUsing() that takes a transformation and a description, e.g.:

assertThat(actual).comparingElementsUsing(Person::getName, "has name").contains("Frodo");

That's essentially just shorthand for implementing a custom Correspondence, but it seems readable / reasonable to me.

@PeteGillinGoogle - thoughts?

@birdayz could you be sure to sign the Google CLA if you haven't already, so we can use your contribution?

@PeteGillin

This comment has been minimized.

Copy link
Contributor

PeteGillin commented Jun 7, 2017

So, first of all: I agree that Correspondence is the right way to do this, I'm glad you found it useful.

Second of all: I agree that creating custom Correspondences is too hard right now. I have an AI to try to address this, but I think we need to be a bit thoughtful about exactly how we do it, so it might take a little time. Please bear with us.

Here's the fully story on that: I think the first step is to review all the custom Correspondences within the Google codebase (which is hopefully fairly representative), identify what patterns keep cropping up, and figuring out how we can help in those cases. The "apply some Function to the actual values and do equality against the expected values" use case is certainly one we're considering. Others are "apply some Function to both expected and actual values and do equality", "use some BiPredicate", "use some Comparator", and "use some hamcrest Matcher". The problem is that we almost certainly don't want to provide all these different options and confuse people with too many similar ways of doing things, which is why I want to be a bit careful about the decision.

One thing I believe quite strongly right now: It's better to provide Correspondence factory methods rather than add overloads to the methods that take Correspondence arguments. One reason is that there are at least three different methods that take Correspondence arguments, and if we went the overload route then we'd want to add the same overloads to all three for consistency and it would get pretty messy, whereas the factory methods could be write-once-use-everywhere. Another is that, if we end up providing several such helpers, it could be very confusing dealing with multiple overloads of the Correspondence-accepting methods with related-but-different signatures (especially it would be confusing dealing with the "Function applied to actual" vs "Function applied to both" distinction) and having a separate well-named factory method for each case would avoid that. Essentially, @dimo414's example would just gain an additional static method call, like this:
assertThat(actual).comparingElementsUsing(someStaticMethod(Person::getName, "has name")).contains("Frodo");
where, er, we need to think about the name for that method.

@dimo414 - there is an internal bug (assigned to me) with some more discussion if you want to read that or contribute.

@dimo414

This comment has been minimized.

Copy link
Member

dimo414 commented Jun 7, 2017

Depending on how many patterns you settle upon, I'd personally advocate for the overload - at least for the transformation+name option. It seems like a clearly useful one, and if nothing else would aid discoverability (especially thinking about tab completion). I'd rather incur a slight overhead internally to maintain n overloads than force external users to discover a Correspondences class (or wherever they live) and know that's what they need to use.

I'm currently +.75 on adding overloads; as you say I'd like to understand better what sort of use-cases we're looking at.

@birdayz

This comment has been minimized.

Copy link
Author

birdayz commented Jun 8, 2017

Thanks for the responses!
@dimo414 You are right about my toString, it's actually incorrect in my case. Passing a specific string to the correspondence/static factory function is a much better idea.
To be honest, i feel that both static constructors for pre-defined Correspondences, but also an overload for comparingElementsUsing make sense. However, i think the discoverability is better with an overload - and extracting a member and comparing it via equals is a MAJOR use case - IMHO - so an overload might be justified.

I would like to work on this once there is a decision, if you want.

@dimo414 dimo414 changed the title Equivalent to AssertJ 'extracting' Fuzzy Truth: better support for simple transformations of the iterable's elements Jun 8, 2017

@cpovirk

This comment has been minimized.

Copy link
Member

cpovirk commented Jun 8, 2017

During the original Fuzzy Truth discussions, @netdpb advocated for IterableSubject.transformedWith, which is similar (identical?) to what's proposed here. I have personally hoped that, if we do add it, we will be able to convince ourselves that it can be an overload of comparingElementsUsing. (In general, I don't mind overloads too much, even if we have to add them in n places. See, e.g., our assertInContext discussion, which I guess isn't the same thing but is at least about having to add a similar method in n places.) That's primarily for discoverability, as discussed, but it's also nice to make the code shorter.

@PeteGillin

This comment has been minimized.

Copy link
Contributor

PeteGillin commented Jun 8, 2017

Yeah, the point you all make about discoverability of overloads being better than the static factory methods is a good one. Let's figure out just how many overloads with what signatures we're talking about & then make that decision.

@cpovirk

This comment has been minimized.

Copy link
Member

cpovirk commented Mar 14, 2019

The main methods are in place for the next release:

We decided against an overload of comparingElementsUsing based on our current usage data, but if this change makes Correspondence use way more common (and we can hope it does :)), we can revisit.

@cpovirk cpovirk closed this Mar 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.