Is this unit test too trivial?

Ask OpenMRS
,
1

I would like to contribute and thought I start with something easy yet useful like unit tests.

According to coverall WebUtil could do with some more tests but I’m wondering if they might be to trivial. There are one line methods like

	/**
	 * Encodes for (X)HTML text content and text attributes.
	 *
	 * @param s
	 * @return Encoded String
	 */
	public static String escapeHTML(String s) {
		return Encode.forHtml(s);
	}

To make sure that Encode.forHtml() is actually called I could write a test like

	@Test
	public void shouldEscapeHtml() {
		String html = "<html>&auml;&szlig;&ccedil;</html>";
		String expectedEncodedHtml = "&lt;html&gt;&amp;auml;&amp;szlig;&amp;ccedil;&lt;/html&gt;";
		Assert.assertEquals(expectedEncodedHtml, WebUtil.escapeHTML(html));
	}

But should Encode.forHtml() ever change (or one of the other methods used) the test would fail so this might be a better solution:

	@Test
	public void shouldEscapeHtml() {
		String html = "<html>&auml;&szlig;&ccedil;</html>";
		Assert.assertEquals(Encode.forHtml(html), WebUtil.escapeHTML(html));
	}

Or is this too trivial? It assumes that someone changing the code might forget to return the correct or any value. Perhaps not so likely with a one liner?

And how would I contribute unit tests? Create a Jira ticket, have it assigned to me, create a pull request?

2

@farndt welcome to the OpenMRS Community and thank you so much for starting to contribute immediately. :slight_smile:

Since that method just delegates to an external library, i would not recommend testing it from the OpenMRS code base. I would rather suggest that you write tests for OpenMRS specific business logic like for this ticket: https://issues.openmrs.org/browse/TRUNK-5068

3

Thanks for the quick reply. I’ll have a look at it.

4

Actually I’ve just found the proof that this kind of unit test might make sense.

In class

org.openmrs.module.webservices.rest.OpenmrsPathMatcher

a delegate is used and while all methods follow the pattern

	public boolean method(String pattern, String path) {
		return this.delegate.method(pattern, path);
	}

one does not:

	public boolean matchStart(String pattern, String path) {
		return this.matchStart(pattern, path);
	}

SpotBugs says

Bug : There is an apparent infinite recursive loop in org.openmrs.module.webservices.rest.OpenmrsPathMatcher.matchStart(String, String)

This method unconditionally invokes itself. This would seem to indicate an infinite recursive loop that will result in a stack overflow.

Rank : Scary (9), confidence : High *Pattern : IL_INFINITE_RECURSIVE_LOOP * Type : IL, Category : CORRECTNESS (Correctness)

I guess I should creat an issue?

5

That is a bug. Should be replaced with: return this.delegate.matchStart(pattern, path);

So feel free to create a ticket for it. Nice catch. :slight_smile: