rethink unit test style to remove duplication

you are correct that this would be good. we didnt talk about dropping that as well, but I would be in favor as well! others? @wyclif @dkayiwa @raff @lluismf

true, thats the last section I still have to clean up, then the other testing pages. the wiki contains such a huge amount of unmaintained pages with broken links and outdated info :frowning:

I want to think true test driven development is not 100% attainable, if you were to write the test code first then the test wouldnā€™t compile since the method you need to call/test would be none existent. The truth is that you still need some sort of a stub method or an interface for the test to compile which is all the test generator requires to work.

that is the first phase of the cycle: RED (write just enough of a test to that fails), GREEN (write just enough of implementation code that passes the test), REFACTOR

when starting you write a test for something that does not exist, the test will hence fail since it doesnt even compile

1 Like

Hmmā€¦ I donā€™t think fail means not compile

@wyclif, hereā€™s the top google hit I got for test driven development example video: https://www.youtube.com/watch?v=O-ZT_dtlrR0

In the first 1:30 of it, he shows the standard first TDD step, which is that you write a test that does not initially compile. (I didnā€™t watch any further.) This is in fact standard TDD practice, and TDD is 100% attainable. :slight_smile:

As far as dropping the method name is concerned, no objections from me. :wink:

1 Like

I think we are also making a huge assumption that we only write tests for new methods or classes, sometimes itā€™s bug fixes where the method and class already exist. I still think TDD is a hoax when it comes to an agile and collaborative development environment where possibly dev1 is expected to write the tests for methods that will be implemented by another dev2, it means dev1 canā€™t commit tests that donā€™t compile or even fail because CI builds would fail yet CI should technically always be green. I guess my point is TDD is good but lets not get carried away too much by it, as the old saying goes, too much of anything is always bad.

I dont think that any change we discussed until now affects this in a negative way. If there is a bug just like before you add a test + a fix. The only difference is that after the change you write less javadocs and a shorter test method name.

Its done in agile, collaborative environments. Also when devs pair there is a game where one writes the test, the other the implementation.

Does this case ever happen that dev1 writes tests (I guess adds @Ignore so they dont run and build passes) and commits them to openmrs-core and then dev2 writes the implementation + removes @Ignore and commits that to openmrs-core? (Edit: I mean doing this on the master branch. You can of course do this on any feature branch, since they are not configured to run through CI)

The goal in my view is:

To put in place conventions that let people use TDD and not hinder them if they want to use this method. After these changes nobody is forced to use TDD. Its not a debate about whether to use TDD or not. You can still write code first and then tests. We just get rid of all this duplication and end up writing less lines of code.

I also think that this topic has a few other important aspects to it. We are doing something very unusual here. And I havent heard convincing arguments for what it really is that justifies this style/where is the value?. It might be that this is a pioneering new development style but I just dont see it.

These other aspects are: we teach new devs this style which I dont think is good, we possibly put of more experienced devs because who wants to be forced to use this plugin or write all this duplication by hand? If you want to do TDD which is used a lot out there, its hard within OpenMRS right now.

1 Like

I agree this is not really about TDD and am not saying no to what the convention should be. What I said initially was that because we are changing the convention doesnā€™t mean we need to drop the plugin, all am saying is that it can co-exist with the new convention

I guess the question is, should we drop the @should annotation or not? This is what would ultimately determine if the plugin should be dropped or not. Keeping the annotations implies the plugin can remain as an option to auto generate test methods while dropping the annotation implies letting go of the plugin because it requires it to work.

I think we should drop it.

From the given history, the @should annotations were put in place to accomplish what they failed to. And now that we have tools to accomplish what the @should annotations were meanā€™t for, but failed to, i think it makes sense to just drop them.

Yes about removing the annotations. They are mostly noise, example:

Is there are any tool to remove lines from a file (using a regexp) ? Doing it by hand could be tedious. I believe that AWK is good for that, but with source control involved Iā€™m not so sure.

Iā€™ve prepared a little script using some sed commands which will do most of the work. a few Test classes need some extra care. but as soon as I get the go Iā€™ll take care of it and make a PR to the master branch :blush:

1 Like

SED + delete original + rename new ? GIT will be happy with that?

I disagree that it is mostly noise; in many cases it helps to document how the code is intended to be used as well as expectations for edge cases. Will this information be preserved anywhere? It seems like @wyclifā€™s suggestion to implement the more traditional TDD-style tests alongside the existing annotation-based tests allows us to keep programmer ā€œdocumentationā€ that weā€™ve accumulated over the years while also updating the testing style.

When I was getting started with omrs I found the @should annotations very helpful to quickly understand how the various services were supposed to work and how I should use them; it seems foolish to simply throw away all of that information.

1 Like

This is a fair point.

I would vote that we leave the existing @should statements within the javadoc of source code, because itā€™s a net positive.

But we also (a) remove all the @verifies from the test code, because what it was trying to do is better handled by a code coverage tool, and (b) we donā€™t promote writing more @should statements going forward.

I agree that there is some useful information in there. The information would not be lost because the same sentences are in the tests but yes they would not end up in the APIā€™s javadocs. Just as a note: its common practice to write if null is accepted as a value next to the parameters in the javadoc, which we dont do since we use @shoulds. Thats also a thing we do different which in my view is more difficult as a dev.

I am ok with keeping the @should statements but not forcing anyone to write them.

I agree that it can be useful, but only if itā€™s updated. Iā€™m old fashioned and prefer to keep the requirements in a separate place (technical document, wiki ā€¦) instead of embedded in the code. IMHO it clutters the API, even if the information is valuable and is not obsolete. Itā€™s a matter of taste I guess.