Provide Assertj helpers #11

sviperll · 2024-11-29T12:53:41Z

Fixes #7

PTAL at the code, you may want to add some code review comments
lets discuss an API a little more before finalizing.

In #10, you provided an example of the "that"-style method usage.

We assume the following Result-value:

    Result result = Result.success(List.of("one", "two", "three"));

and the following imports:

    import static com.github.sviperll.result4jassertj.ResultAssert.assertThat;
    import static org.assertj.core.api.Assertions.assertThat;

I see a several ways assertions can be written:

option

assertThat(result)
        .isSuccess()
        .hasSuccessThat(
                success ->
                        Assertions.assertThat(success)
                                .hasSize(3)
                                .containsExactlyInAnyOrder("three", "two", "one")
        );

option

assertThat(result)
        .isSuccess()
        .hasSuccessThat(
                success ->
                        success.asInstanceOf(InstanceOfAssertFactories.list(String.class))
                                .hasSize(3)
                                .containsExactlyInAnyOrder("three", "two", "one")
        );

option

assertThat(result)
        .isSuccess()
        .hasSuccessThat()
        .asInstanceOf(InstanceOfAssertFactories.list(String.class))
        .hasSize(3)
        .containsExactlyInAnyOrder("three", "two", "one");

If we do with static-imports, like assertj promotes

    import static org.assertj.core.api.InstanceOfAssertFactories.list;

we can get 2 or 3 even shorter:

...

option

assertThat(result)
        .isSuccess()
        .hasSuccessThat(
                success ->
                        success.asInstanceOf(list(String.class))
                                .hasSize(3)
                                .containsExactlyInAnyOrder("three", "two", "one")
        );

option

assertThat(result)
        .isSuccess()
        .hasSuccessThat()
        .asInstanceOf(list(String.class))
        .hasSize(3)
        .containsExactlyInAnyOrder("three", "two", "one");

That said, I've found a precedent for such methods in assertj itself and there they use another form, something that can be written as

...
...
...

option

assertThat(result)
        .isSuccess()
        .extractingValue(as(list(String.class)))
        .hasSize(3)
        .containsExactlyInAnyOrder("three", "two", "one");

See AbstractObjectAssert::extracting and AbstractOptionalAssert::get.

I think we should go with the 4th option, like the following:

class ResultAssert<T, E> extends AbstractObjectAssert<ResultAssert<T, E>, Result<T, E>> {
    public <A extends AbstractAssert<?, ?>> A extractingValue(InstanceOfAssertFactory<?, A> assertFactory) {
        return extracting(ResultAssert::getValueOrFail, assertFactory);
    }
    private T getValueOrFail(Result<T, E> result) {
        if (!(result instanceof Result.Success(var value)))
            throw fail("expecting result to be success");
        return value;
    }
}

WDYT?

Add ResultAssert

Add ResultAssert comments

…eature/assertj

marcindabrowski · 2024-11-29T21:50:57Z

Hey, below is my 5 cents.

Review

Looks good, but I taught about removing AssertJ bom from pom file. Since we are using only one library from them, I think that adding version to assertj-core is enough. Otherwise, everyone who will use result4j will automatically import the bom file.
Can you put back my name in author tag on ResultAssert? File can have multiple author tags, and I feel as partial author of this file.
What about moving all test to result4j-assertj and using ResultAssert in every test? It will validate how the assert can be use at the same time. And since two modules are build, there is no need to run the same tests twice.

ResultAssert

I was also thinking about the assertion and ability to use satisfies method.

Then we can define isError as:

public ObjectAssert<E> isError() {
    isNotNull();

    if (actual instanceof Result.Error(E error)) {
        return Assertions.assertThat(error);
    }
    throw failure("Expected Result to be Error but was Success");
}

and then we can use any assertion method of the error, or if the error is not common type, just use satisfies method to apply custom assertion like this:

 ResultAssert.assertThat(result)
     .isError()
     .satisfies(error ->
          Assertions.assertThat(error)
            .isInstanceOf(IllegalArgumentException.class)
            .hasMessage("Name is invalid")
     );

The same change will be applied to isSuccess.

With this approach user will have ability to anything he want, and isError and isSuccess will return instance of the assertion corresponding to its type (if it will be a common AssertJ type, or the one registred with AssertProvider).

After that change we can remove hasErrorEqualTo, hasErrorThat, hasSuccessEqualTo, hasSuccessThat, actualError and actualSuccess methods, and assert will have only two methods isError and isSuccess.

With this approach your last example will look like this:

assertThat(result)
    .isSuccess()
    .hasSize(3)
    .containsExactlyInAnyOrder("three", "two", "one");

Calling of extractingValue(as(list(String.class))) will be not needed.

WDYT?

sviperll · 2024-12-04T10:42:10Z

Looks good, but I taught about removing AssertJ bom from pom file. Since we are using only one library from them, I think that adding version to assertj-core is enough. Otherwise, everyone who will use result4j will automatically import the bom file.

I think it should work as intended because the bom is listed in dependencyManagement section and not in depenencies and because I use flatten-maven-plugin. flatten-maven-plugin should generate the pom.xml to be published in maven-central for consumers and it should include only actually used dependencies and no dependencyManagement

Can you put back my name in author tag on ResultAssert? File can have multiple author tags, and I feel as partial author of this file.

I do not like to track authorship in the source code. It is usually misleading and outdated (because there are no mechanisms to correctly track it), as you can see my name is absent from any source file.

I'm completely not against any kind of badge of honor. I think your contributions are valuable and worth recognition. I just think that the source code is not a place for this. I think I'm going to write release notes for the release and I'll try to list all your contributions there or you can just refer to your commits, which are all preserved, I haven't squashed anything or overwritten any history and I intend to continue this practice.

What about moving all test to result4j-assertj and using ResultAssert in every test? It will validate how the assert can be use at the same time. And since two modules are build, there is no need to run the same tests twice.

I think I would leave two copies of tests. The tests in result4j are valuable to be able to build result4j in isolation and they should try to cover more of the core functionality. The tests in result4j-assertj are to demonstrate the usage of assertj-helpers (I will convert everything there to use assertj). These two goals are different, so I expect these tests to diverge with the time, that's why I think two copies are actually ok.

ResultAssert

Then we can define isError as ... and then we can use any assertion method of the error, or if the error is not common type.

The same change will be applied to isSuccess.

I do not like it, because with such isError-method we will get that:

 ResultAssert.assertThat(result).isNotNull().isError();

is not the same as

 ResultAssert.assertThat(result).isError().isNotNull();

I would like to have a method with some more telling name, probably hasErrorThat or extractingError, so that it's more clear that you are now switching the subject.

With hasErrorThat the examples will become:

 ResultAssert.assertThat(result)
         .hasErrorThat()
         .satisfies(
                 error -> assertThat(error)
                         .isInstanceOf(IllegalArgumentException.class)
                         .hasMessage("Name is invalid")
         );

or

ResultAssert.assertThat(result)
         .hasErrorThat()
         .isInstanceOf(IllegalArgumentException.class)
         .asInstanceOf(InstanceOfAssertFactories.THROWABLE)
         .hasMessage("Name is invalid");

and for success:

assertThat(result)
         .hasValueThat()
         .satisfies(
                 value -> assertThat(value)
                         .hasSize(3)
                         .containsExactlyInAnyOrder("three", "two", "one")
         );

or differently:

assertThat(result)
        .hasValueThat()
        .asInstanceOf(list(String.class))
        .hasSize(3)
        .containsExactlyInAnyOrder("three", "two", "one");

The above looks to me like the best option that ticks all the boxes...

marcindabrowski · 2024-12-04T10:57:48Z

Hey, I agree with almost everything, but don't agree with that:

I do not like it, because with such isError-method we will get that:
 ResultAssert.assertThat(result).isNotNull().isError();
is not the same as
 ResultAssert.assertThat(result).isError().isNotNull();

Actually this one will be always possible:

ResultAssert.assertThat(result).isNotNull().hasErrorThat().isNotNull();

First .isNotNull() refers to result object, the latter to the error value.

Of course we can name the method as hasErrorThat, but the question is how this method should looks like?

public ObjectAssert<E> hasErrorThat() {
    isNotNull();

    if (actual instanceof Result.Error(E error)) {
        return Assertions.assertThat(error);
    }
    throw failure("Expected Result to be Error but was Success");
}

or somehow differently?

Do you want me to make a pull request with this change?

sviperll · 2024-12-04T11:29:03Z

First .isNotNull() refers to result object, the latter to the error value

Yes, this is mostly about the naming. The following I can understand

assertThat(result).isNotNull().hasErrorThat().isNotNull();

, but the same with the isError-name looks like a puzzler:

assertThat(result).isNotNull().isError().isNotNull();

how this method should looks like?

I think what you've written should work.

Do you want me to make a pull request with this change?

I was going to go through the changes and release a new version by the end of the week. The remaining changes that I know of:

change hasErrorThat to be as one that we have discussed
remove unnecessary specializations (hasErrorEqualTo, hasSuccessEqualTo)
convert all the tests in ResultCollectorsTest.java in result4j-assertj to use ResultAssert or remove those that do not need ResultAssert
check the generated flattened pom.xml in result4j that it doesn't contain any unnecessary dependencies/boms

You can do any of these to speed things up, but I was also planning to do some of this today and tomorrow.

[x] change hasErrorThat to be as one that we have discussed [x] remove unnecessary specializations (hasErrorEqualTo, hasSuccessEqualTo) [x] convert all the tests in ResultCollectorsTest.java in result4j-assertj to use ResultAssert or remove those that do not need ResultAssert

marcindabrowski · 2024-12-04T18:09:30Z

I made #12 PR with:

change hasErrorThat to be as one that we have discussed
remove unnecessary specializations (hasErrorEqualTo, hasSuccessEqualTo)
convert all the tests in ResultCollectorsTest.java in result4j-assertj to use ResultAssert or remove those that do not need ResultAssert

You can cherry pick my commit to this PR.

…thods without parameters.

Change ResultAssert methods.

…eature/assertj

sviperll · 2024-12-04T20:35:14Z

I've merged your changes and added some more modifications on top:

I've retained isError and isSuccess methods, because they may sometimes be useful on their own and they serve a good introduction when we explain what ResultAssert class provides.
I've renamed hasSuccessThat to hasValueThat, I think it is more understandable this way...
I've added some more documentation

Take a look. If there is no strong disagreement, then I'll merge this tomorrow and release version 1.2.

marcindabrowski · 2024-12-04T21:05:52Z

Few things.

You wrote in the Javadoc of the class:

// Standard AsserJ assertions:
import static org.assertj.core.api.Assertions.assertThat;

// Assertions for the Result-values
import static com.github.sviperll.result4jassertj.ResultAssert.assertThat;

Actually you can't import two static assertThat methods at the same time.
So only com.github.sviperll.result4jassertj.ResultAssert.assertThat should be imported.

In javadoc of hasErrorThat you shouldn't use {@link ResultAssert#hasErrorThat()} because it is pointing to the javadoc in which this link is. The same happens to hasValueThat.
And to be honest I don't like the name hasValueThat. Why it can't be hasSuccessThat?
You have isSuccess, and hasValueThat do not correspond with that. What about hasSuccessValueThat or hasSuccessThatValueIs?
But then we should also adjust method for Error.

WDYT?

marcindabrowski · 2024-12-05T10:38:22Z

And one more thing. Why assertion class is in result4jassertj. Why it can't be at the same package as Result class, or at least in result4j.assertj?

sviperll · 2024-12-05T11:08:44Z

You wrote in the Javadoc of the class: ...

Actually you can't import two static assertThat methods at the same time.
So only com.github.sviperll.result4jassertj.ResultAssert.assertThat should be imported.

I do believe this is wrong. You can import multiple methods with the same name. JLS implicitly allows it, see for instance section 15.12.2.1. There it says

<...> if the form of the method invocation expression is MethodName - that is, a single Identifier - then the search for potentially applicable methods also examines all member methods that are imported by single-static-import declarations and static-import-on-demand declarations of the compilation unit where the method invocation occurs (§7.5.3, §7.5.4) and that are not shadowed at the point where the method invocation appears.

Here it says "examines all member methods that are imported" and in section 7.5.3 there is a restriction on the static imports for the types with the same name, but no restrictions for the multiple static imports of methods with the same name.

Moreover, this is not only allowed, it is also the recommended way to use AssertJ. See the Providing an entry point for all custom assertions section in the AssertJ documentation. There is an explicit example that does:

import static my.project.MyProjectAssertions.assertThat;
import static org.assertj.core.api.Assertions.assertThat;

And this is the recommended way to do it. This is actually a reason, why the Creating your own assertion class section recommends that you create "An entry point to your specific assertion class to use with static import", this is because Java is to select the most specific version of assertThat-methods from all statically-imported variables.

And to be honest I don't like the name hasValueThat. Why it can't be hasSuccessThat?
You have isSuccess, and hasValueThat do not correspond with that. What about hasSuccessValueThat or hasSuccessThatValueIs?
But then we should also adjust method for Error.

WDYT?

I like hasValueThat better than hasSuccessThat and slightly better than hasSuccessValueThat. We have Result::orOnErrorThrow method that is named like this, because the "success value" is the main thing, that we do not even need to name it, otherwise the method would be called Result::getSuccessValueOrThrow and I think the current name is the right choice. The type is called Result to mean the result of some operation. The operation can be "successful", but not the result, the result is just there after the successful operation.

I think I'm mostly ok with choosing hasSuccessValueThat as a name instead of the hasValueThat as a name, if you really think that it's better, but I would retain other names, so in the end we will get the following set of methods in the ResultAssert-class:

isSuccess
isError
hasSuccessValueThat
hasErrorThat

Are you ok with that?

sviperll · 2024-12-05T11:23:16Z

And one more thing. Why assertion class is in result4jassertj.
Why it can't be at the same package as Result class, or

We need a separate artifact to depend on in tests.

The goals for result4j is to

have minimal bloat and
minimal dependencies.

Following these goals we do not want to bundle support of assertj in the same artifact as a main Result-class.

Also result4j is to support Java-modules (JPMS). JPMS doesn't support split-packages, i. e. multiple modules providing the same package, so we need to have a different package name then the one used for the main artifact.

at least in result4j.assertj?

This is more stylistic and only partly technical. When we have multiple packages I prefer organizing them following the layering or dependency-graph. I. e. if I see pkg.a.b and pkg.a.b.c, I assume that pkg.a.b.c is an implementation detail used by the pkg.a.b. result4j and result4j.assertj do not have this relationship they share the same level of abstraction, neither is an implementation detail of another. They are more like a two areas of the overarching API-surface, so that's why they are defined as a packages on the same level result4j and result4jassertj. Another way around is to move result4j into something like result4j.core, so that we will get

result4j.core
result4j.assertj

Probably it's a good solution, but this will break backwards compatibility, so we have to stay within the result4j package.

Practically I don't think package name is that big of a problem, so I would probably leave package-naming like it is right now.

marcindabrowski · 2024-12-05T12:25:48Z

Sorry for confusion with static import of assertThat. You are right.
I commented about it, because I'm writing tests with Spock in Groovy, and there it is impossible.
I'm OK with hasSuccessValueThat, and keeping other methods unchanged.
I don't wanted ResultAssert to be in the same Java artifact as Result class - it was me who asked about creating subproject. What I like in my projects to have all classes supporting tests in the same package as core classes.
But I don't agree with the logic you wrote about package tree. For me result4j.assertj means that assertj contains AssertJ assertions for result4j package. Even Spring has such packages - https://docs.spring.io/spring-boot/api/java/org/springframework/boot/test/context/assertj/AssertableApplicationContext.html. But at the end decision is yours.

sviperll · 2024-12-05T21:04:28Z

@marcindabrowski What do you think about renaming result4j-assertj to assertj-result4j and correspondingly com.github.sviperll.result4jassertj module and package to com.github.sviperll.assertj.result4j?

marcindabrowski · 2024-12-05T21:23:52Z

@marcindabrowski What do you think about renaming result4j-assertj to assertj-result4j and correspondingly com.github.sviperll.result4jassertj module and package to com.github.sviperll.assertj.result4j?

It is OK to me

marcindabrowski and others added 7 commits November 26, 2024 17:24

Add ResultAssert

1a69f19

Merge pull request #9 from marcindabrowski/feature/assertj

ea827bd

Add ResultAssert

Add ResultAssert comments

d503c26

Spawn result4j-assertj as a separate module

43cca8b

Merge pull request #10 from marcindabrowski/doc/assertj

d86ed6e

Add ResultAssert comments

Merge branch 'feature/assertj' of github.com:sviperll/result4j into f…

f2c7024

…eature/assertj

Fix javadoc formatting

c8a7c7c

sviperll added 5 commits December 4, 2024 20:38

Change ResultAssert-class to define hasErrorThat- and hasValueThat-me…

3f3e276

…thods without parameters.

Merge pull request #12 from marcindabrowski/feat/rework-result-assert

0fedabe

Change ResultAssert methods.

Merge branch 'feature/assertj' of github.com:sviperll/result4j into f…

61aaa18

…eature/assertj

More javadocs for result4j-assertj

a875136

Fix javadoc

70a954c

Rename result4j-assertj to assertj-result4j, fixes for review comments.

6bdd6d7

sviperll merged commit 6dc92c9 into main Dec 5, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide Assertj helpers #11

Provide Assertj helpers #11

sviperll commented Nov 29, 2024 •

edited

Loading

marcindabrowski commented Nov 29, 2024 •

edited

Loading

sviperll commented Dec 4, 2024 •

edited

Loading

marcindabrowski commented Dec 4, 2024

sviperll commented Dec 4, 2024 •

edited

Loading

marcindabrowski commented Dec 4, 2024

sviperll commented Dec 4, 2024

marcindabrowski commented Dec 4, 2024

marcindabrowski commented Dec 5, 2024 •

edited

Loading

sviperll commented Dec 5, 2024

sviperll commented Dec 5, 2024

marcindabrowski commented Dec 5, 2024

sviperll commented Dec 5, 2024

marcindabrowski commented Dec 5, 2024

Provide Assertj helpers #11

Provide Assertj helpers #11

Conversation

sviperll commented Nov 29, 2024 • edited Loading

marcindabrowski commented Nov 29, 2024 • edited Loading

Review

ResultAssert

sviperll commented Dec 4, 2024 • edited Loading

ResultAssert

marcindabrowski commented Dec 4, 2024

sviperll commented Dec 4, 2024 • edited Loading

marcindabrowski commented Dec 4, 2024

sviperll commented Dec 4, 2024

marcindabrowski commented Dec 4, 2024

marcindabrowski commented Dec 5, 2024 • edited Loading

sviperll commented Dec 5, 2024

sviperll commented Dec 5, 2024

marcindabrowski commented Dec 5, 2024

sviperll commented Dec 5, 2024

marcindabrowski commented Dec 5, 2024

sviperll commented Nov 29, 2024 •

edited

Loading

marcindabrowski commented Nov 29, 2024 •

edited

Loading

sviperll commented Dec 4, 2024 •

edited

Loading

sviperll commented Dec 4, 2024 •

edited

Loading

marcindabrowski commented Dec 5, 2024 •

edited

Loading