[rfc][icedtea-web] PluginAppletViewer unit tests with test extensions for mocking plugin input&output pipes
Jiri Vanek
jvanek at redhat.com
Tue Apr 2 06:46:08 PDT 2013
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:)
> 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...
> 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" ?
>
> 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
> + 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.
> +
> + 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?
> + 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 :(
> + /* 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.
> 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 :) )
> @@ -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 :(
> +
> + 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.
> +
> + 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)
> + }
> + handler.interrupt(); // Finish early
> + }
> + };
> +
> + thread.start();
> +
> + try {
> + Thread.sleep(timeout);
> + } catch (InterruptedException e) {
> + // Finish early
> + return;
Some special ExitStatus here?
> + }
> +
> + if (thread.isAlive()) {
> + exitStatus = ExitStatus.TIMED_OUT;
Maybe throw an exception?
> + }
> +
> + // Make sure the thread is finished
> + while (thread.isAlive()) {
> + thread.interrupt();
argh.. but sure:(
> + }
> + }
> + }
> +}
> diff --git a/tests/test-extensions/sun/applet/PluginPipeMessages.java b/tests/test-extensions/sun/applet/PluginPipeMessages.java
> new file mode 100644
> --- /dev/null
> +++ b/tests/test-extensions/sun/applet/PluginPipeMessages.java
> @@ -0,0 +1,95 @@
> +package sun.applet;
> +
> +import static org.junit.Assert.assertEquals;
> +import static org.junit.Assert.assertTrue;
> +
> +/**
> + * Simple utility for parsing & verifying plugin messages.
> + *
> + * Assumes it is run in a JUnit-test.
> + */
> +public class PluginPipeMessages {
> +
> + private static Object getStoredObject(int id) {
> + return PluginObjectStore.getInstance().getObject(id);
> + }
> +
> + /*
> + * Asserts that the message is a valid javascript request and returns the
> + * reference number
> + */
> + private static int parseAndCheckJSMessage(String message, int messageLength,
> + String messageType, int contextObjectID) {
> + String[] parts = message.split(" ");
> + assertEquals(messageLength, parts.length);
> +
> + assertEquals("instance", parts[0]);
> + assertEquals("0", parts[1]); // JSCall's are prefixed with a dummy '0'
> + // instance
> + assertEquals("reference", parts[2]);
> + int reference = Integer.parseInt(parts[3]);
> + assertEquals(messageType, parts[4]);
> +
> + assertEquals(contextObjectID, Integer.parseInt(parts[5]));
> + return reference;
> + }
> +
> + /*
> + * Asserts that the message is a valid javascript request and returns the
> + * reference number
> + */
> + private static int parseAndCheckJSMessage(String message,
> + String messageType, int contextObjectID, String stringArg,
> + Object[] arguments) {
> + int expectedLength = 7 + arguments.length;
> + int reference = parseAndCheckJSMessage(message, expectedLength, messageType, contextObjectID);
> +
> + String[] parts = message.split(" ");
> + assertEquals(stringArg, getStoredObject(Integer.parseInt(parts[6])));
> +
> + for (int i = 0; i < arguments.length; i++) {
> + int objectID = Integer.parseInt(parts[7+i]);
> + assertEquals(arguments[i], getStoredObject(objectID));
> + }
> +
> + return reference;
> + }
> +
> + /*
> + * Asserts that the message is a valid javascript method call request, and
> + * returns the reference number
> + */
> + public static int parseAndCheckJSCall(String message, int contextObjectID,
> + String callName, Object[] arguments) {
> + return parseAndCheckJSMessage(message, "Call", contextObjectID,
> + callName, arguments);
> + }
> +
> + /*
> + * Asserts that the message is a valid javascript Eval request, and returns
> + * the reference number
> + */
> + public static int parseAndCheckJSEval(String message, int contextObjectID,
> + String evalString) {
> + return parseAndCheckJSMessage(message, "Eval", contextObjectID,
> + evalString, new Object[] {});
> + }
> +
> + /*
> + * Asserts that the message is a valid javascript Finalize request, and returns
> + * the reference number
> + */
> + public static int parseAndCheckJSFinalize(String message, int contextObjectID) {
> + int expectedLength = 6;
> + return parseAndCheckJSMessage(message, expectedLength, "Finalize", contextObjectID);
> + }
> +
> + /*
> + * Asserts that the message is a valid javascript ToString request, and returns
> + * 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.
> +}
> 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?
> + /**
> + * Queues up mocked responses and sends them as replies to icedtea-web
> + */
> + private class PluginInputPipeMock extends InputStream {
> + private StringReader reader = null;
> + private Deque<String> responses = new ArrayDeque<String>();
> +
> + @Override
> + public int read() throws IOException {
> + while (true) {
> + if (reader == null) {
> + reader = new StringReader(getNextResponse() + '\n');
> + }
> + int chr = reader.read();
> + if (chr == -1) {
> + reader = null;
> + continue;
> + }
> + return chr;
> + }
> + }
> +
> + /* Necessary for correct behaviour with BufferedReader! */
> + @Override
> + public int read(byte b[], int off, int len) throws IOException {
> + if (len == 0) {
> + return 0;
> + }
> + b[off] = (byte)read();
> + return 1;
> + }
> +
> + /* Blocks until we get a line of input. */
> + private String getNextResponse() {
> + while (true) {
> + synchronized (this) {
> + String line = responses.pollFirst();
> + if (line != null) {
> + return line;
> + }
> + }
> + }
> + }
> +
> + synchronized void sendReponse(String response) {
> + responses.add(response);
> + }
> + }
> +
> + /**
> + * Splits requests from icedtea-web into lines
> + */
> + private class PluginOutputPipeMock extends OutputStream {
> + private StringBuilder lineBuffer = new StringBuilder();
> + private Deque<String> lines = new ArrayDeque<String>();
> +
> + @Override
> + public synchronized void write(int b) throws IOException {
> + char chr = (char) b;
> + if (chr == '\0') {
> + lines.addLast(lineBuffer.toString());
> + lineBuffer.setLength(0);
> + } else {
> + lineBuffer.append((char) b);
> + }
> + }
> +
> + /* 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.
Or maybe I have deadlocked myself... Looking forward for next round!
//me not as much deep insde as should be
J.
More information about the distro-pkg-dev
mailing list