rethink unit test style to remove duplication

I think we should keep ‘methodName_’ prefix around but shouldn’t require it since it helps the reader to quickly know what method is getting tested in case it’s included

I agree with Wyclif. I would not rename all the existing test methods. But going forwards people can write tests without the “methodName_” part.

I hope that not changing the existing test method names will make future merging and backporting easier. And also, existing ones were written such that the name should read correctly with the method name. If we strip that programmatically, it will make the test method names less clear.

(Going forwards people can write them to have readable names in the new style.)

good point!

updated https://issues.openmrs.org/browse/TRUNK-5122 to match our agreement on:

  • remove @Verifies (oldest style)
  • remove @verifies from javadocs (current style)

do I have your permission to work on the issue?

I vote in favour! :smile:

+1, go ahead!

merged.

  • should I add a checkstyle rule that flags the use of org.openmrs.test.Verifies ? (this can help reviewers as codacy will show it on PRs, guess its easy to overlook)
  • should org.openmrs.test.Verifies be deprecated?

My vote is for Deprecated

Am in favour for both. :slight_smile:

so we get people away from it. I mean the annotation can be removed at a time far in the future but its important we advocate against it from now on.

@darius and @wyclif your votes on this?

I vote yes on both.

@teleivo, you don’t actually need to get everyone’s explicit votes on everything. You can take a “lazy consensus” approach. Especially in cases like this where the answer is obvious. :wink:

I wrote more about that here: Code Quality Leader: Ivo Ulrich

ok, thank you very much! just don’t want to run over anyone and give a chance to object :smile: but I like this “lazy consensus” approach, still makes this possible but speeds things up.

And you don’t need to merge your own pull requests, you can push directly to upstream :slight_smile:

haven’t dared till now, I am a cautious type :joy:

@teleivo i do pull requests only when i would like some one else to review before i can merge. But if am going to merge immediately, then i just do like @lluismf :smile:

I agree with what all the others have said

1 Like

Thanks @teleivo for driving this effort! I’m happy to see the outcomes:

  • Get rid of @verifies and use code coverage tools instead
  • Move away from @should in favor of TDD (writing tests first)
  • Don’t throw away information in existing @should in javadocs
  • Don’t require tested method names as prefix for test methods
  • Keep Dan North’s BDD approach of starting test names with “should”

Let me know if I got these wrong.

BTW… one of the reasons we ended up with “@should” method was stumbling on Dan North’s BDD and completely buying into the power of starting with “should” instead of “test” when describing tests.

<off_topic> Have there been equal advancements in the world of mocks? I’d love to see us embracing mocks for most of our tests. :slight_smile:
</off_topic>

1 Like

It doesn’t require any real advancements here…

It just requires people to do work to rewrite tests. Here’s a wiki page.

related to this, and making it easy to write unit tests, I suggest we tackle:

thanks guys, this was a good dialog, am happy about the votes. let us creep into TDD :slight_smile:

BTW, except if i missed it, https://wiki.openmrs.org/display/docs/Unit+Testing+Conventions or this dialog isn’t straight on the new naming convention for unit test method names! i hope getPatient_anExistingId_shouldReturnItsPatient(), or methodName_shouldSentence() isn’t offensive! is it upto each developer to choose what they prefer. some TDD advocates have something like testMethodName others something else. someone could clarify.

thanks