Example renaming of some tests to the Given When Then style

[mario-paniccia]

No description provided.

[chrisbellingham-hmcts]

Thanks for the naming proposal @mario-paniccia. Looks good, couple of comments/suggestions from me:

It might be worth adding an example for unit tests, some of which test multiple public/package-private methods in the one test class. In those cases, the challenge can be easily seeing which method is actually being tested, as the entry-point can be obsured if there is a lot of test code. Some tests bake the method name being tested into the start of the test name, which I think is useful. The downside of this is the test names wouldn't be automatically updated if the method name gets refactored, so there's a risk of name rot, so maybe there's a better way?

On the subject of name rot, it may be better to make the titles less-specific in some areas, as they are less likely to go out of sync with any changes in behaviour. E.g.:

givenRequestedTranscriptionDoesNotExist_whenGetTranscriptionIsRequested_thenTranscriptionNotFoundErrorIsReturned

Could become:

givenRequestedTranscriptionDoesNotExist_whenGetTranscriptionIsRequested_thenErrorIsReturned

I'd argue this approach still retains the majority of the value of the descriptive test name, and the reader can look to the assertion to see the specific error being returned.

[tonymort]

Definitely a big improvement on what we currently have. I would agree with @chrisbellingham-hmcts point about keeping the names more general. If written well by encapsulating stages of the test logic then the full scenario should described within the test method.

[mario-paniccia]

Thanks for the naming proposal @mario-paniccia. Looks good, couple of comments/suggestions from me:

It might be worth adding an example for unit tests, some of which test multiple public/package-private methods in the one test class. In those cases, the challenge can be easily seeing which method is actually being tested, as the entry-point can be obsured if there is a lot of test code. Some tests bake the method name being tested into the start of the test name, which I think is useful. The downside of this is the test names wouldn't be automatically updated if the method name gets refactored, so there's a risk of name rot, so maybe there's a better way?

On the subject of name rot, it may be better to make the titles less-specific in some areas, as they are less likely to go out of sync with any changes in behaviour. E.g.:

givenRequestedTranscriptionDoesNotExist_whenGetTranscriptionIsRequested_thenTranscriptionNotFoundErrorIsReturned

Could become:

givenRequestedTranscriptionDoesNotExist_whenGetTranscriptionIsRequested_thenErrorIsReturned

I'd argue this approach still retains the majority of the value of the descriptive test name, and the reader can look to the assertion to see the specific error being returned.

Hi @chrisbellingham-hmcts, thanks for your valuable comments.
Let me check with Jack if/when I can spend some more time on this task to rename some unit tests too (personally happy to do that), I need to check priorities.
With regards to your comments:

• "baking the method name being tested in the test name": an alternative could be to use the business domain "operation name" that method name represents, to avoid name rot as you correctly pointed out
• "less-specific titles": I see your point of view, I can do that if the team prefers it. In the rename I've tried to preserve the existing names as much as possible, which were quite specific.
  To play the devil's advocate, it's also true that tests should be considered as living documentation. So if an error type is changed for example, the documentation (test name) should be updated accordingly. I personally always review the test name when updating a test. But I appreciate this is difficult to do.
  The benefit of having more specific test names is that one can quickly learn the precise behavior without looking at test code.

[jackmaloney]

Yeh I agree with Mario with regards to less specific titles. Surely if output of the test is changing, the dev should just update the name?

[tonymort]

Yeh I agree with Mario with regards to less specific titles. Surely if output of the test is changing, the dev should just update the name?

But some of the scenarios are quite complex so the test names would become too long and themselves difficult to read. I think along with fixing test names we need to be encouraging better readability in the test implementation. Similar to a regular method, you don't specify the internal logic in the method name, you just specify it enough to make the context of its execution clear.

[mario-paniccia]

But some of the scenarios are quite complex so the test names would become too long and themselves difficult to read

@tonymort I think that is an indication that the test is too complex and should be broken down into simpler tests. Something that should be tackled separately though

[chrisbellingham-hmcts]

Agree that devs and reviewers should make the effort to review test names when things change but I think it's inevitable there will be occassions where this is missed.

Incorrect documentation is worse than no documentation, so going more specific may have more downside than upside. It's maybe a question of what do we want to know when reading a test name? For me I want an idea of:

• What's the entry-point of the test
• Is it happy or unhappy path
• What is the rough scenario

[tonymort]

But some of the scenarios are quite complex so the test names would become too long and themselves difficult to read

@tonymort I think that is an indication that the test is too complex and should be broken down into simpler tests. Something that should be tackled separately though

Yeah agree, we have many examples of that in test suite.

[mario-paniccia]

I've tackled the review comments, please let me know if this looks better now.
Details:

• renamed some unit tests
• made test names less specific

[jackmaloney]

Ok cheers all, and looks good @mario-paniccia.

Further to the point about generic test names, that's fine, as long as we don't make the test names so generic that there is not a clear differentiation between multiple tests within a class.

[jackmaloney]

don't you need a given statement on here?

[mario-paniccia]

@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

[mestebanez]

Should this be givenANonExistingTranscription_whenGetTranscriptionDetailsIsRequested_thenAnErrorisReturned()

[mario-paniccia]

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

[davet1985]

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

[mario-paniccia]

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

[cmagowan-hmcts]

my concern here is that you have taken a very VERY simple set of tests, like getTranscriptionNotFound that is pretty self explanatory and only 6 lines long and the new name is "whenGetTranscriptionDetailsForNonExistingTranscriptionIsRequested_thenErrorIsReturned" which is 85 characters long.

i use this view in my in my intellij when running tests:-

i can obviously move the divider to make the LHS wider, but its roughly sitting where i would normally want it and i can only see 35 characters.

i mean, i just stumbled across this one, which is 158 characters long!
givenTranscriberUserAndYourWorkViewRequested_thenReturnAssignedTranscriptionAndCompletedTranscriptionTodayAndDoNotReturnCompletedTranscriptionFromBeforeToday

Quite often i would use the name of a test in the files used by that test, so the test files names will end up being huge also, i'm not sure if it will blow a file name length limit

[mario-paniccia]

my concern here is that you have taken a very VERY simple set of tests, like getTranscriptionNotFound that is pretty self explanatory and only 6 lines long and the new name is "whenGetTranscriptionDetailsForNonExistingTranscriptionIsRequested_thenErrorIsReturned" which is 85 characters long.

i use this view in my in my intellij when running tests:-

i can obviously move the divider to make the LHS wider, but its roughly sitting where i would normally want it and i can only see 35 characters.

i mean, i just stumbled across this one, which is 158 characters long! givenTranscriberUserAndYourWorkViewRequested_thenReturnAssignedTranscriptionAndCompletedTranscriptionTodayAndDoNotReturnCompletedTranscriptionFromBeforeToday

Quite often i would use the name of a test in the files used by that test, so the test files names will end up being huge also, i'm not sure if it will blow a file name length limit

hey cmagowan-hmcts,
it's true, with this naming style (but some others too) names can become long.