rethink unit test style to remove duplication

@should annotations don’t show up in the generated javadocs, therefore I see no way it clutters the API. I agree we should get rid of @verifies since it’s duplicating things and that we should not require @should going forward

yes they do, see OpenMRS 2.2.0-SNAPSHOT API go to for ex. ConceptService.getAllConceptProposals

Expected Behaviour: Should return all concept proposals including retired ones when given true …

I summarize, we agree to:

  • remove @Verifies (oldest style)
  • remove @verifies from javadocs (current style)
  • strip “methodName_” from the test method names

(all affects only Test classes)

Do I have your approval to commit these changes to master branch of openmrs-core?

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 [TRUNK-5122] - OpenMRS Issues 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