[rfc][icedtea-web] PluginAppletViewer unit tests with test extensions for mocking plugin input&output pipes
Jiri Vanek
jvanek at redhat.com
Mon Apr 22 02:17:58 PDT 2013
On 04/19/2013 08:31 PM, Adam Domurad wrote:
> Hopefully more tasteful this time around :-)
Ok. Walk through and I think it can go in. Two nits:
1)Maybe some test to new test-extension classes would be worthy:(
2) checking the stared Test utilities methods.. When seeing them separated ... well can be as
it is, but (although I previously said it in reverse way) maybe they are rally woorthy to be in
separate file in test extensions. But as you are confirming my previous suspicions then it is
probably ok and do your best.
J.
>
> On 04/02/2013 09:46 AM, Jiri Vanek wrote:
>> On 03/11/2013 10:21 PM, Adam Domurad wrote:
>>> This is an attempt to enable unit testing of hard-to-test parts of icedtea-web.
>>
>> I'm not big fan of mocking so please do not listen to me 100%
>> But maybe instead of mocking, this patch can be considered as plugin pipes tests:)
>
> I see it like this: It tests completely the Java-side of responsibilities, only the C++ side is
> mocked. In fact as far as Java side is concerned these tests are fairly complete.
>
>>
>>> It introduces a new class 'PluginPipeMock' that allows the requests coming from the plugin to be
>>> read one-by-one, and the ability to send mocked replies.
>>
>> Can't be also the replies be partially from IcedTeaPlugin.so? Cant you connect through JNI?
>> //maybe it will not be useful at all...
>
> Technically this would just be pushing the mocking further (ie, we'd need a Javascript engine
> accessible from C++ side to do anything meaningful, or completely mock one). While a worthy task,
> this is out of the scope of this patch.
>
>>
>>
>>> This is possible thanks to that fact that PluginStreamHandler takes arbitrary and
>>> InputStream&OutputStream.
>>>
>>> Attached are unit tests for PluginAppletViewer.eval, PluginAppletViewer.call,
>>> PluginAppletViewer.toString and PluginAppletViewer.JavascriptFinalize.
>>> While the javascript-related functionality is mocked, the responsibilities from the Java-side of
>>> things are tested.
>>>
>>> It may be worth looking into using iostream's on the C++ side and mocking them similarly (for the
>>> unfamiliar, they are also classes that can be inherited from, so a similar approach can be taken).
>>> Even better would be to provide the C++ side with some NPAPI-enabled javascript engine to interface
>>> with so we can fully test the javascript stuff with unit tests.
>>>
>>> ChangeLog:
>>> 2013-XX-XX Adam Domurad <adomurad at redhat.com>
>>>
>>> * Makefile.am
>>> (stamps/test-extensions-compile.stamp): Make plugin classes available
>>> to test extensions
>>> * tests/test-extensions/net/sourceforge/jnlp/AsyncCallWithTimeout.java:
>>> New, helper for doing asynchronous calls with a given timeout. Returns
>>> an exit code.
>>> * tests/test-extensions/sun/applet/PluginPipeMessages.java: New, helper
>>> for verifying plugin messages and getting the reference number.
>>> * tests/netx/unit/sun/applet/PluginAppletViewerTest.java: New, uses
>>> PluginPipeMock to test the javascript requests to the plygin.
>>> * tests/test-extensions/sun/applet/mock/PluginPipeMock.java: New, helper
>>> for getting the plugin requests and mocking the replies.
>>
>> The whole concept is quite complicated....
>> From need of time-out as separate class across s mocking of input/output messages...
>> Maybe some object which will generate the strings from setted values?
>> I'm aware of PluginPipeMessages, but how generic it is? Can it be more simple? More "user guiding" ?
>
> Not as generic as I thought it was on further thought. Included in the main test.
>
>>
>>>
>>> test-liveconnect-messages.patch
>>
>>
>> There are sometimes unused imports, please fix.
>>>
>>>
>>> diff --git a/Makefile.am b/Makefile.am
>>> --- a/Makefile.am
>>> +++ b/Makefile.am
>>> @@ -758,7 +758,7 @@ stamps/test-extensions-compile.stamp: st
>>> ln -s $(TEST_EXTENSIONS_DIR) $(TEST_EXTENSIONS_COMPATIBILITY_SYMLINK);
>>> $(BOOT_DIR)/bin/javac $(IT_JAVACFLAGS) \
>>> -d $(TEST_EXTENSIONS_DIR) \
>>> - -classpath $(JUNIT_JAR):$(NETX_DIR)/lib/classes.jar \
>>> + -classpath
>>> $(JUNIT_JAR):$(NETX_DIR)/lib/classes.jar:$(abs_top_builddir)/liveconnect/lib/classes.jar \
>>> @test-extensions-source-files.txt && \
>>> mkdir -p stamps && \
>>> touch $@
>>
>> this can go in anyway.
>>
>>> diff --git a/tests/netx/unit/sun/applet/PluginAppletViewerTest.java
>>> b/tests/netx/unit/sun/applet/PluginAppletViewerTest.java
>>> new file mode 100644
>>> --- /dev/null
>>> +++ b/tests/netx/unit/sun/applet/PluginAppletViewerTest.java
>>> @@ -0,0 +1,155 @@
>>> +package sun.applet;
>>> +
>>> +import static org.junit.Assert.assertEquals;
>>> +import static org.junit.Assert.assertTrue;
>>> +import net.sourceforge.jnlp.AsyncCallWithTimeout;
>>> +import net.sourceforge.jnlp.AsyncCallWithTimeout.ExitStatus;
>>> +
>>> +import org.junit.Test;
>>> +
>>> +import sun.applet.mock.PluginPipeMock;
>>> +
>>> +public class PluginAppletViewerTest {
>>> +
>>> + static private PluginPipeMock installPipeMock() throws Exception {
>>> + AppletSecurityContextManager.addContext(0,
>>> + new PluginAppletSecurityContext(0, false /* no security */));
>>> +
>>> + PluginPipeMock pipeMock = new PluginPipeMock();
>>> +
>>> + // TODO: Ensure the thread this creates is stopped
>>
>> quite nasty todo
>
> But in the end not so bad :-)
> Found a trick to do it: Put initialization in a specific thread group & all threads will be
> automatically part of this or child thread group.
> We can then stop this thread group at the end.
> Set setup portion of test.
>
>>
>>> + PluginStreamHandler streamHandler = new PluginStreamHandler(
>>> + pipeMock.getInputStream(), pipeMock.getOutputStream());
>>> + PluginAppletViewer.setStreamhandler(streamHandler);
>>> + PluginAppletViewer
>>> + .setPluginCallRequestFactory(new PluginCallRequestFactory());
>>> +
>>> + streamHandler.startProcessing();
>>> +
>>> + return pipeMock;
>>> + }
>>> +
>>> + /*
>>> + * Helpers for manipulating the object mapping using to refer to objects in
>>> + * the plugin
>>> + */
>>> + private static Object getStoredObject(int id) {
>>> + return PluginObjectStore.getInstance().getObject(id);
>>> + }
>>
>> never used, why? Looks quite good test to get the stored object.
>
> It was used in PipeMessages, but duplicated. It is used now (because PipeMessages is no more).
>
>>
>>> +
>>> + private static int storeObject(Object obj) {
>>> + PluginObjectStore.getInstance().reference(obj);
>>> + return PluginObjectStore.getInstance().getIdentifier(obj);
>>> + }
>>> +
>>> + @Test
>>> + public void testJavascriptCall() throws Exception {
>>> + final PluginPipeMock pipeMock = installPipeMock();
>>> +
>>> + /* JS call parameters */
>>> + final int jsObjectID = 0;
>>
>> Always 0, can't it be hidden?
>
> This object ID is an arbitrary choice and thus I named it for clarity. I would make it a static
> variable but I'm afraid it will seem like a 'magic value'.
>
>>
>>> + final String callName = "testfunction";
>>> + final Object[] arguments = { "testargument", 1 }; // Arbitrary objects
>>> +
>>> + /* JS call result */
>>> + final Object[] returnValue = { null };
>>> +
>>> + AsyncCallWithTimeout call = new AsyncCallWithTimeout(new Runnable() {
>>> + public void run() {
>>> + returnValue[0] = PluginAppletViewer.call(jsObjectID, callName,
>>> + arguments);
>>> + }
>>> + });
>>> +
>>> + String message = pipeMock.getNextRequest();
>>> +
>>> + int reference = PluginPipeMessages.parseAndCheckJSCall(message,
>>> + jsObjectID, callName, arguments);
>>> +
>>> + Object expectedReturn = new Object();
>>> + pipeMock.sendResponse("context 0 reference " + reference
>>> + + " JavaScriptCall " + storeObject(expectedReturn));
>>> +
>>
>> So much calls :(
>
> I have tried somewhat to make them more managable, to the extent that it was possible.
>
>>
>>> + /* Check if we returned correctly (and have the right value) */
>>> + assertEquals(ExitStatus.FINISHED_CORRECTLY, call.joinAndGetExitStatus());
>>> + assertEquals(expectedReturn, returnValue[0]);
>>> + }
>>> +
>>> + @Test
>>> + public void testJavascriptEval() throws Exception {
>>> + final PluginPipeMock pipeMock = installPipeMock();
>>> +
>>> + /* JS eval parameters */
>>> + final int jsObjectID = 0;
>>> + final String callName = "testfunction";
>>> +
>>> + /* JS eval result */
>>> + final Object[] returnValue = { null };
>>> +
>>> + AsyncCallWithTimeout call = new AsyncCallWithTimeout(new Runnable() {
>>> + public void run() {
>>> + returnValue[0] = PluginAppletViewer.eval(jsObjectID, callName);
>>> + }
>>> + });
>>> +
>>> + String message = pipeMock.getNextRequest();
>>> +
>>> + int reference = PluginPipeMessages.parseAndCheckJSEval(message,
>>> + jsObjectID, callName);
>>> +
>>> + Object expectedReturn = new Object();
>>> + pipeMock.sendResponse("context 0 reference " + reference
>>> + + " JavaScriptEval " + storeObject(expectedReturn));
>>> +
>>> + /* Check if we returned correctly (and have the right value) */
>>> + assertEquals(ExitStatus.FINISHED_CORRECTLY, call.joinAndGetExitStatus());
>>> + assertEquals(expectedReturn, returnValue[0]);
>>> + }
>>> +
>>> + @Test
>>> + public void testJavascriptFinalize() throws Exception {
>>> + final PluginPipeMock pipeMock = installPipeMock();
>>> +
>>> + final int jsObjectID = 0;
>>> + AsyncCallWithTimeout call = new AsyncCallWithTimeout(new Runnable() {
>>> + public void run() {
>>> + PluginAppletViewer.JavaScriptFinalize(jsObjectID);
>>> + }
>>> + });
>>> +
>>> + String message = pipeMock.getNextRequest();
>>> +
>>> + int reference = PluginPipeMessages.parseAndCheckJSFinalize(message,
>>> + jsObjectID);
>>> +
>>> + pipeMock.sendResponse("context 0 reference " + reference
>>> + + " JavaScriptFinalize ");
>>> +
>>> + /* Check if we returned correctly (and have the right value) */
>>> + assertEquals(ExitStatus.FINISHED_CORRECTLY, call.joinAndGetExitStatus());
>>> + }
>>> +
>>> + @Test
>>> + public void testJavascriptToString() throws Exception {
>>> + final PluginPipeMock pipeMock = installPipeMock();
>>> +
>>> + final int jsObjectID = 0;
>>> + AsyncCallWithTimeout call = new AsyncCallWithTimeout(new Runnable() {
>>> + public void run() {
>>> + PluginAppletViewer.javascriptToString(jsObjectID);
>>> + }
>>> + });
>>> +
>>> + String message = pipeMock.getNextRequest();
>>> +
>>> + int reference = PluginPipeMessages.parseAndCheckJSToString(message,
>>> + jsObjectID);
>>> +
>>> + String expectedReturn = "testreturn";
>>> + pipeMock.sendResponse("context 0 reference " + reference
>>> + + " JavaScriptToString " + storeObject(expectedReturn));
>>> +
>>> + /* Check if we returned correctly (and have the right value) */
>>> + assertEquals(ExitStatus.FINISHED_CORRECTLY, call.joinAndGetExitStatus());
>>> + }
>>> +}
>>> \ No newline at end of file
>>
>> Well the best on this patch, is that plugin applet viewr got some coverage by unittests :)
>> And also it is reason why I do not want to let this idea die.
>
> When was this great patch in danger of dying ? :-))
>
>>
>>> diff --git a/tests/test-extensions/net/sourceforge/jnlp/AsyncCallWithTimeout.java
>>> b/tests/test-extensions/net/sourceforge/jnlp/AsyncCallWithTimeout.java
>>> new file mode 100644
>>> --- /dev/null
>>> +++ b/tests/test-extensions/net/sourceforge/jnlp/AsyncCallWithTimeout.java
>>
>> I don't like this class to much :((
>> But I agree that this code will be quite repeated... We should try to find soemthing better. (not
>> more generic in this case :) )
>
> I came up with something more tasteful, I think. Actually, it ended up being more generic, but that
> was a natural result of cleaning up the API.
>
>>
>>> @@ -0,0 +1,75 @@
>>> +package net.sourceforge.jnlp;
>>> +
>>> +/**
>>> + * Creates an async call with a certain timeout. It takes a runnable and allows
>>> + * joining & querying the exit status.
>>> + *
>>> + * It starts immediately after creation.
>>> + */
>>> +public class AsyncCallWithTimeout {
>>> + public enum ExitStatus {
>>> + FINISHED_CORRECTLY, THREW_EXCEPTION, TIMED_OUT
>>> + }
>>
>> balh :) Enum for testing the result? I don't like it :(
>
> Good point, gone.
>
>>
>>
>>> +
>>> + private Thread handler;
>>> + private Runnable task;
>>> + private long timeout;
>>> + private ExitStatus exitStatus;
>>> +
>>> + public AsyncCallWithTimeout(Runnable task) {
>>> + this(task, 1000); // Default timeout is 1 second
>>> + }
>>> +
>>> + public AsyncCallWithTimeout(Runnable task, long timeout) {
>>> + this.task = task;
>>> + this.timeout = timeout;
>>> + this.handler = new HandlerThread();
>>> + this.handler.start();
>>> + }
>>
>> Thread should not be statrted in constructor. Just thinking about delegating the method start. But
>> it can make the things even more messy and hard to understand.
>
> Agreed. Helper static method is good compromise, see startWithTimeOut in update.
>
>>> +
>>> + public ExitStatus joinAndGetExitStatus() throws InterruptedException {
>>> + handler.join();
>>> + return exitStatus;
>>> + }
>>> +
>>> + private class HandlerThread extends Thread {
>>> + @Override
>>> + public void run() {
>>> + // We deduce that we threw an exception if we did not
>>> + // time-out nor complete correctly.
>>> + exitStatus = ExitStatus.THREW_EXCEPTION;
>>> +
>>> + Thread thread = new Thread() {
>>> + @Override
>>> + public void run() {
>>> + try {
>>> + task.run();
>>> + exitStatus = ExitStatus.FINISHED_CORRECTLY;
>>> + } catch (Exception e) {
>>> + exitStatus = ExitStatus.THREW_EXCEPTION;
>>> + e.printStackTrace();
>>
>> I would rather see this rethrown out. (Tofind way how to rethrow this is even better then store it)
>
> Very good point, and easily done.
>
>>
>>> + }
>>> + handler.interrupt(); // Finish early
>>> + }
>>> + };
>>> +
>>> + thread.start();
>>> +
>>> + try {
>>> + Thread.sleep(timeout);
>>> + } catch (InterruptedException e) {
>>> + // Finish early
>>> + return;
>>
>> Some special ExitStatus here?
>
> Exit statuses gone
>
>>
>>> + }
>>> +
>>> + if (thread.isAlive()) {
>>> + exitStatus = ExitStatus.TIMED_OUT;
>>
>> Maybe throw an exception?
>
> Good idea, done.
>
>>
>>> + }
>>> +
>>> + // Make sure the thread is finished
>>> + while (thread.isAlive()) {
>>> + thread.interrupt();
>>
>> argh.. but sure:(
>
> I don't think it is too bad.
>
>>> + }
>>> + }
>>> + }
>>> +}
>
>>> [..snip..]
>>> + * the reference number
>>> + */
>>> + public static int parseAndCheckJSToString(String message, int contextObjectID) {
>>> + int expectedLength = 6;
>>> + return parseAndCheckJSMessage(message, expectedLength, "ToString", contextObjectID);
>>> + }
>>
>> Those methods are strongly single-purpose so I would expect them in Tets where are used. Are they
>> about to be reused? . But at least serves as examples of usage of this class.
>
> Originally I thought they might be reused, but I can't really think of any other classes that would
> use them actually. Merged into test.
>
>>
>>> +}
>>> diff --git a/tests/test-extensions/sun/applet/mock/PluginPipeMock.java
>>> b/tests/test-extensions/sun/applet/mock/PluginPipeMock.java
>>> new file mode 100644
>>> --- /dev/null
>>> +++ b/tests/test-extensions/sun/applet/mock/PluginPipeMock.java
>>> @@ -0,0 +1,120 @@
>>> +package sun.applet.mock;
>>> +
>>> +import java.io.IOException;
>>> +import java.io.InputStream;
>>> +import java.io.OutputStream;
>>> +import java.io.StringReader;
>>> +import java.util.ArrayDeque;
>>> +import java.util.Deque;
>>> +import java.util.List;
>>> +
>>> +/**
>>> + * Helper for getting an input & output stream for use with PluginStreamHandler.
>>> + * Provides a convenient way of reading the Java requests and sending mocked plugin responses.
>>> + *
>>> + * The handling of these requests should be done on a different thread from the tested method,
>>> + * as icedtea-web will block waiting for a reply after sending a request.
>>> + */
>>> +public class PluginPipeMock {
>>> + private PluginInputPipeMock inputStream = new PluginInputPipeMock();
>>> + private PluginOutputPipeMock outputStream = new PluginOutputPipeMock();
>>> +
>>> + public PluginPipeMock() {
>>> + }
>>> +
>>> + public InputStream getInputStream() {
>>> + return inputStream;
>>> + }
>>> +
>>> + public OutputStream getOutputStream() {
>>> + return outputStream;
>>> + }
>>> +
>>> + public String getNextRequest() {
>>> + return outputStream.getNextRequest();
>>> + }
>>> +
>>> + public void sendResponse(String response) {
>>> + inputStream.sendReponse(response);
>>> + }
>>> +
>>
>>
>> Do we really need to override so ..complexly both basic streams?
>> Cant be better - more readable - solution found?
>
> I tried. I'm not sure how much better it is, but I tried to make it more clear in its intent & usage
> of multi-threaded queues. Suggestions welcome of course!
>
>>> +
>>> + /* Blocks until we get a line of output. */
>>> + String getNextRequest() {
>>> + while (true) {
>>> + synchronized (this) {
>>> + String line = lines.pollFirst();
>>> + if (line != null) {
>>> + return line;
>>> + }
>>> + }
>>> + }
>>> + }
>>> + }
>>> +}
>>>
>>
>>
>> Don't take my criticism to hard, this is great stuff and Even if it will not be changed the
>> benefit of having PluginAppletViewer unittested is *excelent*. But really whole solution need
>> huge documentation or little bit better api. As I see above, I'm not sure if I will be able to
>> write simple test with understanding of what I'm doing without deep digging.
>
> What sort of documentation would you like to see ?
> You do need understand the specific messages ITW sends back&forth to test it, but once you do I
> don't think it is *too* difficult.
>
>>
>> Or maybe I have deadlocked myself... Looking forward for next round!
>> //me not as much deep insde as should be
>>
>> J.
> //me admittedly fond of this patch 8^)
>
> Happy hacking,
> -Adam
More information about the distro-pkg-dev
mailing list