rethink unit test style to remove duplication

Development
,
1

Hi there!

I was so getting used to the OpenMRS unit test style that I didnt see this until someone told me that we have duplication in our unit test style convention.

In the class we want to test we write the @should say hello, and nothing more that that which via the handy test case plugin translates to

/**
* @see Person#sayHello()
* @verifies say hello, and nothing more that that
*/
@Test
public void sayHello_shouldSayHelloAndNothingMoreThatThat() throws Exception {

so we have almost 3 identical occurrences of the same string.

Why do we need the @verifies javadoc?

If there is no compelling reason, are you for revamping the convention to remove duplication? I would volunteer to update the test case plugins and the wiki.

2

+1 from me!

3

Am in for it! :slight_smile:

4

I guess not everybody looking at the unit tests wants to read the carmelcase method name, the @verifies is to provide an easily readable text.

5

I remember being the main originator of these @should and @verifies annotations back in 2010 or earlier (though maybe I’m stealing the credit from Ben and/or Burke).

@should and @verifies are completely misguided. We came up with this approach because (a) we were focused on the question of “how to deal with a huge pile of existing API code with no tests” and not thinking about how best to write code going forwards (b) we were completely unfamiliar with TDD best practices and test coverage tools, and none of us had ever pair programmed with a dev who did TDD right

@should is exactly backwards from a TDD perspective, because it pushes you to write the code first and then write what it should do, and then create the test class. It’s okay as a halfway approach to get someone who is doing development wrong to start doing it a bit better. But if someone learns large-scale development via OpenMRS, then this @should approach teaches them the wrong lessons.

The purpose of @verifies was to let us calculate how many of our @should clauses actually had tests implemented. (I don’t recall if we ever actually built this tooling.) The correct solution to that problem is to just incorporate a code coverage tool in our build pipeline.

I have wanted to get rid of @should and @verifies for years, and have us instead take industry-standard approaches (which to be fair were much less mature last decade). But it has felt like a big task to clean up all the wiki pages and examples where we’re telling people to do exactly this, and I haven’t ever prioritized it.

@teleivo, If you would like to help improve our process here, I’d be eternally grateful. Just removing @verifies would be a start (since I don’t think we actually have any automation based on this), but I’d also be supportive of going further.

A question for you: when I first did some pair programming on TDD, I immediately stopped ever using the OpenMRS Test Generator plugin and started doing things the TDD way instead. (Also I switched from Eclipse to IntelliJ at the time, which played a role.) @teleivo, I know that you’ve done modern professional software development, but you still say the test generator plugin is “nifty”. Do you think it adds value to OpenMRS development?

6

yes thats the actual target I would like to reach :smile:

I meant its handy because it helps in fulfilling the openmrs unit testing requirement of writing so much boilerplate of @verifies and @should which if you dont have this plugin is such a pain.

I would totally go for promoting TDD practices within OpenMRS :blush:

I would take care of updating the wikis and examples. So this shouldnt stop us!

The question than would be:

Is it worth the effort of doing this in an iterative fashion of now removing @verifies and than going to the TDD style?

Or lets go cold turkey and remove the @verifies/@should and update the wikis.

openmrs-core has coveralls.io integration on PRs so we already prevent code from coming in that reduces coverage.

7

@darius should we discuss this on a call with others?

I would love to improve this! Currently the Eclipse plugin cannot be installed see Problems installing Eclipse plugins and I see new contributors creating PRs with the older test style. This just shows that this is a pressing issue. Its hard to tell them what to do. Old & new style mixed in the code, documentation about test style inconsistent and plugin cannot be installed.

8

Definitely let’s move this conversation forwards.

(I’m on vacation for the next 1.5 weeks though, so I won’t be joining any calls until I’m back at work.)

@dkayiwa, @wyclif, @raff, and others, what do you think of this?

-Darius

9

@teleivo i think you can go ahead and work with @jthomas to set up the call.

1 Like
10

@teleivo would Monday, March 20th be okay? @darius should be back and this is far enough out that @dkayiwa @wyclif and @raff should be able to join as well.

11

works for me, thank you!!!

1 Like
12

@teleivo, hey, my apologies for missing this call. (I’m in India, we had a team dinner, and I forgot to check my calendar beforehand.)

Was there a productive discussion? Do we need to reschedule?

13

No worries, the discussion was very good and the participants agreed that we should move away from the annotations. But I will write up a more detailed post tomorrow on how we might proceed and give the rest of the community a chance to try out what I’m proposing and some more time to discuss the details. Until tomorrow :smile:

14

Back again :smile:

So we agreed that the approach we use now should be replaced, reasons can be found on the Unit Testing Conventions page - Now & The Future - New Style

we agreed on these first TODOs (which I almost completed)

  1. added Eclipse and IntelliJ templates.xml to openmrs-core/tools/src/main/resources for unit test snippets as a replacement for the Generate Test Case Plugin
  2. added a section on how to import and use the templates in the respective IDE setup wiki pages for Eclipse and IntelliJ
  3. Updated https://wiki.openmrs.org/display/docs/Unit+Testing+Conventions to show that a change in unit test convention is on its way and provided some background info (based on the infos posted by @darius here) (I still need to fill a proposition for “Naming Test Methods” which I want us to discuss then, please give me a bit more time :smile:, also I need to update the code examples, and resources on test driven development).

There some more items I would like to address:

ITEM 1

@wyclif mentioned that using the generate test case plugin is convenient because you can write the sentence like a normal English sentence with whitespaces “should return an empty list given null” as opposed to just writing the method name directly as myMethod_shouldReturnAnEmptyListGivenNull. Your also added that devs will not be used to it. But I think devs are very much used to writing camel cased variables, methods so I would not see this as a concern. But I get the convenience factor.

I therefore played a little with templates and IntelliJ has a camelCase() function which can turn a sentence as we wrote it before with whitespaces into what we want the method name to be. But and this defeats the purpose: this only works (or at least I found no other way) when you add a variable for example in a javadoc which contains the sentence which you take as an input to the template function camelCase(MY_VARIABLE) and put that into another variable which feeds the method name. Also this does not work in Eclipse.

So I would not go this route of trying to replace the generate test case plugin. I would drop advocating the plugin, I dont think anyone maintains it anyway. I think we should agree on a common language (I write a proposal in “Naming Test Methods” in the wiki, tomorrow), use code templates that generate the boilerplate and just fill in the method name. Our devs will get used to it and I think other devs are already used to this.

Maybe @lluismf or @raff can also chime in on this, if I am wrong on how its done out there on other projects.

Other objections, thoughts?

ITEM 2

We discussed that once we agreed on the templates, naming conventions and updated the wikis we would point devs to this new convention and only accept new PRs using this style.

Does anyone have an objection to this? Would it make sense to only do this for the master branch?

ITEM 3

We also discussed that it might be good to make a PR which removes all @should’s and @verifies from the javadocs.

This is important because the code serves as an example and currently we have two styles and after the change we would have 3 styles if we dont get rid of the old ones. It happens all the time that new devs use the old style. I also fell into this trap and changed all the tests in radiology to the old style when I started :cry:

I therefore strongly hope that once we agree and finished all above TODOs we can create a PR which cleans the openmrs-core master branch.

Objections? Thoughts? Other branches as well or just master?

That was a lot, :sweat_smile: Thanks for reading!

I ask you to please try to import the templates and use them to see how it goes!!! We can of course tweak them!! I just need your feedback and if someone else has a better template, come forward and lets use it :wink:

I’ll continue working on the wiki and am waiting for your feedback.

2 Likes
15

I don’t think we need to drop the test generator plugin as such because I still think it’s convenient, I believe that if it works for a given IDE version, a dev should be able to continue using it as an option as long as we fix it to not generate the duplications, it should be very easy to fix this. And it’s not like it breaks with every new release of an IDE, it has worked for several eclipse releases over the years, it’s the one for IDEA that is found of breaking and the fixed are usually easy one line code changes.

16

I guess my point is, as long as we have no other mechanism to auto generate these methods names, we should be leave the door open for one to use the test generator plugin or not. If someone wants to generate method names manually that’s fine and if one one wishes to use the plugin it should also be okay.

17

For what I understood of previous messages, we are specifying the same thing 3 times: should annotation, verifies annotation and method name. If the agreement is to remove duplication and leave only the method name, there’s no need to autogenerate it. Therefore the plugin is of no use because the test method is going to be written in the first place.

1 Like
18

I object to the test generator plugin, and think we should drop it. My reason is that the plugin promotes the bad practice of writing a method first and its test later.

Best practice is to do test-driven development, where you write the test first, and then you write the code that will make it pass.

1 Like
19

https://wiki.openmrs.org/display/docs/Unit+Testing+Conventions is now up to date, everyone is more than welcome to make changes/additions!!!

  1. should I now remove the @should/verifies also from https://wiki.openmrs.org/display/docs/Testing ?

  2. then create a commit which cleans openmrs-core master of @should/verifies ?

20

We name our test methods in this scheme: methodName_shouldDoThisGivenThat()

Personally I actually write code like shouldDoThisGivenThat() because when I’m writing the behavior-driven test case I won’t have yet decided the method name I’m going to write.

Did you all feel that it’s important to continue with methodName_should in our conventions?

@teleivo there are a bunch of broken links in the wiki page (e.g. EncounterTest.java http:–dev.openmrs.org-browser-openmrs-trunk-test-api-org-openmrs-test-EncounterTest.java). I haven’t explored why.

1 Like