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

Adopt dedicated AssertJ assertions for more expressive test failure messages #13619

Merged

Conversation

timtebeek
Copy link
Contributor

Noticed that there were some instances where AssertJ wasn't used to it's full expressive potential, so I ran our AssertJ best practices recipe (via) to adopt dedicated assertions. This should help the message when an assertion fails, such that it's not "expected true but was false", but instead something like "expected collection to contain "apple", but was ["banana"]".

-assertThat(objectIdentities.size()).isEqualTo(1);
+assertThat(objectIdentities).hasSize(1);

-assertThat(objects.contains("apple")).isTrue();
+assertThat(objects).contains("apple");

These are mostly automated changes with some manual fixes; I recommend a squash merge if possible. I hope this is appreciated! :)

Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks, @timtebeek! This is very helpful.

I've left a comment inline about one of the changes.

Also, if you wish at this point, you are welcome to squash your commits to a single commit.

@jzheaux jzheaux self-assigned this Aug 9, 2023
@jzheaux jzheaux added in: core An issue in spring-security-core type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 9, 2023
@timtebeek timtebeek requested a review from jzheaux August 9, 2023 09:11
@timtebeek
Copy link
Contributor Author

Glad to hear you think it's helpful! I've fixed my manual mistake and think we should be good for a (squash) merge.

I'd noticed another small possible improvement, which is why it's fun for me to do these PRs; can circle back on that in the future if that's appreciated; should make for a smaller PR by then.

@timtebeek
Copy link
Contributor Author

Anything else you'd like to see before a squash merge @jzheaux ? I'd hope to get this in before there's merge conflicts if at all possible, but understand this is far from a critical change.

Use this link to re-run the recipe: https://app.moderne.io/recipes/builder/bGVuS?organizationId=RGVmYXVsdA%3D%3D

Co-authored-by: Moderne <team@moderne.io>
@timtebeek timtebeek force-pushed the refactor/assertj-best-practices branch from 650a2fc to 1d113fd Compare August 19, 2023 23:08
@timtebeek
Copy link
Contributor Author

Gone ahead and merged the followup commits into the first one such that it's now just a single commit; hope that helps!

@jzheaux jzheaux added this to the 6.2.0-M3 milestone Sep 12, 2023
@jzheaux jzheaux merged commit 9df9cb5 into spring-projects:main Sep 12, 2023
2 checks passed
@jzheaux
Copy link
Contributor

jzheaux commented Sep 12, 2023

Thanks, @timtebeek! This is now merged into main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core An issue in spring-security-core type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants