committing Iain's new tests and polished things a little

(also added a test-specific log4j conf so that we don't show all the warnings during the tests that were distracting)


git-svn-id: http://google-refine.googlecode.com/svn/trunk@744 7d457c2a-affb-35e4-300a-418c747d4874

build.xml

@@ -95,6 +95,7 @@
         <javac srcdir="${tests.dir}/src" destdir="${tests_classes.dir}" debug="true" includeAntRuntime="no">
             <classpath refid="tests.class.path" />
         </javac>
+        <copy file="${tests.dir}/src/log4j.properties" tofile="${tests_classes.dir}/log4j.properties"/>
     </target>
 
     <target name="jar" depends="build">

gridworks

@@ -526,7 +526,7 @@ server_test() {
     ant build_tests
     echo ""
     
-    CLASSPATH="$GRIDWORKS_BUILD_DIR/classes${SEP}$GRIDWORKS_WEBAPP/WEB-INF/classes${SEP}$GRIDWORKS_TEST_DIR/java/classes${SEP}$GRIDWORKS_TEST_DIR/java/lib/*${SEP}$GRIDWORKS_LIB_DIR/*${SEP}$GRIDWORKS_WEBAPP/WEB-INF/lib/*"
+    CLASSPATH="$GRIDWORKS_TEST_DIR/java/classes${SEP}$GRIDWORKS_WEBAPP/WEB-INF/classes${SEP}$GRIDWORKS_BUILD_DIR/classes${SEP}$GRIDWORKS_TEST_DIR/java/lib/*${SEP}$GRIDWORKS_LIB_DIR/*${SEP}$GRIDWORKS_WEBAPP/WEB-INF/lib/*"
 
     if [ -z "$1" ]; then
         cd "$GRIDWORKS_TEST_DIR/java/classes"

src/main/java/com/metaweb/gridworks/commands/Command.java

@@ -53,7 +53,10 @@ public abstract class Command {
      * @return
      * @throws Exception
      */
-    static protected JSONObject getEngineConfig(HttpServletRequest request) throws Exception {
+    static protected JSONObject getEngineConfig(HttpServletRequest request)
+    throws JSONException {
+        if (request == null) throw new IllegalArgumentException("parameter 'request' should not be null");
+        
         String json = request.getParameter("engine");
         return (json == null) ? null : ParsingUtilities.evaluateJsonStringToObject(json);
     }
@@ -67,13 +70,15 @@ public abstract class Command {
      * @return
      * @throws Exception
      */
-    static protected Engine getEngine(HttpServletRequest request, Project project) throws Exception {
+    static protected Engine getEngine(HttpServletRequest request, Project project)
+    throws Exception {
+        if (request == null) throw new IllegalArgumentException("parameter 'request' should not be null");
+        if (project == null) throw new IllegalArgumentException("parameter 'project' should not be null");
+
         Engine engine = new Engine(project);
-        String json = request.getParameter("engine");
+        JSONObject o = getEngineConfig(request);
-        if (json != null) {
+        if (o != null)
-            JSONObject o = ParsingUtilities.evaluateJsonStringToObject(json);
             engine.initializeFromJSON(o);
-        }
         return engine;
     }
 
@@ -85,7 +90,8 @@ public abstract class Command {
      * @return
      * @throws ServletException
      */
-    static protected Project getProject(HttpServletRequest request) throws ServletException {
+    protected Project getProject(HttpServletRequest request) throws ServletException {
+        if (request == null) throw new IllegalArgumentException("parameter 'request' should not be null");
         try {
             Project p = ProjectManager.singleton.getProject(Long.parseLong(request.getParameter("project")));
             if (p != null) {
@@ -94,10 +100,11 @@ public abstract class Command {
         } catch (Exception e) {
             // ignore
         }
-        throw new ServletException("Missing or bad project URL parameter");
+        throw new ServletException("Can't find project: missing or bad URL parameter");
     }
 
     static protected int getIntegerParameter(HttpServletRequest request, String name, int def) {
+        if (request == null) throw new IllegalArgumentException("parameter 'request' should not be null");
         try {
             return Integer.parseInt(request.getParameter(name));
         } catch (Exception e) {
@@ -107,6 +114,7 @@ public abstract class Command {
     }
 
     static protected JSONObject getJsonParameter(HttpServletRequest request, String name) {
+        if (request == null) throw new IllegalArgumentException("parameter 'request' should not be null");
         String value = request.getParameter(name);
         if (value != null) {
             try {

src/main/java/com/metaweb/gridworks/util/ParsingUtilities.java

@@ -78,14 +78,22 @@ public class ParsingUtilities {
 
     static public JSONObject evaluateJsonStringToObject(String s) throws JSONException {
         JSONTokener t = new JSONTokener(s);
-        JSONObject o = (JSONObject) t.nextValue();
+        Object o = t.nextValue();
-        return o;
+        if (o instanceof JSONObject) {
+            return (JSONObject) o;
+        } else {
+            throw new JSONException(s + " couldn't be parsed as JSON object");
+        }
     }
     
     static public JSONArray evaluateJsonStringToArray(String s) throws JSONException {
         JSONTokener t = new JSONTokener(s);
-        JSONArray a = (JSONArray) t.nextValue();
+        Object o = t.nextValue();
-        return a;
+        if (o instanceof JSONArray) {
+            return (JSONArray) o;
+        } else {
+            throw new JSONException(s + " couldn't be parsed as JSON array");
+        }
     }
     
     private static final URLCodec codec = new URLCodec();

tests/java/src/com/metaweb/gridworks/tests/GridworksTests.java

@@ -1,14 +0,0 @@
-package com.metaweb.gridworks.tests;
-
-import static org.junit.Assert.assertSame;
-
-import org.junit.Test;
-
-public class GridworksTests {
-
-    // this is just a placeholder for now
-    
-    @Test public void test() {
-        assertSame("Just a placeholder for now", Integer.parseInt("1"), 1);
-    }
-}

tests/java/src/com/metaweb/gridworks/tests/commands/CommandStub.java

@@ -0,0 +1,42 @@
+package com.metaweb.gridworks.tests.commands;
+
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+
+import org.json.JSONException;
+import org.json.JSONObject;
+import org.junit.Ignore;
+
+import com.metaweb.gridworks.browsing.Engine;
+import com.metaweb.gridworks.commands.Command;
+import com.metaweb.gridworks.model.Project;
+
+/**
+ * Implementation of abstract class for testing Exposes protected members as public
+ */
+@Ignore
+public class CommandStub extends Command {
+
+    public Project wrapGetProject(HttpServletRequest request)
+    throws ServletException {
+        return getProject(request);
+    }
+
+    public JSONObject wrapGetEngineConfig(HttpServletRequest request)
+    throws JSONException {
+        return getEngineConfig(request);
+    }
+
+    public Engine wrapGetEngine(HttpServletRequest request, Project project)
+    throws Exception {
+        return getEngine(request, project);
+    }
+
+    public int wrapGetIntegerParameter(HttpServletRequest request, String name,int def) {
+        return getIntegerParameter(request, name, def);
+    }
+
+    public JSONObject wrapGetJsonParameter(HttpServletRequest request,String name) {
+        return getJsonParameter(request, name);
+    }
+}

tests/java/src/com/metaweb/gridworks/tests/commands/CommandTests.java

@@ -0,0 +1,315 @@
+package com.metaweb.gridworks.tests.commands;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+
+import org.json.JSONException;
+import org.json.JSONObject;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.metaweb.gridworks.ProjectManager;
+import com.metaweb.gridworks.browsing.Engine;
+import com.metaweb.gridworks.model.Project;
+
+public class CommandTests {
+
+    final static protected Logger logger = LoggerFactory.getLogger("CommandTests");
+
+    CommandStub SUT = null;
+    HttpServletRequest request = null;
+    ProjectManager projectManager = null;
+    Project project = null;
+
+    @Before
+    public void SetUp() {
+        SUT = new CommandStub();
+        request = mock(HttpServletRequest.class);
+        projectManager = mock(ProjectManager.class);
+        project = mock(Project.class);
+    }
+
+    @After
+    public void TearDown() {
+        SUT = null;
+        request = null;
+        projectManager = null;
+        project = null;
+    }
+
+    // -----------------getProject tests------------
+
+    @Test
+    public void getProjectThrowsWithNullParameter() {
+        try {
+            SUT.wrapGetProject(null);
+            Assert.fail(); // should throw exception before this
+        } catch (IllegalArgumentException e) {
+            // expected
+        } catch (Exception e) {
+            Assert.fail();
+        }
+    }
+
+    @Test
+    public void getProjectThrowsIfResponseHasNoOrBrokenProjectParameter() {
+        when(request.getParameter("project")).thenReturn(""); // null
+        try {
+            SUT.wrapGetProject(request);
+        } catch (ServletException e) {
+            // expected
+        } catch (Exception e) {
+            Assert.fail();
+        }
+        verify(request, times(1)).getParameter("project");
+    }
+
+    // -----------------getEngineConfig tests-----------------
+    @Test
+    public void getEngineConfigThrowsWithNullParameter() {
+        try {
+            SUT.wrapGetEngineConfig(null);
+            Assert.fail();
+        } catch (IllegalArgumentException e) {
+            // expected
+        } catch (Exception e) {
+            Assert.fail();
+        }
+    }
+
+    @Test
+    public void getEngineConfigReturnsNullWithNullEngineParameter() {
+        when(request.getParameter("engine")).thenReturn(null);
+        try {
+            Assert.assertNull(SUT.wrapGetEngineConfig(request));
+        } catch (JSONException e) {
+            Assert.fail();
+        } catch (Exception e) {
+            Assert.fail();
+        }
+    }
+
+    @Test
+    public void getEngineConfigThrowsWithEmptyOrBadParameterValue() {
+        when(request.getParameter("engine")).thenReturn("sdfasdfas");
+
+        try {
+            Assert.assertNull(SUT.wrapGetEngineConfig(request));
+            Assert.fail();
+        } catch (Exception e) {
+            // expected
+        }
+
+        verify(request, times(1)).getParameter("engine");
+    }
+
+    @Test
+    public void getEngineConfigRegressionTest() {
+        when(request.getParameter("engine")).thenReturn("{\"hello\":\"world\"}");
+        JSONObject o = null;
+        try {
+            o = SUT.wrapGetEngineConfig(request);
+            Assert.assertEquals("world", o.getString("hello"));
+        } catch (JSONException e) {
+            Assert.fail();
+        } catch (Exception e) {
+            Assert.fail();
+        }
+        verify(request, times(1)).getParameter("engine");
+    }
+
+    // -----------------getEngine tests----------------------
+    @Test
+    public void getEngineThrowsOnNullParameter() {
+        try {
+            SUT.wrapGetEngine(null, null);
+        } catch (IllegalArgumentException e) {
+            // expected
+        } catch (Exception e) {
+            Assert.fail();
+        }
+
+        try {
+            SUT.wrapGetEngine(null, project);
+        } catch (IllegalArgumentException e) {
+            // expected
+        } catch (Exception e) {
+            Assert.fail();
+        }
+
+        try {
+            SUT.wrapGetEngine(request, null);
+        } catch (IllegalArgumentException e) {
+            // expected
+        } catch (Exception e) {
+            Assert.fail();
+        }
+    }
+
+    @Test
+    public void getEngineRegressionTest() {
+        // TODO refactor getEngine to use dependency injection, so a mock Engine
+        // object can be used.
+
+        Engine engine = null;
+        when(request.getParameter("engine")).thenReturn("{\"hello\":\"world\"}");
+
+        try {
+            engine = SUT.wrapGetEngine(request, project);
+            Assert.assertNotNull(engine);
+        } catch (Exception e) {
+            Assert.fail();
+        }
+
+        verify(request, times(1)).getParameter("engine");
+        // JSON configuration doesn't have 'facets' key or 'INCLUDE_DEPENDENT'
+        // key, so there should be no further action
+        // Engine._facets is protected so can't test that it is of zero length.
+    }
+
+    // ------------------
+    @Test
+    public void getIntegerParameterWithNullParameters() {
+        // all null
+        try {
+            SUT.wrapGetIntegerParameter(null, null, 0);
+            Assert.fail();
+        } catch (IllegalArgumentException e) {
+            // expected
+        }
+
+        // request null
+        try {
+            SUT.wrapGetIntegerParameter(null, "name", 0);
+            Assert.fail();
+        } catch (IllegalArgumentException e) {
+            // expected
+        }
+    }
+
+    @Test
+    public void getIntegerParametersWithIncorrectParameterName() {
+
+        when(request.getParameter(null)).thenReturn(null);
+        when(request.getParameter("incorrect")).thenReturn(null);
+
+        // name null
+        try {
+            int returned = SUT.wrapGetIntegerParameter(request, null, 5);
+            Assert.assertEquals(5, returned);
+        } catch (IllegalArgumentException e) {
+            Assert.fail();
+        }
+
+        // name incorrect
+        try {
+            int returned = SUT.wrapGetIntegerParameter(request, "incorrect", 5);
+            Assert.assertEquals(5, returned);
+        } catch (IllegalArgumentException e) {
+            Assert.fail();
+        }
+
+        verify(request, times(1)).getParameter(null);
+        verify(request, times(1)).getParameter("incorrect");
+    }
+
+    @Test
+    public void getIntegerParametersRegressionTest() {
+        when(request.getParameter("positivenumber")).thenReturn("22");
+        when(request.getParameter("zeronumber")).thenReturn("0");
+        when(request.getParameter("negativenumber")).thenReturn("-40");
+
+        // positive
+        try {
+            int returned = SUT.wrapGetIntegerParameter(request,"positivenumber", 5);
+            Assert.assertEquals(22, returned);
+        } catch (IllegalArgumentException e) {
+            Assert.fail();
+        }
+
+        // zero
+        try {
+            int returned = SUT.wrapGetIntegerParameter(request, "zeronumber", 5);
+            Assert.assertEquals(0, returned);
+        } catch (IllegalArgumentException e) {
+            Assert.fail();
+        }
+
+        // negative
+        try {
+            int returned = SUT.wrapGetIntegerParameter(request,
+                    "negativenumber", 5);
+            Assert.assertEquals(-40, returned);
+        } catch (IllegalArgumentException e) {
+            Assert.fail();
+        }
+
+        verify(request, times(1)).getParameter("positivenumber");
+        verify(request, times(1)).getParameter("zeronumber");
+        verify(request, times(1)).getParameter("negativenumber");
+    }
+
+    // ---------------------getJsonParameter tests----------------
+    @Test
+    public void getJsonParameterWithNullParameters() {
+        when(request.getParameter(null)).thenReturn(null);
+        when(request.getParameter("")).thenReturn(null);
+
+        try {
+            SUT.wrapGetJsonParameter(null, null);
+            Assert.fail();
+        } catch (IllegalArgumentException e) {
+            // expected
+        }
+
+        Assert.assertNull(SUT.wrapGetJsonParameter(request, null));
+
+        try {
+            SUT.wrapGetJsonParameter(null, "test");
+        } catch (IllegalArgumentException e) {
+            // expected
+        }
+
+        Assert.assertNull(SUT.wrapGetJsonParameter(request, ""));
+
+        verify(request, times(1)).getParameter(null);
+        verify(request, times(1)).getParameter("");
+    }
+
+    @Test
+    public void getJsonParameterRegressionTest() {
+        when(request.getParameter("test")).thenReturn("{\"foo\":\"bar\"}");
+
+        JSONObject o = SUT.wrapGetJsonParameter(request, "test");
+        Assert.assertNotNull(o);
+        try {
+            Assert.assertEquals("bar", o.getString("foo"));
+        } catch (JSONException e) {
+            Assert.fail();
+        }
+
+        verify(request, times(1)).getParameter("test");
+    }
+
+    @Test
+    public void getJsonParameterWithMalformedJson() {
+        when(request.getParameter("test")).thenReturn("brokenJSON");
+
+        try {
+            Assert.assertNull(SUT.wrapGetJsonParameter(request, "test"));
+        } catch (Exception e) {
+            Assert.fail();
+        }
+
+        verify(request, times(1)).getParameter("test");
+    }
+}

tests/java/src/com/metaweb/gridworks/tests/commands/util/CancelProcessesCommandTests.java

@@ -41,15 +41,17 @@ public class CancelProcessesCommandTests {
     HttpServletRequest request = null;
     HttpServletResponse response = null;
     ProjectManager projMan = null;
-    Project p = null;
+    Project proj = null;
     ProcessManager processMan = null;
+    PrintWriter pw = null;
 
     @Before
     public void SetUp() {
         projMan = mock(ProjectManager.class);
         ProjectManager.singleton = projMan;
-        p = mock(Project.class);
+        proj = mock(Project.class);
         processMan = mock(ProcessManager.class);
+        pw = mock(PrintWriter.class);
 
         request = mock(HttpServletRequest.class);
         response = mock(HttpServletResponse.class);
@@ -62,7 +64,8 @@ public class CancelProcessesCommandTests {
 
         projMan = null;
         ProjectManager.singleton = null;
-        p = null;
+        proj = null;
+        pw = null;
         request = null;
         response = null;
     }
@@ -101,15 +104,16 @@ public class CancelProcessesCommandTests {
         }
     }
 
-    // runs through a complete working post
+    /**
+     *  Contract for a complete working post
+     */
     @Test
-    public void doPost() {
+    public void doPostRegressionTest() {
 
         // mock dependencies
         when(request.getParameter("project")).thenReturn(PROJECT_ID);
-        when(projMan.getProject(anyLong())).thenReturn(p);
+        when(projMan.getProject(anyLong())).thenReturn(proj);
-        when(p.getProcessManager()).thenReturn(processMan);
+        when(proj.getProcessManager()).thenReturn(processMan);
-        PrintWriter pw = mock(PrintWriter.class);
         try {
             when(response.getWriter()).thenReturn(pw);
         } catch (IOException e1) {
@@ -128,17 +132,72 @@ public class CancelProcessesCommandTests {
         // verify
         verify(request, times(1)).getParameter("project");
         verify(projMan, times(1)).getProject(PROJECT_ID_LONG);
-        verify(p, times(1)).getProcessManager();
+
+        verify(processMan, times(1)).cancelAll();
+        verify(response, times(1)).setCharacterEncoding("UTF-8");
+        verify(response, times(1)).setHeader("Content-Type", "application/json");
+        verify(proj, times(1)).getProcessManager();
         try {
             verify(response, times(1)).getWriter();
         } catch (IOException e) {
             Assert.fail();
         }
-
-        verify(processMan, times(1)).cancelAll();
-        verify(response, times(1)).setCharacterEncoding("UTF-8");
-        verify(response, times(1))
-                .setHeader("Content-Type", "application/json");
         verify(pw, times(1)).write("{ \"code\" : \"ok\" }");
     }
+
+     @Test
+     public void doPostThrowsIfCommand_getProjectReturnsNull(){
+        // mock dependencies
+        when(request.getParameter("project")).thenReturn(PROJECT_ID);
+        when(projMan.getProject(anyLong()))
+            .thenReturn(null);
+
+        // run
+        try {
+            SUT.doPost(request, response);
+        } catch (ServletException e) {
+            //expected
+        } catch (IOException e) {
+            Assert.fail();
+        }
+
+        // verify
+        verify(request, times(1)).getParameter("project");
+        verify(projMan, times(1)).getProject(PROJECT_ID_LONG);
+     }
+
+     @Test
+     public void doPostCatchesExceptionFromWriter(){
+         String ERROR_MESSAGE = "hello world";
+
+        // mock dependencies
+        when(request.getParameter("project")).thenReturn(PROJECT_ID);
+        when(projMan.getProject(anyLong())).thenReturn(proj);
+        when(proj.getProcessManager()).thenReturn(processMan);
+        try {
+            when(response.getWriter())
+              .thenThrow(new IllegalStateException(ERROR_MESSAGE))
+              .thenReturn(pw);
+        } catch (IOException e) {
+            Assert.fail();
+        }
+
+        // run
+        try {
+            SUT.doPost(request, response);
+        } catch (ServletException e) {
+            Assert.fail();
+        } catch (IOException e) {
+            Assert.fail();
+        }
+
+        verify(request, times(1)).getParameter("project");
+        verify(projMan, times(1)).getProject(PROJECT_ID_LONG);
+
+        verify(processMan, times(1)).cancelAll();
+        verify(response, times(3)).setCharacterEncoding("UTF-8");
+        //omitted other verifications for brevity.
+        //assumption is that expecting response.setCharacterEncoding times(3)
+        //implies it has Command.respondException has been called as expected
+     }
 }

tests/java/src/log4j.properties

@@ -0,0 +1,4 @@
+log4j.rootLogger=ERROR, console
+
+log4j.appender.console=org.apache.log4j.ConsoleAppender
+log4j.appender.console.layout=com.metaweb.util.logging.IndentingLayout