alexruiz / fest-assert-2.x

FEST Fluent Assertions 2.x
http://fest.easytesting.org
Apache License 2.0
402 stars 69 forks source link

Problems with ListAssert and Wildcards #121

Open jschneider opened 11 years ago

jschneider commented 11 years ago

I'd expected the following code to compile successfully:

List<String> strings0 = new ArrayList<String>();
List<? extends String> strings1 = new ArrayList<String>();

assertThat( strings0 ).isEqualTo( strings0 );
assertThat( strings0 ).isEqualTo( strings1 );
assertThat( strings1 ).isEqualTo( strings1 );
assertThat( strings1 ).isEqualTo( strings0 );
joel-costigliola commented 11 years ago

Well ... generics hell.

If you look at isEqualTo definition, you can see that it takes the object under assertion type as the parameter type To be clear : assertThat("str") expects to get a String as isEqualTo parameter.

Note that, in your example, strings0 = strings1; does not compile because strings0 only accepts String but no subclasses of String. This explains why assertThat( strings0 ).isEqualTo( strings1 ); does not compile : you are trying to compare incompatible types.

When it comes to generics hell, I often read Angelika Langer Generics FAQ.

Without having consulted Anglika's FAQ (which is bad I know), I would say that for assertThat( strings1 ).isEqualTo( strings1 );, you can't compare two List<? extends String> because they may contain different objects type (let's say List<MyStringSubclass> and List<YourStringSubclass>). Same reason for the last one.

alexruiz commented 11 years ago

I think that making FEST-Assert more 'type safe' by using generics is not the right approach. In tests it is OK to relax type safety a bit. Sometimes making things compile in tests can add more work than necessary. For example, if I'm asserting that an instance of Person is equal to another one, and the method that the returns the Person to verify returns it as Entity, I would have to cast it to Person just to satisfy the test. I don't see the point. The test is going to run anyway and it will tell us if it passed or not.

In the case that Johannes showed, who cares if the lists are declared with slightly different types? what matters is verifying that the elements are equal.

I've been thinking about it for some time, even before Johannes' message. I think reverting to the old behavior (in FEST-Assert) is the best to do.

Cheers, -Alex

On Sat, Oct 27, 2012 at 4:13 PM, Joel Costigliola notifications@github.comwrote:

Well ... generics hell.

If you look at isEqualTo definition, you can see that it takes the object under assertion type as the parameter type To be clear : assertThat("str") expects to get a String as isEqualToparameter.

Note that, in your example, strings0 = strings1; does not compile because strings0 only accepts String but no subclasses of String. This explains why assertThat( strings0 ).isEqualTo( strings1 ); does not compile : you are trying to compare incompatible types.

When it comes to generics hell, I often read Angelika Langer Generics FAQhttp://www.angelikalanger.com/GenericsFAQ/JavaGenericsFAQ.html .

Without having consulted Anglika's FAQ (which is bad I know), I would say that for assertThat( strings1 ).isEqualTo( strings1 );, you can't compare two List<? extends String> because they may contain different objects type (let's say List and List). Same reason for the last one.

— Reply to this email directly or view it on GitHubhttps://github.com/alexruiz/fest-assert-2.x/issues/121#issuecomment-9840918.

joel-costigliola commented 11 years ago

I see your point Alex, in some other cases having generics is an improvement, when writing Condition for example. I think we should weight the pros and cons, and maybe ask in user list what people prefer.

WDYT ?

Joel

On Sun, Oct 28, 2012 at 5:36 PM, Alex Ruiz notifications@github.com wrote:

I think that making FEST-Assert more 'type safe' by using generics is not the right approach. In tests it is OK to relax type safety a bit. Sometimes making things compile in tests can add more work than necessary. For example, if I'm asserting that an instance of Person is equal to another one, and the method that the returns the Person to verify returns it as Entity, I would have to cast it to Person just to satisfy the test. I don't see the point. The test is going to run anyway and it will tell us if it passed or not.

In the case that Johannes showed, who cares if the lists are declared with slightly different types? what matters is verifying that the elements are equal.

I've been thinking about it for some time, even before Johannes' message. I think reverting to the old behavior (in FEST-Assert) is the best to do.

Cheers, -Alex

On Sat, Oct 27, 2012 at 4:13 PM, Joel Costigliola notifications@github.comwrote:

Well ... generics hell.

If you look at isEqualTo definition, you can see that it takes the object under assertion type as the parameter type To be clear : assertThat("str") expects to get a String as isEqualToparameter.

Note that, in your example, strings0 = strings1; does not compile because strings0 only accepts String but no subclasses of String. This explains why assertThat( strings0 ).isEqualTo( strings1 ); does not compile : you are trying to compare incompatible types.

When it comes to generics hell, I often read Angelika Langer Generics FAQhttp://www.angelikalanger.com/GenericsFAQ/JavaGenericsFAQ.html .

Without having consulted Anglika's FAQ (which is bad I know), I would say that for assertThat( strings1 ).isEqualTo( strings1 );, you can't compare two List<? extends String> because they may contain different objects type (let's say List and List). Same reason for the last one.

— Reply to this email directly or view it on GitHub< https://github.com/alexruiz/fest-assert-2.x/issues/121#issuecomment-9840918>.

— Reply to this email directly or view it on GitHubhttps://github.com/alexruiz/fest-assert-2.x/issues/121#issuecomment-9847411.

tedyoung commented 11 years ago

Yes, let's ask users, though since it involves generics, we'd need to provide a few examples to show the impact of the changes.

I definitely agree, though, that writing Conditions in 1.x was a real pain in terms of generics, so if the current 2.x improves that (I haven't tried it yet), then I'm all for it.

;ted

On Sun, Oct 28, 2012 at 9:51 AM, Joel Costigliola notifications@github.comwrote:

I see your point Alex, in some other cases having generics is an improvement, when writing Condition for example. I think we should weight the pros and cons, and maybe ask in user list what people prefer.

WDYT ?

Joel

On Sun, Oct 28, 2012 at 5:36 PM, Alex Ruiz notifications@github.com wrote:

I think that making FEST-Assert more 'type safe' by using generics is not the right approach. In tests it is OK to relax type safety a bit. Sometimes making things compile in tests can add more work than necessary. For example, if I'm asserting that an instance of Person is equal to another one, and the method that the returns the Person to verify returns it as Entity, I would have to cast it to Person just to satisfy the test. I don't see the point. The test is going to run anyway and it will tell us if it passed or not.

In the case that Johannes showed, who cares if the lists are declared with slightly different types? what matters is verifying that the elements are equal.

I've been thinking about it for some time, even before Johannes' message. I think reverting to the old behavior (in FEST-Assert) is the best to do.

Cheers, -Alex

On Sat, Oct 27, 2012 at 4:13 PM, Joel Costigliola notifications@github.comwrote:

Well ... generics hell.

If you look at isEqualTo definition, you can see that it takes the object under assertion type as the parameter type To be clear : assertThat("str") expects to get a String as isEqualToparameter.

Note that, in your example, strings0 = strings1; does not compile because strings0 only accepts String but no subclasses of String. This explains why assertThat( strings0 ).isEqualTo( strings1 ); does not compile : you are trying to compare incompatible types.

When it comes to generics hell, I often read Angelika Langer Generics FAQhttp://www.angelikalanger.com/GenericsFAQ/JavaGenericsFAQ.html .

Without having consulted Anglika's FAQ (which is bad I know), I would say that for assertThat( strings1 ).isEqualTo( strings1 );, you can't compare two List<? extends String> because they may contain different objects type (let's say List and List). Same reason for the last one.

— Reply to this email directly or view it on GitHub<

https://github.com/alexruiz/fest-assert-2.x/issues/121#issuecomment-9840918>.

— Reply to this email directly or view it on GitHub< https://github.com/alexruiz/fest-assert-2.x/issues/121#issuecomment-9847411>.

— Reply to this email directly or view it on GitHubhttps://github.com/alexruiz/fest-assert-2.x/issues/121#issuecomment-9847570.

alexruiz commented 11 years ago

Writing conditions is the best use case for generics. The thing is the way we are doing conditions (a carry over from the 1.x branch.)

We were treating conditions as any other assertion, and I think the Hamcrest way is the best:

assertThat(object, condition)

WDYT?

-Alex

On Sun, Oct 28, 2012 at 9:51 AM, Joel Costigliola notifications@github.comwrote:

I see your point Alex, in some other cases having generics is an improvement, when writing Condition for example. I think we should weight the pros and cons, and maybe ask in user list what people prefer.

WDYT ?

Joel

On Sun, Oct 28, 2012 at 5:36 PM, Alex Ruiz notifications@github.com wrote:

I think that making FEST-Assert more 'type safe' by using generics is not the right approach. In tests it is OK to relax type safety a bit. Sometimes making things compile in tests can add more work than necessary. For example, if I'm asserting that an instance of Person is equal to another one, and the method that the returns the Person to verify returns it as Entity, I would have to cast it to Person just to satisfy the test. I don't see the point. The test is going to run anyway and it will tell us if it passed or not.

In the case that Johannes showed, who cares if the lists are declared with slightly different types? what matters is verifying that the elements are equal.

I've been thinking about it for some time, even before Johannes' message. I think reverting to the old behavior (in FEST-Assert) is the best to do.

Cheers, -Alex

On Sat, Oct 27, 2012 at 4:13 PM, Joel Costigliola notifications@github.comwrote:

Well ... generics hell.

If you look at isEqualTo definition, you can see that it takes the object under assertion type as the parameter type To be clear : assertThat("str") expects to get a String as isEqualToparameter.

Note that, in your example, strings0 = strings1; does not compile because strings0 only accepts String but no subclasses of String. This explains why assertThat( strings0 ).isEqualTo( strings1 ); does not compile : you are trying to compare incompatible types.

When it comes to generics hell, I often read Angelika Langer Generics FAQhttp://www.angelikalanger.com/GenericsFAQ/JavaGenericsFAQ.html .

Without having consulted Anglika's FAQ (which is bad I know), I would say that for assertThat( strings1 ).isEqualTo( strings1 );, you can't compare two List<? extends String> because they may contain different objects type (let's say List and List). Same reason for the last one.

— Reply to this email directly or view it on GitHub<

https://github.com/alexruiz/fest-assert-2.x/issues/121#issuecomment-9840918>.

— Reply to this email directly or view it on GitHub< https://github.com/alexruiz/fest-assert-2.x/issues/121#issuecomment-9847411>.

— Reply to this email directly or view it on GitHubhttps://github.com/alexruiz/fest-assert-2.x/issues/121#issuecomment-9847570.

tedyoung commented 11 years ago

I have to disagree -- but then, I also disagree with some of the other changes to the fluent syntax (that Alex blogged about) that I haven't had time to write about.

However, if 2.x makes writing new assertion classes easier (again, I haven't looked at 2.x yet from this point of view), then conditions become less important.

My concern is about discoverability, and one of the great things about a fluent API is how discoverable things are: I just type assertThat(actual).and I get a bunch of choices of things I can do. In my environment, that's just about the most important thing.

;ted

On Sun, Oct 28, 2012 at 10:24 AM, Alex Ruiz notifications@github.comwrote:

Writing conditions is the best use case for generics. The thing is the way we are doing conditions (a carry over from the 1.x branch.)

We were treating conditions as any other assertion, and I think the Hamcrest way is the best:

assertThat(object, condition)

WDYT?

-Alex

On Sun, Oct 28, 2012 at 9:51 AM, Joel Costigliola notifications@github.comwrote:

I see your point Alex, in some other cases having generics is an improvement, when writing Condition for example. I think we should weight the pros and cons, and maybe ask in user list what people prefer.

WDYT ?

Joel

On Sun, Oct 28, 2012 at 5:36 PM, Alex Ruiz notifications@github.com wrote:

I think that making FEST-Assert more 'type safe' by using generics is not the right approach. In tests it is OK to relax type safety a bit. Sometimes making things compile in tests can add more work than necessary. For example, if I'm asserting that an instance of Person is equal to another one, and the method that the returns the Person to verify returns it as Entity, I would have to cast it to Person just to satisfy the test. I don't see the point. The test is going to run anyway and it will tell us if it passed or not.

In the case that Johannes showed, who cares if the lists are declared with slightly different types? what matters is verifying that the elements are equal.

I've been thinking about it for some time, even before Johannes' message. I think reverting to the old behavior (in FEST-Assert) is the best to do.

Cheers, -Alex

On Sat, Oct 27, 2012 at 4:13 PM, Joel Costigliola notifications@github.comwrote:

Well ... generics hell.

If you look at isEqualTo definition, you can see that it takes the object under assertion type as the parameter type To be clear : assertThat("str") expects to get a String as isEqualToparameter.

Note that, in your example, strings0 = strings1; does not compile because strings0 only accepts String but no subclasses of String. This explains why assertThat( strings0 ).isEqualTo( strings1 ); does not compile : you are trying to compare incompatible types.

When it comes to generics hell, I often read Angelika Langer Generics FAQhttp://www.angelikalanger.com/GenericsFAQ/JavaGenericsFAQ.html .

Without having consulted Anglika's FAQ (which is bad I know), I would say that for assertThat( strings1 ).isEqualTo( strings1 );, you can't compare two List<? extends String> because they may contain different objects type (let's say List and List). Same reason for the last one.

— Reply to this email directly or view it on GitHub<

https://github.com/alexruiz/fest-assert-2.x/issues/121#issuecomment-9840918>.

— Reply to this email directly or view it on GitHub<

https://github.com/alexruiz/fest-assert-2.x/issues/121#issuecomment-9847411>.

— Reply to this email directly or view it on GitHub< https://github.com/alexruiz/fest-assert-2.x/issues/121#issuecomment-9847570>.

— Reply to this email directly or view it on GitHubhttps://github.com/alexruiz/fest-assert-2.x/issues/121#issuecomment-9847938.

joel-costigliola commented 11 years ago

Writing custom assertions in 2.x is not yet simple enough.

We have #109 to ease that (but I can't work on it until Alex is done with its refactoring) and also the possibility to generate some with fest-assertion-generator and the corresponding maven plugin maven-fest-assertion-generator-plugin. There is also an eclipse plugin coming soon that will allow to do the same thing.

I agree with Ted on discoverability, that made me choose Fest over Hamcrest few years ago.

alexruiz commented 11 years ago

Ted,

It is not about replacing the fluent API with conditions. It is about removing conditions from the method chain.

Joel,

I think we will be able to make FEST semi-easy to extend (at least in Java.) I'm really excited about the FEST Assertion Generator. It will fill the gap nicely. This project will be our user's best friend :)

Cheers, -Alex

On Sun, Oct 28, 2012 at 10:46 AM, Joel Costigliola <notifications@github.com

wrote:

Writing custom assertions in 2.x is not yet simple enough.

We have #109 https://github.com/alexruiz/fest-assert-2.x/issues/109 to ease that (but I can't work on it until Alex is done with its refactoring) and also the possibility to generate some with fest-assertion-generatorhttps://github.com/joel-costigliola/fest-assertion-generatorand the corresponding maven plugin maven-fest-assertion-generator-pluginhttps://github.com/joel-costigliola/maven-fest-assertion-generator-plugin. There is also an eclipse pluginhttps://github.com/joel-costigliola/fest-eclipse-plugincoming soon that will allow to do the same thing.

I agree with Ted on discoverability, that made me choose Fest over Hamcrest few years ago.

— Reply to this email directly or view it on GitHubhttps://github.com/alexruiz/fest-assert-2.x/issues/121#issuecomment-9848176.

jschneider commented 11 years ago

BTW: I think the code example above compiles with fest-assert 1.x...

alexruiz commented 11 years ago

Yes, it should, and that's the behavior I strongly think we should get back to in 2.x.

On Mon, Oct 29, 2012 at 12:14 AM, Johannes Schneider < notifications@github.com> wrote:

BTW: I think the code example above compiles with fest-assert 1.x...

— Reply to this email directly or view it on GitHubhttps://github.com/alexruiz/fest-assert-2.x/issues/121#issuecomment-9857873.

joel-costigliola commented 11 years ago

I also think we should get back to that behavior but only keep strongly typed Condition, which should be relatively easy, just a matter of changing assertion parameters from generic type to Object.

On Mon, Oct 29, 2012 at 6:35 PM, Alex Ruiz notifications@github.com wrote:

Yes, it should, and that's the behavior I strongly think we should get back to in 2.x.

On Mon, Oct 29, 2012 at 12:14 AM, Johannes Schneider < notifications@github.com> wrote:

BTW: I think the code example above compiles with fest-assert 1.x...

— Reply to this email directly or view it on GitHub< https://github.com/alexruiz/fest-assert-2.x/issues/121#issuecomment-9857873>.

— Reply to this email directly or view it on GitHubhttps://github.com/alexruiz/fest-assert-2.x/issues/121#issuecomment-9877937.

jsravn commented 11 years ago

I have to agree w/ Alex here. We've been using fest-assert 1.x for the past year or so. One of my favorite things about fest-asserts is how it relaxes type safety, so you don't get the code-diarrhea that is endemic of java generics. Something which substantially detracts from the readability of test code and provides very little positive benefit.

I feel that 1.4 took a step in the wrong direction with the self referencing types and loss of IterableAssert, but the negative effects were pretty limited. It looks like 2.x so far takes it even more extreme, making it much more painful to use.

alexruiz commented 11 years ago

Hi James,

I'm very curious to know your take on the self-referencing types. It made code maintenance a lot easier for us. I'd like to know the downsides from the user point of view.

Thanks! -Alex

On Fri, Nov 9, 2012 at 12:58 PM, James Ravn notifications@github.comwrote:

I have to agree w/ Alex here. We've been using fest-assert 1.x for the past year or so. One of my favorite things about fest-asserts is how it relaxes type safety, so you don't get the code-diarrhea that is endemic of java generics. Something which substantially detracts from the readability of test code and provides very little positive benefit.

I feel that 1.4 took a step in the wrong direction with the self referencing types and loss of IterableAssert, but the negative effects were pretty limited. It looks like 2.x so far takes it even more extreme, to the point of the library being very difficult to use for what should be simple tests as the OP points out.

— Reply to this email directly or view it on GitHubhttps://github.com/alexruiz/fest-assert-2.x/issues/121#issuecomment-10244395.

jsravn commented 11 years ago

Sorry, I should have been more specific. The self referencing types were fine. It was that #isEqualTo (and some other methods?) starting taking a parameterized type as their argument instead of Object. This led to issues where I couldn't do stuff like assertThat(someList).isEqualTo(someCollection), even if I knew for sure the collection was a list. This could occur for legacy api reasons, or whatever.

alexruiz commented 11 years ago

Thanks James. The behavior you described is completely wrong and should be fixed. 'isEqualTo' should take an Object. We'll fix that in 2.0.

Cheers, -Alex

On Sat, Nov 10, 2012 at 4:47 PM, James Ravn notifications@github.comwrote:

Sorry, I should have been more specific. The self referencing types were fine. It was that #isEqualTo (and some other methods?) starting taking a parameterized type as their argument instead of Object. This led to issues where I couldn't do stuff like assertThat(someList).isEqualTo(someCollection), even if I knew for sure the collection was a list. This could occur for legacy api reasons, or whatever.

— Reply to this email directly or view it on GitHubhttps://github.com/alexruiz/fest-assert-2.x/issues/121#issuecomment-10261869.