add: unit test for textutils issue #196 #204

lindsay-stevens · 2016-09-21T09:28:26Z

existing code is working but this verifies it
had to change to class methods because android unit tests require mocking out
android library calls and this is not obvious (or possible?) with static. impact
on callers is minimal at least.

If there is a better way to do this please advise. I haven't done much Java (mostly Python atm) so the way this all works is a bit opaque to me.

- existing code is working but this verifies it - had to change to class methods because android unit tests require mocking out android library calls and this is not obvious (or possible?) with static. impact on callers is minimal at least.

yanokwa · 2016-09-22T12:24:50Z

@srsudar @batkinson Since you two are the testing experts, can you take a quick look at Lindsay's work?

srsudar · 2016-09-22T16:43:18Z

Not sure I'm worthy of that mantle, but I can take a look this weekend.

batkinson

I don't know if the approach you used will be consistent with the testing conventions for Collect going forward, but the fact you took the initiative to create the test is awesome. Thank you.

batkinson · 2016-09-22T17:09:25Z

build.gradle

@@ -4,7 +4,7 @@ buildscript {
        jcenter()
    }
    dependencies {
-        classpath 'com.android.tools.build:gradle:2.1.0'
+        classpath 'com.android.tools.build:gradle:2.1.3'


Commit message should describe the commit contents comprehensively. Updating the gradle tools version should not be part of this commit.

batkinson · 2016-09-22T17:27:11Z

collect_app/src/main/java/org/odk/collect/android/utilities/TextUtils.java

+        public Spanned toHtml(String text) {
+            return Html.fromHtml(text);
+        }
+    }


Changing the API like this smells unless we intend to make it a convention for similar. I suspect we would use Roboelectric or make it an instrumentation test.

This might be a good occasion to continue the discussion on our testing frameworks. This is the kind of thing I would use Robolectric for, but I'm not sure how the group feels about it. I'll post back there about it, as the answer will impact how Lindsay modifies the PR.

batkinson · 2016-09-22T17:28:49Z

gradle/wrapper/gradle-wrapper.properties

-zipStoreBase=GRADLE_USER_HOME
-zipStorePath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-2.13-all.zip
+#Wed Sep 21 15:07:15 AEST 2016


Again, commit contents should be apparent from description. Updating the gradle wrapper should not be part of it.

batkinson · 2016-09-22T17:35:03Z

collect_app/src/test/java/org/odk/collect/android/utilities/TextUtilsTest.java

+        verify(mHtml).toHtml(captor.capture());
+        String input_arg = captor.getValue();
+        assertEquals(input, input_arg);
+    }


Nice and concise test.

lindsay-stevens · 2016-09-23T02:54:33Z

Thanks for reviewing this and the helpful comments. Robolectric seems to have done the trick, so I've rolled back the changes to the main classes.

I was wondering (maybe best for a new ticket) but is there any point to the 4 product flavors (demo, master, snapshot, uitest)? They seem to be identical and just causing the CI build to take 4 times longer.

jbeorse · 2016-09-23T21:07:18Z

@lindsay-stevens The 4 product build flavors exist because Collect currently shares its gradle configuration with the ODK 2.0 tool suite. ODK 2.0 uses the 3 of the build flavors to generate different binaries (Snapshot = new binary for each commit, Demo = synchronized stable points, Master = full release), and the last flavor (uitest) for separating out tests that require graphics rendering (our build server runs headless).

Collect is not currently part of that workflow, so it is possible to remove those flavors if there is community will to do it. I do not anticipate it being a lot of work, but we'd want to check against the common gradle config in this repository: https://github.com/opendatakit/gradle-config/

srsudar · 2016-09-24T06:08:36Z

Jeff, what does Collect depend on in that common repo? Does Collect depend on any shared libraries? I'm not too familiar with shared gradle configs, so this might be an obvious question if you know where to look in that repo.

srsudar · 2016-09-26T00:32:58Z

collect_app/src/test/java/org/odk/collect/android/utilities/TextUtilsTest.java

+    public void textToHtml_CallsHTMLOnRealInput() {
+        String input = "This *bold* markdown is \n- nice,\n- efficient.\n";
+        CharSequence result = TextUtils.textToHtml(input);
+        assertNotNull(result);


I LOVE that we are adding tests, and a huge kudos to you for doing so. I feel like with the change from the original commit (which had the captor business), however, that this method name is no longer appropriate. Right now it's just asserting that the returned result is not null, not that it actually calls the value on the input. Someone could rewrite textToHtml to just return an empty string and this test would still pass.

I think that a better test for this would be something like:

@Test public void textToHtml_CallsHTMLOnRealInput() String arg = "bar"; String expected = "supposedly styled string"; TextUtils tuMock = mock(TextUtils.class); when(tuMock.textToHtml(eq(arg))).thenReturn(expected); String actual = tuMock.textToHtml(arg); verify(tuMock, times(1)).textToHtml(eq(arg)); assertEquals(actual, expected); }

There I'm trying to assert both that the method was called with the argument expected and that we are returning the result of the textToHtml method. The unfortunate thing is that all the methods are static as written. I agree with your original sentiment that this is ugly. I'd vote that these methods either change to non-static and we test like above, or we just change the name of the test to better describe what it's actually doing.

Adding a test framework is a big win, and I'd LGTM a PR like this that lays the groundwork for more tests even if it doesn't add a lot of tests itself.

edit: updated some typos from some local tests I was running.

Thanks @srsudar I think that's a very sensible change. I wanted to do something like that but wasn't sure what the best approach might be. Maybe if @batkinson could confirm I'll update the PR for merge?

Re: gradle configs: maybe we can address this by only running one flavour in CI for collect. I'd imagine there is a way to set that as a build parameter in circle.yml, which I've opened a new ticket for adding (#205) once we're set on this PR.

@lindsay-stevens I appreciate the sentiment, but I can't claim special expertise over @srsudar. However, after looking closely at the code he commented on, I agree with his sentiment (especially with establishing the testing foundation in the code base), but I am not sure I like the alternative offered any better.

My major complaint is that a good test usually makes the issue involved clear. To gauge the relevance of the test I had to reference #196. Unfortunately, the issue isn't clear on the root problem either. So, we (as will others in the future) have to lean on the test code. A good test method name consistent with a concise implementation should be enough. Sam's re-implementation brings the method contents back into alignment with the method name, but the method name does not help to understand the value of the test (other than a html method is called).

I'm still foggy on the purpose of the test, but is there a reason we're diving into the guts of the implementation? For example, could we create a focused test for the defect like so?

@Test public void singleMarkdownReplacement() String input = "This *bold* markdown is \n- nice,\n- efficient.\n"; assertEquals("unexpected markdown-to-html translation", "This bold markdown is \n- nice,\n- efficient.\n", TextUtils.textToHtml(input)); }

Obviously, this test does not help to understand #196, but it is a clear test and it is stable with a change in implementation since we treat the method as a black box. If you have a specific test case and a good description that uniquely corners #196 like this, it would be a significant improvement. If I get a chance, I will try to come up with one myself.

Let me know if my feedback isn't clear.

lindsay-stevens · 2016-09-26T14:38:27Z

Fair points. Thanks for your patience with this. I guess the reason for the test is that since this TextToHtml function is more or less the class's interface, and it doesn't declare that it throws any exceptions, the test nails down the expectation that at least it won't throw an NPE on null input (which is what caused #196). Maybe a more precise test would focus on that, with an assertNotRaises type of test? As far as I can tell it's not possible to actually test the input / output because the Android library functions can't be used in unit tests and robolectric only provides fakes / stubs. If there's some way to do that I think it'd be another useful test though, so there's something testing what the class is expected to produce.

batkinson · 2016-09-26T15:12:06Z

My understanding after reading #196 was that certain inputs caused it to
generate an java.lang.ArrayIndexOutOfBoundsException. However, something
here is misleading. Mitch posted an example of a form, but after testing it
on the latest version of Collect on master, no exception is thrown. I can't
tell if the problem is a bad test form or, because the actual issue
description is unclear, the issue being reported is fixed. In other
projects, I would probably ignore this issue until there is a minimal
example that generates the stack trace. The example form here includes
multiple markdown strings of which their purpose is unclear, so it is not
very helpful.

As for the test feedback I offered, my apologies. I have not used
roboelectric much myself. After writing an example test like I suggested I
noticed the same thing you did - it doesn't really work. I'm not sure if
Sam could offer a way to write this type of test with roboelectric, but
from a verification standpoint if roboelectric doesn't provide an accurate
Android environment it might be better to write this as an instrumentation
test. Alternatively, we could attempt to rewrite the implementation to
enable test, but without test coverage that can span both implementations
we're likely to exchange one set of defects for another.

On Mon, Sep 26, 2016 at 10:38 AM, Lindsay Stevens notifications@github.com
wrote:

Fair points. Thanks for your patience with this.

I guess the reason for the test is that since this TextToHtml function is
more or less the class's interface, and it doesn't declare that it throws
any exceptions, the test nails down the expectation that at least it won't
throw an NPE on null input (which is what caused #196).

Maybe a more precise test would focus on that, with an assertNotRaises type
of test?

As far as I can tell it's not possible to actually test the input / output
because the Android library functions can't be used in unit tests and
robolectric only provides fakes / stubs. If there's some way to do that I
think it'd be another useful test though, so there's something testing what
the class is expected to produce.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#204 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAdPFW-6YnwQSW78mK1gcXDsxKKsB2eNks5qt9jkgaJpZM4KClcd
.

srsudar · 2016-09-26T17:05:12Z

I don't think the problem in this case is actually with robolectric but rather with how we're trying to test the output. As a general rule, robolectric shouldn't just provide fakes/stubs. It generally calls through to either the real Android implementation or a (sometimes imperfect) re-implementation. For example, it looks like they support the android.text.Html class and test its behavior.

I believe the reason @batkinson's test is failing is that the result of Html.fromHtml doesn't include HTML tags, so we shouldn't expect to see them in the output string. It [returns a Spannable object](https://developer.android.com/reference/android/text/Html.html#fromHtml%28java.lang.String, int%29) with the characters styled appropriately according to the input tags. The assertEquals method is comparing two Strings, losing the styling.

Looking out the output from Brent's test, I think the method is actually behaving correctly. Rather than:

This <b>bold</b> markdown is
- nice,
- efficient.

the text is being converted first to this markdown:

<p>This <em>bold</em> markdown is</p><p>- nice,</p><p>- efficient.</p>

and then a Spannable that looks like this in the logs:

This bold markdown is

- nice,

- efficient.

So the assertion is failing but I actually think it's working as expected. You can plug this code into MainMenuActivity's onCreate method and see that indeed it is being styled and outputting the same results we see in the test:

        TextView header = (TextView) findViewById(R.id.main_menu_header);

        String input = "This *bold* markdown is \n- nice,\n- efficient.\n";
        String expected = "This <b>bold</b> markdown is \n- nice,\n- efficient.\n";
        CharSequence actual = TextUtils.textToHtml(input);
        CharSequence fromTags = TextUtils.textToHtml(expected);
        Log.e(t, "actual: " + actual);
        Log.e(t, "fromTags: " + fromTags);
        header.setText(actual);

So, that's a roundabout way of saying that in this case I don't think robolectric is the problem. If we want to test the API rather than the implementation (which in general is of course the way to go), I think we should move markdownToHtml's visibility to protected or package private and perform these tag-based tests on that method. The fact that fromHtml doesn't return a type that supports easy assertions (Spanned looks unpleasant) means I would be ok just asserting that for the textToHtml method we invoke the correct methods and return the result from the correct method.

batkinson · 2016-09-26T17:47:40Z

Thanks Sam. That explains the output I was seeing, and brings my view of
what roboelectric's responsibilities are back to sanity. I agree with your
conclusion as well.

On Mon, Sep 26, 2016 at 1:05 PM, Sam Sudar notifications@github.com wrote:

I don't think the problem in this case is actually with robolectric but
rather with how we're trying to test the output. As a general rule,
robolectric shouldn't just provide fakes/stubs. It generally calls through
to either the real Android implementation or a (sometimes imperfect)
re-implementation. For example, it looks like they support the
android.text.Html class and test its behavior
https://github.com/robolectric/robolectric/blob/78ef7facd8df6c2c2f8dafedfbc2d1af19f3558e/robolectric/src/test/java/org/robolectric/shadows/ShadowHtmlTest.java#L47
.

I believe the reason @batkinson https://github.com/batkinson's test is
failing is that the result of Html.fromHtml doesn't include HTML tags, so
we shouldn't expect to see them in the output string. It returns a
Spannable object
https://developer.android.com/reference/android/text/Html.html#fromHtml(java.lang.String,%20int)
with the characters styled appropriately according to the input tags. The
assertEquals method is comparing two Strings, losing the styling.

Looking out the output from Brent's test, I think the method is actually
behaving correctly. Rather than:

This bold markdown is

nice,

efficient.

the text is being converted first to this markdown:

This bold markdown is
- nice,
- efficient.

and then a Spannable that looks like this in the logs:

This bold markdown is

nice,

efficient.

So the assertion is failing but I actually think it's working as expected.
You can plug this code into MainMenuActivity's onCreate method and see
that indeed it is being styled and outputting the same results we see in
the test:
 TextView header = (TextView) findViewById(R.id.main_menu_header);

 String input = "This *bold* markdown is \n- nice,\n- efficient.\n";
 String expected = "This bold markdown is \n- nice,\n- efficient.\n";
 CharSequence actual = TextUtils.textToHtml(input);
 CharSequence fromTags = TextUtils.textToHtml(expected);
 Log.e(t, "actual: " + actual);
 Log.e(t, "fromTags: " + fromTags);
 header.setText(actual);
So, that's a roundabout way of saying that in this case I don't think
robolectric is the problem. If we want to test the API rather than the
implementation (which in general is of course the way to go), I think we
should move markdownToHtml's visibility to protected or package private
and perform these tag-based tests on that method. The fact that fromHtml
doesn't return a type that supports easy assertions (Spanned looks
unpleasant
https://developer.android.com/reference/android/text/Spanned.html)
means I would be ok just asserting that for the textToHtml method we
invoke the correct methods and return the result from the correct method.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#204 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAdPFVIFSXSmmfbTXppv1IK5Vy-BKkbwks5qt_tJgaJpZM4KClcd
.

yanokwa · 2016-10-05T10:36:57Z

Sounds like we have consensus. @lindsay-stevens Can make the changes @srsudar suggested? And if those changes get @batkinson's LGTM, I think we'd be ready to merge and do a release.

lindsay-stevens · 2016-10-12T01:49:56Z

Sorry it took a little while to address this, I was away for a week. I think this is probably the best we can do in terms of testing the interface, as @srsudar detailed.

Also I realised while writing the descriptions that #196 isn't exactly what this is all about. I think I just saw "TextUtils" in the description and assumed it was the same problem. In fact it's the NPE issue I mentioned over in getodk/getodk#1247.

srsudar · 2016-10-12T23:36:07Z

collect_app/src/test/java/org/odk/collect/android/utilities/TextUtilsTest.java

+     * In the real app this call would produce the following string, but
+     * because the implementation uses android.Html.fromHtml, we end up with
+     * robolectric's implementation of fromHtml which isn't exactly the same.
+     * See opendatakit/collect#204.


This comment isn't quite right, I don't think. My comment in the discussion tries to explain why. Robolectric is doing the right thing. TextUtils.textToHtml() is returning a Spannable with HTML-like tag styling applied, it isn't converting markdown to HTML tags.

The "Correct with a capital C" way to test this method, I think, would be to remove the static from these methods, mock out markdownToHtml, and assert that textToHtml passes its input to markdownToHtml and returns the result of that method.

Is that something you're willing/able to do? If not, I would be ok removing this method and just keeping the textToHtml_BouncesNullInput method, as it would be a nice simple way to add a testing framework and is the functionality you started this PR to test.

I read over your earlier comment and my understanding is that if we nest some debug code inside an activity and run it (thereby calling the Android implementation), we get a HTML styled string in the logs:

"This bold markdown is \n- nice,\n- efficient.\n"

But in the context of this test (thereby calling the Robolectric implementation), we get a string that's not quite the same:

"This bold markdown is\n\n- nice,\n\n- efficient.\n\n"

Which is what I'm trying to convey in the test comment.

Either way, I think it's clear that it's not possible to perfectly test this class' interface as it is right now. But we can at least establish that the crash described in getodk/getodk#1247 is fixed, which is the goal of this PR. The criteria for that is that null input doesn't cause a crash, and that in the course of meeting that requirement we retain normal behaviour: non-null input causes some kind of Markdown to HTML processing to happen (disregarding the nature / correctness of that processing).

I agree this all could / should be a cleaner, but as you say it requires so rework of the TextUtils class which would be something for a separate PR. That might end up involving reworking of one or both of these tests, but who knows what will happen when we actually try to do it :)

I think that my confusing wording and variables names made that unclear. Sorry. The output of markdownToHtml is the bold bit. textToHtml converts HTML tags to a styled Spannable, which when logged loses the styling. So I think that Robolectric is behaving the same way the Android platform does.

I agree however that this is really neither here nor there. I'd be fine removing this test altogether, leaving only the non-null test, which as you say was the point of the PR to begin with.

Let's not let perfect be the enemy of good. @lindsay-stevens remove the HTML test and then I'll be happy to merge.

- properly testing md/HTML processing requires some refactoring - dev team prefers to not have a superficial test for it in the meantime

add: unit test for textutils issue getodk#196

5e26b7b

- existing code is working but this verifies it - had to change to class methods because android unit tests require mocking out android library calls and this is not obvious (or possible?) with static. impact on callers is minimal at least.

batkinson reviewed Sep 22, 2016

View reviewed changes

lindsay-stevens added 4 commits September 23, 2016 10:11

chg: rollback build file changes

3293917

add: test library dependencies

aff0988

chg: robolectric to mock android instead

6ef58fb

chg: remove now-ununsed argument captor

39da153

lindsay-stevens mentioned this pull request Sep 23, 2016

add circle.yml so CI test output is visible #205

Closed

srsudar reviewed Sep 26, 2016

View reviewed changes

chg: elaborate test description, test impl as best possible

90b6664

srsudar reviewed Oct 12, 2016

View reviewed changes

chg: remove expected function test, to be done later

2112a99

- properly testing md/HTML processing requires some refactoring - dev team prefers to not have a superficial test for it in the meantime

yanokwa merged commit b592015 into getodk:master Oct 24, 2016

lognaturel mentioned this pull request Feb 7, 2017

Start programmatic testing of core functionality #392

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add: unit test for textutils issue #196 #204

add: unit test for textutils issue #196 #204

lindsay-stevens commented Sep 21, 2016

yanokwa commented Sep 22, 2016

srsudar commented Sep 22, 2016

batkinson left a comment

batkinson Sep 22, 2016

batkinson Sep 22, 2016

srsudar Sep 23, 2016

batkinson Sep 22, 2016 •

edited

batkinson Sep 22, 2016

lindsay-stevens commented Sep 23, 2016

jbeorse commented Sep 23, 2016

srsudar commented Sep 24, 2016

srsudar Sep 26, 2016 •

edited

lindsay-stevens Sep 26, 2016

batkinson Sep 26, 2016 •

edited

lindsay-stevens commented Sep 26, 2016 via email

batkinson commented Sep 26, 2016

srsudar commented Sep 26, 2016

batkinson commented Sep 26, 2016

yanokwa commented Oct 5, 2016

lindsay-stevens commented Oct 12, 2016

srsudar Oct 12, 2016

lindsay-stevens Oct 13, 2016

srsudar Oct 13, 2016

yanokwa Oct 15, 2016

add: unit test for textutils issue #196 #204

add: unit test for textutils issue #196 #204

Conversation

lindsay-stevens commented Sep 21, 2016

yanokwa commented Sep 22, 2016

srsudar commented Sep 22, 2016

batkinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

batkinson Sep 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lindsay-stevens commented Sep 23, 2016

jbeorse commented Sep 23, 2016

srsudar commented Sep 24, 2016

srsudar Sep 26, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

batkinson Sep 26, 2016 • edited

Choose a reason for hiding this comment

lindsay-stevens commented Sep 26, 2016 via email

batkinson commented Sep 26, 2016

srsudar commented Sep 26, 2016

batkinson commented Sep 26, 2016

yanokwa commented Oct 5, 2016

lindsay-stevens commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

batkinson Sep 22, 2016 •

edited

srsudar Sep 26, 2016 •

edited

batkinson Sep 26, 2016 •

edited