feedback

[szczepiq]

Hi

I discovered FEST asserts just yesterday and they seem to be very
interesting! I implemented something similar for internal purposes but
it is not nearly as good as fest.

However, there is something important that inhibits extending
assertions. Assertions class and Assert classes are final and it:
- makes it hard to introduce assertions for custom types
- makes it hard to introduce new assertion methods for already handled types

The way I imagine working with FEST assertions in my projects is that
I have my own Assertions class. If someone needs a custom assertion he
can just create overloaded assertThat() method in Assertion class. If
someone needs a custom assertion method for common type (e.g.
collection) he can create a new assertThat() method that hide the one
from the parent class. New assertThat() would return
MyOwnCollectionAssert with extra features.

This the example from the FEST wiki:

ServerSocketAssertion socket = new ServerSocketAssertion(server.getSocket());
assertThat(socket).isConnectedTo(2000);
//I'd prefer:
assertThat(socket).isConnectedTo(2000);

You might also consider providing humanized aliases for satisfies(),
e.g. is(), has(), etc.

assertThat("hello").as("Greeting").satisfies(isUppercase());
//I'd prefer:
assertThat("hello").as("Greeting").is(uppercase());

Hope this helps!
Szczepan Faber
http://mockito.org

[Alex Ruiz]

Hi Szczepan,

Thank you so much for your feedback!

These are the reasons for the Assertions and Assert classes to be final...please let me know if they make sense, and please correct me if I'm wrong :)

1. Assertions is final because all its methods are static. My understanding was to reference static methods it is better to do so from the class declaring them (e.g. Assertions.assertThat(boolean)). At least in Eclipse, the compiler gives me a warning when accessing a static method from a class other the declaring class.
   
2. Assert classes are final because of the lack of "self-types" in Java. For example, if we MyStringAssert subclasses StringAssert, we need to override all the methods in StringAssert to return MyStringAssert instead of StringAssert. I found that it is very easy to forget to do so, and it may be a source of confusion. I thought that by using composition instead of inheritance, we could remember to return the correct type from the assertion methods.
   

I like the proposed changes you suggested! I'll be filing bugs to:

1. Make Assertions non-final and change its private constructor to protected. I agree with you that this is more extension-friendly, regardless of the "warning" from Eclipse (I think this warning is just a setting in the compiler, and not a default)
2. Add a "is" and "isNot" methods that do the same as "satisfies" and "doesNotSatisfy"
   

Thanks a lot for helping us improve the project! :)

Best regards,
-Alex

[szczepiq]

Hi,

> Thank you so much for your feedback!

It was purely selfish. I want to use FEST efficiently =)

> These are the reasons for the Assertions and Assert classes to be
> final

You're right, there are reasons for sealing classes but there is one
important issue with it: Making the class final makes it harder to
extend the software in the way the class designer didn't anticipate.
Final classes should be banned from open-source. Amen.

> Assert classes are final because of the lack of "self-types" in Java. For
> example, if we MyStringAssert subclasses StringAssert, we need to override
> all the methods in StringAssert to return MyStringAssert instead of
> StringAssert. I found that it is very easy to forget to do so, and it may be
> a source of confusion. I thought that by using composition instead of
> inheritance, we could remember to return the correct type from the assertion
> methods.

Hmmmm, I see point now. I would still recommend to un-finalize the
assert classes... Internally, you can probably figure out some way of
verifying if you didn't forget to override (like extensive functional
test suite, PMD rule or jUnit test that reflects the methods, etc.).
Externally, I'd like to have an easy way to extend assertions on
already handled types. Extension via custom conditions doesn't work
for me because it's too hamcresty and I'm doing FEST here, right? :)
Anyway, it's up to you but I heartily recommend to un-finalize.

> "warning" from Eclipse (I think this warning is just a setting in the
> compiler, and not a default)

I wouldn't worry about the warning as it's only when someone calls the
static method on the instance. Why would anyone do it?

> Add a "is" and "isNot" methods that do the same as "satisfies" and
> "doesNotSatisfy"

Cool. I also recommended 'has' alias because sometimes it's hard to
express the assertion with 'is', e.g. page.hasHeadline() vs
page.isPageWithHeadline(). Again, it's up to you. I'm not interested
too much in custom conditions as I think custom assertions are way
more powerful.

Cheers,
Szczepan Faber
http://mockito.org

[Alex Ruiz]

Hi Szczepan,

Excellent points! These are the bugs I'm going to file:

1. Make Assertions non-final and make its constructor protected
2. Make assertion classes non-final
3. Create a PMD rule for users to check that their extensions have the correct return type
4. Add "is" and "isNot" to all assertions
5. Add "has" and "doesNotHave" to all assertions

Completely agree, custom assertions are more powerful than custom conditions!

Thanks a lot! :)

Cheers!
-Alex

[Ansgar Konermann]

Hi all,

szczepiq wrote:
>> Assert classes are final because of the lack of "self-types" in Java. For
>> example, if we MyStringAssert subclasses StringAssert, we need to override
>> all the methods in StringAssert to return MyStringAssert instead of
>> StringAssert. I found that it is very easy to forget to do so, and it may be
>> a source of confusion. I thought that by using composition instead of
>> inheritance, we could remember to return the correct type from the assertion
>> methods.
>>
>
> Hmmmm, I see point now. I would still recommend to un-finalize the
> assert classes... Internally, you can probably figure out some way of
> verifying if you didn't forget to override (like extensive functional
> test suite, PMD rule or jUnit test that reflects the methods, etc.).
> Externally, I'd like to have an easy way to extend assertions on
> already handled types.

As far as I understand, the two alternatives are:
1. use final assert classes and composition/delegation. "Finalness"
makes sure that we do not forget to define appropriate return types for
the derived assert classes
2. use non-final assert classes and take additional measures to make
sure we did not forget to override the base methods with more specific
return types

Comparing them, a FEST user defining an extension to an assert class has
to (correct me if I'm wrong):
1. define new check methods calling a delegate (assert<Condition>;
return this;)
2. define overridden methods calling the supertype's method and cast the
result to <SelfType>.

If you forget to override the super methods in 2), you cannot use the
new check methods after the first call to a check method of the
superclass, since it returns a more generic assertion class. If you do
override the method, the effort required to do this is nearly the same
as for option 1.

E. g. option 2 for XmlAssert extends StringAssert:

XmlAssert.assertThat(someXml).isNotEmpty().isSimilarTo(someOtherXml)

This will fail, since isNotEmpty will return a StringAssert instance
which does not know isSimilarTo().

In my eyes, this is a potential source of confusion to the casual FEST
user. Besides, the difference between defining an overridden method
calling super+casting on the one hand and defining a new method
delegating to the base class seems neglectable to me.

One disadvantage: you commonly have to define *new* methods in the
custom/extension assert classes before you can delegate. These methods
should have the same signature as in the base class. It's probably quite
error-prone to do this manually. Most modern IDEs have support for
implementing methods of java interfaces semi-automatically. If we start
to define the API of the base classes as interfaces, it should become
more easy to both implement the delegating methods and keep the
interface of the extension aligned with the one of the base classes.

Given we want to make sure that the API is consistent (i. e. always
returns <SelfType>):
- finalizing the base classes *guarantees* this
- alternative 1 does not significantly reduce implementation effort

I'd opt to keep finalized assert classes. Since extension of FEST seems
to be a source of questions and extension requests, we might do good in
agreeing on and documenting some "best practices" on how this should be
done.

>> "warning" from Eclipse (I think this warning is just a setting in the
>> compiler, and not a default)
>>
>
> I wouldn't worry about the warning as it's only when someone calls the
> static method on the instance. Why would anyone do it?
>

Here, we're also using our own custom assertions without extending class
Assertions at all. We just put a static factory method "assertThat" into
each new assert class and do a static import of this method in each test
we want to use the class in. This works quite smoothly. To us, the
Assertions class is basically a central factory provided by the
FEST-Assertions library which we do not fiddle about at all.

I invite you to discuss whether a single static factory for all your
assert classes in your project is the best approach. Remember that this
class would have to reference all your classes you'd like to write
assertions for, since assertThat takes the actual value as an argument.
This factory class becomes a dependency magnet. It will be hard to
manage these dependencies. Try to imagine your assertion factory
referencing classes from all over your domain model, your technical
infrastructure, ...

The solution using a single static factory method in the assert class
seems more appealing to me.

>
>> Add a "is" and "isNot" methods that do the same as "satisfies" and
>> "doesNotSatisfy"
>>
>
> Cool. I also recommended 'has' alias because sometimes it's hard to
> express the assertion with 'is', e.g. page.hasHeadline() vs
> page.isPageWithHeadline(). Again, it's up to you. I'm not interested
> too much in custom conditions as I think custom assertions are way
> more powerful.
>

I like FEST assertions for its simple API. I think it is a significant
plus to attract new users. We should take care not to complicate the api
unnecessarily. When I was new to the FEST API, I even considered it
strange to have both as() and describedAs(), which do exactly the same
thing. I later understood this is a technical necessity to allow usage
of FEST in groovy. I'd love to see FEST keep its API as simple as possible.

Think about http://c2.com/xp/OnceAndOnlyOnce.html

I'd agree that satisfies/doesNotSatisfy sounds like math, not like a
check of domain conditions. Nevertheless, I feel that this basic concept
of a condition/predicate is absolutely valid to use when defining tests.
We also kind of agree that "assertThat" is fine for all tests, however
your business analyst would probably express this a bit different (e. g.
"makeSureThat", "itMustAlwaysBeTheCase"). If someone asked me, I'd opt
not to include each and every variant which a natural language might
have developed for the same meaning over time, but to convey this
meaning using exactly one, carefully chosen word.

Kind regards,

Ansgar

[szczepiq]

Hi,

First of all, what I wrote earlier is just feedback & suggestions - do
whatever you like with it.

> If you forget to override the super methods in 2), you cannot use the
> new check methods after the first call to a check method of the
> superclass

Yes I noticed that. Personally, I'm fine with this. I want small,
kissy code even if there are trade-offs.

> In my eyes, this is a potential source of confusion to the casual FEST
> user. Besides, the difference between defining an overridden method

I guess you're right. Maybe I'm not a casual user and it just won't be
confusing to me. You have to decide whether you want to keep causal
user happy or regular user happy :)

> Given we want to make sure that the API is consistent (i. e. always
> returns <SelfType>):
> - finalizing the base classes *guarantees* this

You're right, this indeed guarantee it for FEST api. But how do you
guarantee if my own Assertions return self type all the time?

> I'd opt to keep finalized assert classes.

And you have full right to do so. In my view, it's just limiting
extensibility but it doesn't seem we will agree on it :)

> agreeing on and documenting some "best practices" on how this should be
> done.

How about un-finalizing the classes and documenting "best practices"

on how this should be done.

> I invite you to discuss whether a single static factory for all your
> assert classes in your project is the best approach. Remember that this
> class would have to reference all your classes you'd like to write
> assertions for, since assertThat takes the actual value as an argument.
> This factory class becomes a dependency magnet. It will be hard to
> manage these dependencies. Try to imagine your assertion factory
> referencing classes from all over your domain model, your technical
> infrastructure, ...
> The solution using a single static factory method in the assert class
> seems more appealing to me.

I really like single point of entry for all custom assertions. It
makes it so easy to find relevant assertions, it makes it obvious
where to place new ones. I've never had problem with it. Thank you for
looking after on my test dependencies but would you please give me
choice if I want to follow your pattern or mine, ok? :)

> not to include each and every variant which a natural language might
> have developed for the same meaning over time, but to convey this
> meaning using exactly one, carefully chosen word.

Hehe, this sounds like the ruby vs python war (or aliases&readability
vs exactly-one&consistency) :) I surrender from fighting but please
just allow me to have a different opinion.

Cheers,
Not arguing, just giving feedback and my perspective
Szczepan Faber

[Alex Ruiz]

IMHO, Szczepan's suggestions are valid and useful. They show a way of using the API that we didn't anticipate before. I admit I wanted to "protect" users from potential confusion, but now I see that giving more flexibility/extensibility is a good thing. Even the worst-case scenerios, where users forget to return the correct type from assertions methods and confusion from having multiple Assertions classes, are not difficult to troubleshoot.

Please review the list of bugs I'm going to file. I sent it to the mailing list yesterday (under this thread.)

Many thanks!
-Alex

[szczepiq]

Thanks Alex!

There is another thing that you might find interesting. You might
provide some kind of adapter for hamcrest matchers so that they can be
used in satisfies() expression.

I'm really happy you guys consider un-finalizing the classes as it
will make my life so much easier!

Thanks again for sharing your assertions. Every day I like them more.

Cheers,
Szczepan Faber

[Alex Ruiz]

Thank you Szczepan! I'll be filing those bugs today. As per support for Hamcrest matchers, I'm not sure how to proceed. I wouldn't want FEST-Assert to have a dependency on Hamcrest. Probably we can have an extension for Hamcrest matcher support, which contains an adapter that adapts Hamcrest matchers into FEST conditions. My concern is that the API can be too verbose. For example:

assertThat("hello").is(hc(notEmpty()));

(I'm using the "is" method instead of "satisfies") "notEmpty" would be a Hamcrest matcher and "hc" the adapter method ("hc" is just a random name, definitely needs improvement.) I think it is too verbose. So far I haven't figured out a way to make it "developer friendly".

Cheers,
-Alex

[szczepiq]

> Hamcrest matchers, I'm not sure how to proceed. I wouldn't want FEST-Assert
> to have a dependency on Hamcrest. Probably we can have an extension for

You're right. To make sure compiler doesn't complain and there is no
additional runtime dependency it probably needs to go to a separate
class. When the user imports this class he already has hamcrest on the
classpath because he already uses matchers and wants to use them with
fest.

Hamcrest adapter is just a suggestion - I don't have that use case in
my code - just loudly thinking. Sometimes it's good to show the user
ideas for potential integration points via concrete api. I found it
very useful in some cases.

I also noticed that fest is very smart when it comes asserting
Strings. It can figure out that jUnit is on classpath and throw
ComparisonFailure that has extra benefits in reporting mismatch,
especially if you use a decent IDE (I do something similar in
Mockito). I would suggest even going a little bit further: let's say
someone uses isEqualTo() on a non-string. What if instead of throwing
generic AssertionError you throw ComparisonFailure using x.toString()
results from compared objects? I guess you would have to compare the
strings and if they are the same then I would throw generic
AssertionError anyway. There is a reason I'm asking you to consider
this feature. Few times I remember I temporarily changed my
assertions: assertEquals(x, y) -> assertEquals(x.toString(),
y.toString()) just have better failure message in my tests. I don't
know what are the drawbacks of this idea but I thought I will throw it
in the air for you guys to consider.

Cheers,
Szczepan Faber

[Alex Ruiz]

Thanks Szczepan! I agree that the Hamcrest support can wait a little bit till we figure a not-so-verbose API :)

About comparing Objects and using 'toString' in a ComparisonFailure, I like the idea. If I understood correctly, your suggestion involves comparing objects like we always do, and if they are not equal, we throw a ComparisonFailure showing the 'toString' values of the objects. I think it can be really useful. So far, I cannot think of any drawbacks for existing users of FEST-Assert. Definitely a feature to think about :)

Cheers,
-Alex

[szczepiq]

>About comparing Objects and using 'toString' in a ComparisonFailure, I like the idea. If I understood correctly, your suggestion involves comparing objects like we always do, and if they are not equal, >we throw a ComparisonFailure showing the 'toString' values of the objects. I think it can be really useful. So far, I cannot think of any drawbacks for existing users of FEST-Assert. Definitely a feature >to think about :)

Yeah, this is exactly what I thought.

Cheers,
Szczepan Faber

[Alex Ruiz]

Cool! I'll add it the list of features for this release!

Once again, thanks Szczepan!

Cheers,
-Alex

[Ansgar Konermann]

szczepiq wrote:
> [...] would you please give me choice [...]
>

After thinking about it for a while, I must admit: Szczepan, you are right.

For a library which should be useful in a large number of projects, with a large number of different requirements, giving the user choice is the preferrable way to go.

Kind regards

Ansgar

[szczepiq]

Thanks Ansgar!

Szczepan

[Alex Ruiz]

New bugs can viewed at http://jira.codehaus.org/secure/IssueNavigator.jspa?reset=true&mode=hide&sorter/order=DESC&sorter/field=priority&resolution=-1&pid=11920&fixfor=15402

Thanks Szczepan!

Cheers,
-Alex