hapi-fhir icon indicating copy to clipboard operation
hapi-fhir copied to clipboard

Replace hamcrest with assertj

Open fil512 opened this issue 9 months ago • 2 comments

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:

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

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

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

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

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

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

   				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") );

fil512 avatar May 07 '24 20:05 fil512

Formatting check succeeded!

github-actions[bot] avatar May 07 '24 20:05 github-actions[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.42%. Comparing base (497b9f2) to head (5a2359a). Report is 80 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #5911      +/-   ##
============================================
+ Coverage     83.39%   83.42%   +0.03%     
- Complexity    26927    27117     +190     
============================================
  Files          1681     1694      +13     
  Lines        103965   104727     +762     
  Branches      13189    13275      +86     
============================================
+ Hits          86702    87370     +668     
- Misses        11613    11658      +45     
- Partials       5650     5699      +49     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 08 '24 20:05 codecov[bot]

@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 avatar May 28 '24 20:05 dotasek

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.

dotasek avatar May 31 '24 15:05 dotasek