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

Move internal extracting to AbstractAssert #1781

Merged
merged 1 commit into from Mar 4, 2020
Merged

Move internal extracting to AbstractAssert #1781

merged 1 commit into from Mar 4, 2020

Conversation

scordio
Copy link
Member

@scordio scordio commented Feb 10, 2020

The PR is not ready yet, tests and javadoc need some reworking.

@kriegfrj @bjhargrave would this help your needs?

@joel-costigliola I added the isPublic() matcher to METHODS_CHANGING_THE_OBJECT_UNDER_TEST in SoftProxies to allow a protected extracting without changing the method name. At the moment I don't see any side effect but please let me know your opinion about it. ✅ Applied in 3534841.

Check List:

@scordio scordio changed the title Add protected extracting to AbstractAssert WIP Add protected extracting to AbstractAssert Feb 10, 2020
Copy link
Contributor

@bjhargrave bjhargrave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, with suggested changes, this will meet our use cases.

@scordio
Copy link
Member Author

scordio commented Feb 10, 2020

Thanks for the feedback @bjhargrave! I should be able to continue on this topic in the next days.

In general InstanceOfAssertFactory<T,ASSERT> should be InstanceOfAssertFactory<? super T,ASSERT> to provide maximum flexibility.

I agree with you that it would make sense to increase flexibility, I only want to see the impact of it adding proper tests. If the changes get too big, I may do them in a separate PR.

Copy link
Contributor

@kriegfrj kriegfrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few ideas

@bjhargrave
Copy link
Contributor

I built this PR locally and tried using the new extracting(Function,AssertFactory) method in my custom assert class. It looks good to me. The code compiled fine and passed the tests which verified the assertion state was propagated.

Thanks!

@@ -210,8 +198,7 @@ public abstract class AbstractPromiseAssert<SELF extends AbstractPromiseAssert<S
         *             resolved successfully.
         */
        public AbstractThrowableAssert<?, ? extends Throwable> hasFailedWithThrowableThat() {
-               return hasFailed().asObjectAssert()
-                       .extracting(this::getFailure, InstanceOfAssertFactories.THROWABLE);
+               return hasFailed().extracting(this::getFailure, InstanceOfAssertFactories.THROWABLE);
        }
 
        void assertNotFailed() {
@@ -271,8 +258,7 @@ public abstract class AbstractPromiseAssert<SELF extends AbstractPromiseAssert<S
         *             resolved with a failure.
         */
        public AbstractObjectAssert<?, RESULT> hasValueThat() {
-               return isSuccessful().asObjectAssert()
-                       .extracting(this::getValue);
+               return isSuccessful().extracting(this::getValue, Assertions::assertThat);
        }
 
        /**
@@ -294,7 +280,7 @@ public abstract class AbstractPromiseAssert<SELF extends AbstractPromiseAssert<S
         */
        public <ASSERT extends AbstractAssert<?, ?>> ASSERT hasValueThat(
                InstanceOfAssertFactory<? super RESULT, ASSERT> assertFactory) {
-               return hasValueThat().asInstanceOf(assertFactory);
+               return isSuccessful().extracting(this::getValue, assertFactory);
        }

@scordio
Copy link
Member Author

scordio commented Feb 19, 2020

@bjhargrave thanks for the feedback, glad it worked! To make it mergeable I need to rework the tests a bit more but I don't expect further changes in AbstractAssert.

@scordio scordio changed the title WIP Add protected extracting to AbstractAssert Add protected extracting to AbstractAssert Mar 3, 2020
@scordio scordio merged commit 0ec343b into assertj:master Mar 4, 2020
@scordio scordio deleted the abstract-assert_extracting branch March 4, 2020 21:12
@bjhargrave
Copy link
Contributor

Thanks so much for all your work on this!

@scordio scordio changed the title Add protected extracting to AbstractAssert Move internal extracting to AbstractAssert Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom Assertions and comparison failures
3 participants