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

Replace hamcrest with assertj #5911

Open
wants to merge 133 commits into
base: master
Choose a base branch
from
Open

Replace hamcrest with assertj #5911

wants to merge 133 commits into from

Conversation

fil512
Copy link
Collaborator

@fil512 fil512 commented May 7, 2024

This merge request replaces practically all usages of hamcrest with assertj.

One thing I wanted to include in this MR but was unable to was to run checkstyle on all our tests and add a checkstyle to enforce no hamcrest imports.

I also tried to remove the hamcrest jar entirely, but sadly it is currently required by awaitility. I opened a ticket a ticket with awaitility asking them if they had considered switching from hamcrest to assertj.

Some tricky cases noted here:
(1)

		assertThat(composition.getText().getDivAsString(), stringContainsInOrder(
			"Vax 2015", "Vax 2010", "Vax 2005"
		));

becomes

		assertThat(composition.getText().getDivAsString()).containsSubsequence(
                        "Vax 2015", "Vax 2010", "Vax 2005"
                 );

(2)

                 assertThat(x, either(equalTo("a"), or(equalTo("b")));

becomes

                assertThat(x).isIn("a", "b");

(3)

       		assertThat(messageString, not(stringContainsInOrder(
			"extension",
			"meta"
		)));

becomes

       		assertThat(messageString).doesNotContainPattern("(?s)extension.*meta");

(4)

assertThat(myCaptureQueriesListener.getSelectQueries().get(0).getSql(true, false),
  either(containsString("rt1_0.RES_TYPE='Patient' and rt1_0.FHIR_ID='B' and rt1_0.PARTITION_ID is null or rt1_0.RES_TYPE='Patient' and rt1_0.FHIR_ID='A' and rt1_0.PARTITION_ID is null"))
  .or(containsString("rt1_0.RES_TYPE='Patient' and rt1_0.FHIR_ID='A' and rt1_0.PARTITION_ID is null or rt1_0.RES_TYPE='Patient' and rt1_0.FHIR_ID='B' and rt1_0.PARTITION_ID is null")));

becomes

assertThat(sql).satisfiesAnyOf(
  s -> assertThat(s).contains("rt1_0.RES_TYPE='Patient' and rt1_0.FHIR_ID='B' and rt1_0.PARTITION_ID is null or rt1_0.RES_TYPE='Patient' and rt1_0.FHIR_ID='A' and rt1_0.PARTITION_ID is null"),
  s -> assertThat(s).contains("rt1_0.RES_TYPE='Patient' and rt1_0.FHIR_ID='A' and rt1_0.PARTITION_ID is null or rt1_0.RES_TYPE='Patient' and rt1_0.FHIR_ID='B' and rt1_0.PARTITION_ID is null")
);

dotasek

This comment was marked as outdated.

@dotasek
Copy link
Contributor

dotasek commented May 28, 2024

@fil512 I did a review of ~50 randomly chosen test files, including all contents of hapi-fhir-test-util.

Some of them, particularly those that suggest switching to assertThatThrownBy(), isEqualTo() or contains(), are probably nitpicky, and you can feel free to mark them as resolved.

There are a few regarding test asserts that are always true, and some that appear to test nothing except assertj itself that I'm more interested in.

This looks like it was a long haul, @fil512 @tadgh, your patience with this is laudable!

@dotasek
Copy link
Contributor

dotasek commented May 31, 2024

I resolved the outstanding assertThatThrownBy() requested changes after your comments on the first few. If you find any left, they're clearly fine to resolve.

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.

None yet

3 participants