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

[FEATURE] Replace JUnit assertEquals() with Hamcrest matchers assertThat() #3680

Open
EduardoCorazon opened this issue Nov 12, 2023 · 1 comment
Labels
enhancement New feature or request triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.

Comments

@EduardoCorazon
Copy link
Contributor

Is your feature request related to a problem?
It is recommended to replace Assert.assertEquals() with assertThat() for a greater degree of verbosity and readability as seen in issue #1832 . But as of November 2023 there're a little less than 3k instances that need to be replaced. I've made some attempts as seen in #3500 and #3443 but after replacing more than half cases and running into a few issues with new updates to main, perhaps a new take on this issue would make for a good first issue.

What solution would you like?
Replacing all JUnit assertEquals(expected, actual) with Hamcrest assertThat(actual, is(expected))

Based on my experience I would suggest:

  • using regular expressions to automate much of the work:
sed 's/assertEquals(\(.*\), \(.*\));/assertThat(\2, is(\1));/' inputfile.java > outputfile.java

Note* After running the regex and saving the output as the original file name with mv outputfile.java inputfile.java you might have to then go the outputfile and edit any instances of Assert.assertThat() to assertThat() using a simple replace command in your editor. Also make sure to add imports for each file (as seen below) or create a higher order import:

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
  • Pushing for a single PR to have all changes be done all at once to reduce the errors your could receive from file conflicts.
  • Or breaking up changes into sections (test, integrationTests, etc) and merging them one at a time.

What alternatives have you considered?
As for alternatives to this issue itself, this change isn't really that necessary as it's mainly focused helping with debugging. One could argue that this feature isn't as useful but it could make for a good first PR, albeit a bit tedious.

Do you have any additional context?
As mentioned earlier, I don't think this issue presents too much significance outside the added readability/debugging but it could be something to do or I could simply be mistaken.
Best of luck :)

@EduardoCorazon EduardoCorazon added enhancement New feature or request untriaged Require the attention of the repository maintainers and may need to be prioritized labels Nov 12, 2023
@stephen-crawford
Copy link
Contributor

[Triage] Hi @EduardoCorazon, thank you for filing this issue. This looks like you have added some great detail and cover the background well. Judging from what you wrote, we can close this once we swap over all the of the assertions. Marking as triaged.

@stephen-crawford stephen-crawford added triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. and removed untriaged Require the attention of the repository maintainers and may need to be prioritized labels Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.
Projects
None yet
Development

No branches or pull requests

2 participants