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

fix: assertj contains is weaker than hamcrests contains #371

Merged

Conversation

davidburkhart
Copy link
Contributor

@davidburkhart davidburkhart commented Jul 5, 2023

What's changed?

Hamcrest's contains matches AssertJ's containsExactly. AssertJs contains does not care about order and completeness. Hamcrests contains does, see the docs!

What's your motivation?

Fix this bug.

Anything in particular you'd like reviewers to focus on?

Should I create an issue or is just the pr fine?

Another topic: I am not so sure about the mapping of containsInRelativeOrder to containsExactly as I don't know how containsInRelativeOrder works. From the docs I don't understand if it matches for completeness in both directions. Kind of related to this pr, but also a different topic.

Anyone you would like to review specifically?

no

Have you considered any alternatives or workarounds?

It is a bug to me.

Any additional context

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've added the license header to any new files through ./gradlew licenseFormat
  • I've used the IntelliJ auto-formatter on affected files
  • I've updated the documentation (if applicable)

@timtebeek timtebeek merged commit 5bf8029 into openrewrite:main Jul 5, 2023
@timtebeek
Copy link
Contributor

timtebeek commented Jul 5, 2023

Thanks for the quick work on this!

As for:

Another topic: I am not so sure about the mapping of containsInRelativeOrder to containsExactly as I don't know how containsInRelativeOrder works. From the docs I don't understand if it matches for completeness in both directions. Kind of related to this pr, but also a different topic.

I'd appears you've uncovered another issue, as when I look at the documentation there's subtle differences:
https://hamcrest.org/JavaHamcrest/javadoc/2.0.0.0/org/hamcrest/collection/IsIterableContainingInRelativeOrder.html#containsInRelativeOrder-E...-
https://joel-costigliola.github.io/assertj/core/api/org/assertj/core/api/ListAssert.html#containsExactly(ELEMENT...)

There might not even be an exact replacement in AssertJ yet, so definitely something to follow up.

This implementation was a quick one over a weekend some weeks ago. I'd been meaning to get back to it to verify it at scale, but have been traveling a bit too much to continue this as a weekend project, so I really appreciate you having a look now.

I am curious: how did you find these recipes? I'd consciously not shared them widely yet, as it's all pretty new. :)

@davidburkhart
Copy link
Contributor Author

I am trying out openrewrite to add some annotations in my project but also replacing hamcrest by assertj. I found the recipes by searching the repo for code samples and also looking specifically for hamcrest to assertj as well.

@timtebeek
Copy link
Contributor

Well glad you found these recipes and are trying them out; hope it helps you migrate.
We'll give it some more priority to work on the finer details such that it should all work as expected.
Feel free to share your thoughts as well; valuable feedback at this stage!

@timtebeek
Copy link
Contributor

Quick update to let you know we've released v2.0.7 containing your fix; thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants