Skip to content

Example renaming of some tests to the Given When Then style #1

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.

Sign up for GitHub

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

Open
mario-paniccia wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Example renaming of some tests to the Given When Then style #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
15bb2fd
example naming of some tests to the proposed given when then style
mario-paniccia Dec 4, 2023
0106d9e
simplified some test names by omitting the given clause when not needed
mario-paniccia Dec 5, 2023
710a922
renamed some unit tests
mario-paniccia Dec 5, 2023
64c8966
made some test names less specific as per code review comments
mario-paniccia Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...ov/hmcts/darts/transcriptions/controller/TranscriptionControllerGetTranscriptionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ void setUp() {
}

@Test
void getTranscription() throws Exception {
void givenExistingTranscription_whenGetTranscriptionDetailsIsRequested_thenDetailsAreReturned() throws Exception {
HearingEntity hearingEntity = dartsDatabase.getHearingRepository().findAll().get(0);
TranscriptionEntity transcription = dartsDatabase.getTranscriptionStub().createTranscription(hearingEntity);
transcription.setCreatedDateTime(OffsetDateTime.of(2023, 6, 20, 10, 0, 0, 0, ZoneOffset.UTC));
Expand Down Expand Up @@ -117,7 +117,7 @@ private void addCommentToWorkflow(TranscriptionWorkflowEntity workflowEntity, St
}

@Test
void getTranscriptionNotFound() throws Exception {
void whenGetTranscriptionDetailsForNonExistingTranscriptionIsRequested_thenErrorIsReturned() throws Exception {
Copy link
Copy Markdown

@mestebanez mestebanez Dec 6, 2023

Choose a reason for hiding this comment

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

Should this be givenANonExistingTranscription_whenGetTranscriptionDetailsIsRequested_thenAnErrorisReturned()

Copy link
Copy Markdown
Owner Author

@mario-paniccia mario-paniccia Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

hi @mestebanez, you raised a good point. @jackmaloney raised a similar one here.
Please let me know your preference

MockHttpServletRequestBuilder requestBuilder = get(ENDPOINT_URL_TRANSCRIPTION, -999);
MvcResult response = mockMvc.perform(requestBuilder).andExpect(status().isNotFound()).andReturn();
String actualResponse = response.getResponse().getContentAsString();
Expand Down
8 changes: 4 additions & 4 deletions .../transcriptions/controller/TranscriptionControllerUpdateTranscriptionApprovedIntTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ void beforeEach() {
}

@Test
void updateTranscriptionApprovedWithoutComment() throws Exception {
void givenAwaitingAuthorisationTranscription_whenApproveWithoutCommentIsRequested_thenNewStatusIsApprovedAndHasNoComment() throws Exception {

TranscriptionEntity existingTranscription = dartsDatabase.getTranscriptionRepository().findById(
transcriptionId).orElseThrow();
Expand Down Expand Up @@ -145,7 +145,7 @@ void updateTranscriptionApprovedWithoutComment() throws Exception {
}

@Test
void updateTranscriptionApprovedWithComment() throws Exception {
void givenAwaitingAuthorisationTranscription_whenApproveWithCommentIsRequested_thenNewStatusIsApprovedAndHasComment() throws Exception {

UpdateTranscription updateTranscription = new UpdateTranscription();
updateTranscription.setTranscriptionStatusId(APPROVED.getId());
Expand Down Expand Up @@ -190,7 +190,7 @@ void updateTranscriptionApprovedWithComment() throws Exception {
}

@Test
void updateTranscriptionShouldReturnTranscriptionNotFoundError() throws Exception {
void whenUpdateTranscriptionForNonExistingTranscriptionIsRequested_thenErrorIsReturned() throws Exception {
Copy link
Copy Markdown

@jackmaloney jackmaloney Dec 6, 2023

Choose a reason for hiding this comment

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

don't you need a given statement on here?

Copy link
Copy Markdown
Owner Author

@mario-paniccia mario-paniccia Dec 7, 2023

Choose a reason for hiding this comment

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

@jackmaloney I initially had the test name as:

givenRequestedTranscriptionDoesNotExist_whenUpdateTranscriptionIsRequested_thenTranscriptionNotFoundErrorIsReturned

but the given part there felt a bit "forced". In this scenario I think there really isn't a valid precondition.
So I just renamed the test omitting it, which I think it's ok sometimes.

But if you prefer always having a given, we can use the initial name. Let me know

UpdateTranscription updateTranscription = new UpdateTranscription();
updateTranscription.setTranscriptionStatusId(APPROVED.getId());
updateTranscription.setWorkflowComment("APPROVED");
Expand All @@ -216,7 +216,7 @@ void updateTranscriptionShouldReturnTranscriptionNotFoundError() throws Exceptio
}

@Test
void updateTranscriptionShouldReturnTranscriptionWorkflowActionInvalidError() throws Exception {
void givenExistingTranscription_whenUpdateToInvalidStatusIsRequested_thenErrorIsReturned() throws Exception {
UpdateTranscription updateTranscription = new UpdateTranscription();
updateTranscription.setTranscriptionStatusId(WITH_TRANSCRIBER.getId());
updateTranscription.setWorkflowComment("APPROVED");
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/uk/gov/hmcts/darts/audio/component/impl/AddAudioRequestMapperImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ void setUp() {
}

@Test
void testMapToMedia() {
void givenCourtroomExistsOrCreated_whenMapCourtroomAudioMetadataToMediaIsRequested_thenMetadataIsSuccessfullyMapped() {
Copy link
Copy Markdown

@davet1985 davet1985 Dec 6, 2023

Choose a reason for hiding this comment

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

This could get tedious when writing lots of unit tests. I tend to think of these as shouldDoSomething.

Copy link
Copy Markdown
Owner Author

@mario-paniccia mario-paniccia Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

hey @davet1985, it's true, for some test types it takes a bit of diligence and effort to come up with a good given/when/then name

CourthouseEntity courthouse = new CourthouseEntity();
courthouse.setCourthouseName("SWANSEA");

Expand Down
4 changes: 2 additions & 2 deletions src/test/java/uk/gov/hmcts/darts/event/service/impl/EventDispatcherImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class EventDispatcherImplTest {
EventHandlerRepository eventHandlerRepository;

@Test
void receiveWithNoHandlers() {
void givenMatchingEventHandlerOnDatabaseDoesNotExist_whenReceiveEventIsRequested_thenErrorIsReturned() {
List<EventHandler> eventHandlers = new ArrayList<>();
when(eventHandlerRepository.findByTypeAndSubType(anyString(), anyString())).thenReturn(Collections.emptyList());
EventDispatcherImpl eventDispatcher = new EventDispatcherImpl(eventHandlers, eventHandlerRepository);
Expand All @@ -47,7 +47,7 @@ void receiveWithNoHandlers() {
}

@Test
void receiveWithOneHandlers() {
void givenMatchingEventHandlerExists_whenReceiveEventIsRequested_thenEventIsSuccessfullyReceived() {
EventHandlerEntity eventHandlerEntity = new EventHandlerEntity();
when(eventHandlerRepository.findByTypeAndSubType(anyString(), anyString())).thenReturn(List.of(eventHandlerEntity));

Expand Down