Should Fail.failIf<Condition> be used in test code?

[Ansgar Konermann]

Hi,

I'm moving this discussion to the dev mailing list.

Alex wrote on the generic mailing list:

> Hi Ansgar,
>
> My original thought was that the API in the Fail class is kind of
> ugly and it does not follow the fluent approach of the assertions,
> that's why I preferred not to expose it :D
>
> Once again, I'm over-worrying. Your argument for making the API
> public is the right one. If everybody can benefit from it, I have no
> objections :)

After thinking about my initial proposal for a while, I came to the
conclusion that it might be too liberal.

I think there are two different target audiences for the Fail classes' API:
1. Test author - should get a simple API and only use the unconditional
fail(...) methods from withing test code.
2. Assertion class author - should get reusable functions to check
frequent conditions like null, equals, etc.

I feel that it might be better to split the interfaces for these two
groups. If we offer Fail.failIf<Condition> to the casual test author, he
will probably start using failIf<Condition> instead of
assertThat(...).<Condition>, because then, there are two different APIs
do do the same thing: check that certain conditions hold. I'd like to
keep this separate.

One idea how to realize this is:
- introduce a dedicated java package for those classes intended to be
used by authors of custom assertion classes; the package name should
reflect the fact that it is intended to be used by assertion class
authors, not by test authors (maybe 'implementation', 'internal' --
certainly to be improved).
- extract all methods Fail.failIf<Condition> from class Fail and move
them into a new class which lives in this new package.

This way, we at least _guide_ both audiences. Of course, this does not
_guarantee_ that test authors don't call the conditional failIf<...>
methods, but a mindful test author will probably notice that he's doing
something wrong if he's calling into the library's internals.

Looking forward to feedback.

Kind regards

Ansgar

[Alex Ruiz]

Hi Ansgar,

I like your idea a lot! I think only the Fail class is a candidate for the 'impl' (or whatever name we decide later) package so far. wdyt?

Thanks!
-Alex

[Ansgar Konermann]

Alex Ruiz wrote:
> Hi Ansgar,
>
> I like your idea a lot! I think only the Fail class is a candidate for
> the 'impl' (or whatever name we decide later) package so far. wdyt?

I think class "Formatting", "PrimitiveFail" are probably also
candidates. Maybe even "AssertExtension".

Kind regards

Ansgar

[Alex Ruiz]

Yep, those are good ones. AssertExtension is kind of the exception, IMHO. I don't have a specific reason, just a gut-feeling that it should stay where it is.

BTW, probably "internal" would be a better name for the package, instead of "impl" :)

Thanks,
-Alex

[Ansgar Konermann]

Alex Ruiz wrote:

Yep, those are good ones. AssertExtension is kind of the exception, IMHO. I don't have a specific reason, just a gut-feeling that it should stay where it is.

BTW, probably "internal" would be a better name for the package, instead of "impl" :)

As I see it, the classes in this package would still comprise a "public API", but for a different use case (extending FEST). So "internal" might be a bit misleading.

What about "extensionpoints" or some other name expressing "hey, if you want to *extend* FEST, this is for you". In this case, AssertExtension would fit into this package well - at least in my eyes.

Regards

Ansgar

[Ansgar Konermann]

Alex Ruiz schrieb:

> Yep, those are good ones. AssertExtension is kind of the exception,
> IMHO. I don't have a specific reason, just a gut-feeling that it
> should stay where it is.
>
> BTW, probably "internal" would be a better name for the package,
> instead of "impl" :)

I'd like to settle the name. I feel "internal" is too strong, since the
package must be "a little bit public" for those users who want to extend
FEST Assertions.

If we could use "protected", that would be fine for me - but we can't,
since it's no valid Java identifier, but a reserved word.

Next best suggestion would be "confidential"...

Since I'm not a native speaker of English, but I feel that wording does
matter, I encourage you all to contribute your own suggestions and vote
for the one you like best.

Kind regards

Ansgar

[Ansgar Konermann]

Alex Ruiz wrote:
> BTW, probably "internal" would be a better name for the package,
> instead of "impl" :)

Maybe we can just append "api" to "protected" and get a package name of:

org.fest.assertions.protectedapi

What do you think?

Kind regards

Ansgar

[Alex Ruiz]

Hi Ansgar,

I'd prefer to wait a little bit before implementing this. My concern
is now the dependencies between packages. org.fest.assertions will
depend on org.fest.assertions.protectedapi, it seems like a package
depending on a sub-package. I'd like to take more time to think/talk/
discuss.

Many thanks!
-Alex

On Jul 26, 7:23 am, Ansgar Konermann <ansgar.konerm...@googlemail.com>
wrote:

[Ansgar Konermann]

Alex Ruiz wrote:
> I'd prefer to wait a little bit before implementing this. My concern
> is now the dependencies between packages. org.fest.assertions will
> depend on org.fest.assertions.protectedapi, it seems like a package
> depending on a sub-package. I'd like to take more time to think/talk/
> discuss.
>

Hi Alex,

unfortunately I commited the relevant changes a few minutes before I
received your mail. But that's no problem. I'll do a reverse merge
tomorrow or so.

The public part of FEST Assertions (org.fest.assertions) would depend on
a "subpackage", that's true. As long as there are no circular
dependencies, I don't see any problems with this by now. I. e.,
org.fest.assertions may depend on classes in
org.fest.assertions.extensionapi, but not vice versa.

Of course we could also decide to use a different structure, with
org.fest.assertions as the public part of the library and say
org.fest.assertionimpl or similar containing the "extension api".

Kind regards

Ansgar

[szczepiq]

> The public part of FEST Assertions (org.fest.assertions) would depend on
> a "subpackage", that's true. As long as there are no circular
> dependencies, I don't see any problems with this by now.

I agree with Ansgar. Alex, are you saying depending on subpackage is
wrong? (Damn, if this appears to be true I will have to refactor 50%
of all java code I wrote in my life ;)

Szczepan

[Alex Ruiz]

A couple of years ago, I read a paper about dependencies (where is the link when I need it?) It basically said that is better than a sub-package depends on a parent package, but not the other way around. I don't remember the details, so I don't have a strong argument here. All that was left in my head was what I just said :D

Let's just wait a little bit before opening the API further (please?) Once open, it's hard to make changes.

Thanks!
-Alex

[Alex Ruiz]

Sorry Ansgar for making you roll back your changes. I should have spoken earlier :(

Cheers,
-Alex

[szczepiq]

>Let's just wait a little bit before opening the API further (please?) Once open, it's hard to make changes.

Tell me about it... You've gotta maintain it forever. I'd say FEST
assert is a kind of library that can evolve slowly without losing
attraction or users.

Cheers,
szczepan

[Ansgar Konermann]

Alex Ruiz wrote:
> Sorry Ansgar for making you roll back your changes. I should have
> spoken earlier :(

Hi Alex,

not a problem at all. The changes are not lost, I simply created an
experimental branch containing all the changes and reverted the trunk :-)

Kind regards

Ansgar

[Alex Ruiz]

I agree :)

Thanks!
-Alex

[Alex Ruiz]

The experimental branch is a good idea :)

Cheers!
-Alex