Add some more tests #33

evansiroky · 2017-09-15T17:53:30Z

Add tests for the VanillaExtractServer and PostgresSQL.

Also, calculate code coverage and send results to codecov.io.

The statement to set the representative position of a way was having weird issues on travis, so I refactored it. Also, I renamed the a column called “nodes” to “node_ids” to avoid calling a table and column the same thing.

codecov-io · 2017-09-15T17:53:31Z

Codecov Report

❗ No coverage uploaded for pull request base (rdbms-sink@baa67c6). Click here to learn what that means.
The diff coverage is 38.46%.

@@              Coverage Diff              @@
##             rdbms-sink      #33   +/-   ##
=============================================
  Coverage              ?   51.94%           
  Complexity            ?      277           
=============================================
  Files                 ?       37           
  Lines                 ?     2058           
  Branches              ?      233           
=============================================
  Hits                  ?     1069           
  Misses                ?      910           
  Partials              ?       79

Impacted Files	Coverage Δ	Complexity Δ
...in/java/com/conveyal/osmlib/PostgresOSMSource.java	`77.27% <100%> (ø)`	`10 <2> (?)`
...rc/main/java/com/conveyal/osmlib/PostgresSink.java	`77.51% <100%> (ø)`	`16 <0> (?)`
.../main/java/com/conveyal/osmlib/VanillaExtract.java	`61.76% <20%> (ø)`	`3 <1> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update baa67c6...9b2abff. Read the comment docs.

abyrd

Overall looks good as a first step toward expanding testing of these libraries.

abyrd · 2017-09-18T20:48:22Z

.travis.yml

@@ -24,6 +29,9 @@ script: |
    mvn clean verify --settings maven-settings.xml
  fi

+after_success:
+  - bash <(curl -s https://codecov.io/bash)


This is running arbitrary code off a remote URL. This raises a red flag for me, especially on a repo that contains signing keys. Can we copy the script into our repo so we know what we're running?

The script at the url is pretty large and may change over time. I think this request is a bit excessive because that would also mean that we should examine and put every single maven dependency in our repo because that code could also echo environment variables or send it to a remote server.

True enough but Maven artifacts are signed. This is taking arbitrary code from a URL with no verification, and anyone can stick a proxy in between you and the server. It's always debatable how paranoid one needs to be, but the recent trend in piping the contents of URLs into bash is really a security hole.

Note that it's also unlikely we'd ever know if someone got the keys, which then compromises the signing system of Maven. The security problem is contagious.

Ah... but it's an https URL so there's a certificate for a particular domain involved. Maybe it's not so insecure then...

abyrd · 2017-09-18T20:49:46Z

pom.xml

@@ -135,6 +135,25 @@
          </execution>
        </executions>
      </plugin>
+      <plugin>
+        <groupId>org.jacoco</groupId>


Please add XML comments to the POM briefly explaining what this is. I know all the other dependencies don't have this but we should start adding it. It makes it less painful to browse through in the future.

abyrd · 2017-09-18T20:53:37Z

src/main/java/com/conveyal/osmlib/PostgresSink.java

@@ -246,7 +244,10 @@ public void writeEnd() throws IOException {
            // statement.execute("create index on nodes(lat, lon)");
            LOG.info("Assigning representative coordinates to ways...");
            statement.execute("alter table ways add column rep_lat float(9), add column rep_lon float(9)");
-            statement.execute("update ways set (rep_lat, rep_lon) = (select lat, lon from nodes where nodes.node_id = nodes[array_length(nodes, 1)/2])");
+            statement.execute("UPDATE ways " +


Personally I don't like the peculiar SQL convention of capitalizing keywords, especially since it's not checked by the parser so doesn't add any semantic safety. It mostly just achieves a cyber 1980s look. It's of course not going to directly hurt anything to have some statements using one convention and some using another, but just for consistency I'd rather do all lower case. This is a pretty superficial comment though.

ok, I'll change it to lowercase

abyrd · 2017-09-18T20:55:40Z

src/main/java/com/conveyal/osmlib/VanillaExtract.java

@@ -71,16 +66,14 @@ public static void main(String[] args) {
        try {
            httpServer.start();
            LOG.info("VEX server running.");
-            Thread.currentThread().join();
-//            updateThread.interrupt();
+            //Thread.currentThread().join();


Why did you take out the thread join and the server shutdown? It will probably work without it but it's more clean to properly shut down.

I did this because it was causing the VanillaExtractTest to hang when it tried to start the server. The test would not continue after the server started up.

I don't think we should be changing the flow of our main method in order to make a test easier to write. We need to establish clearly what we're losing by not doing the join and clean shutdown -- this doesn't seem like the kind of thing that should just be changed without understanding the consequences.

I'm not a java expert so I don't understand what the thread joining does.

Thread.currentThread().join(); is kind of strange. It just blocks the current thread forever, waiting for itself to end. It only makes sense because this is intended to be run as a command line main method. Once the server thread starts, we don't want the main thread to shut down and end the whole process. This is another reason that if we want to test this logic, it should be factored out and called slightly differently from a test and from a main method.

abyrd · 2017-09-18T20:56:48Z

.travis.yml

@@ -8,6 +8,11 @@ jdk:
 before_install:
  - openssl aes-256-cbc -K $encrypted_3ec7d78ab93d_key -iv $encrypted_3ec7d78ab93d_iv -in maven-artifact-signing-key.asc.enc -out maven-artifact-signing-key.asc -d
  - gpg --import --batch maven-artifact-signing-key.asc
+  - psql -U postgres -c 'CREATE DATABASE osm_lib_test;'


We should probably add comments to this file explaining why all these lines are here. Not just the database setup for the tests but the encrypted variables etc.

abyrd · 2017-09-18T21:04:39Z

src/test/java/com/conveyal/osmlib/PostgresTest.java

+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+
+public class PostgresTest {


As a matter of policy all new classes and methods should have comments. This should contain a human-readable explanation of what is being tested.

abyrd · 2017-09-18T21:07:13Z

src/test/java/com/conveyal/osmlib/PostgresTest.java

+    }
+}
+
+class TableTestCase {


Nice clear data types.

abyrd · 2017-09-18T21:09:36Z

src/test/java/com/conveyal/osmlib/PostgresTest.java

+        };
+
+        // perform file to postgres load
+        PostgresSink.main(args);


I find it kind of strange to be passing string arguments into a main method in a test. Main methods are really only intended to be used as a command line interface. Ideally we'd pull the "load a feed" functionality out of the main method and call the same functionality from the main and the test. But this will do for now.

abyrd · 2017-09-18T21:27:41Z

src/test/java/com/conveyal/osmlib/VanillaExtractTest.java

+import static org.hamcrest.Matchers.*;
+
+public class VanillaExtractTest {
+    static final String TEST_FILE = "./src/test/resources/porto_portugal.osm.pbf";


Resources are associated with a package (in this case com.conveyal.osmlib) so it should be possible to put them in the appropriate place (/src/test/resources/com/conveyal/osmlib/porto_portugal.osm.pbf) then open them as a stream without giving full paths. That would require us to have load methods that accepted something other than a string path though.

abyrd · 2017-09-18T21:28:53Z

src/test/java/com/conveyal/osmlib/VanillaExtractTest.java

+
+        // assert that the response is not an error response
+        assertThat(response, not(containsString("URI format")));
+


We should also attempt to decode the data that comes back as a PBF file and make sure it contains anything meaningful.

I added some more logic that tests this.

abyrd · 2017-09-19T18:46:40Z

src/test/java/com/conveyal/osmlib/TestUtils.java

+            connection = DriverManager.getConnection(pgUrl);
+        } catch (SQLException e) {
+            e.printStackTrace();
+            LOG.error("Error creating new database!");


This log message does not correspond to the purpose of the method.

abyrd · 2017-09-19T18:47:06Z

src/test/java/com/conveyal/osmlib/TestUtils.java

+            connection.prepareStatement(statement).execute();
+        } catch (SQLException e) {
+            e.printStackTrace();
+            LOG.error("Error creating new database!");


This log message does not correspond to the purpose of the method.

abyrd · 2017-09-19T18:47:24Z

src/test/java/com/conveyal/osmlib/TestUtils.java

+     * An MD5 can be represented as 32 hex digits so we don't save much length, but we do make it entirely alphabetical.
+     * log base 2 of 26 is about 4.7, so each character represents about 4.7 bits of randomness.
+     *
+     * The question remains of whether we need globally unique IDs or just application-unique IDs. The downside of


This code appears to be copied out of gtfs-lib. Since these are separate libraries I guess we have no choice but to duplicate the code. But the javadoc comments contain references to "feeds" and "older databases" which only make sense in the gtfs-lib context.

I'm also wondering why we need random database names - weren't you doing it before with a single predetermined name?

I was using a single name, but I want to avoid that because I want to run each test on it's own database so that tests won't depend on each other. I've thought of a different idea for creating unique IDs and will implement that instead.

abyrd · 2017-09-21T12:05:58Z

Other than the concerns about curl | bash (for which I don't think there's a much better solution) this looks pretty good. Should we merge and continue to add more tests later?

evansiroky · 2018-12-17T05:12:21Z

@abyrd This PR also looks like it is awaiting your approval.

evansiroky added 4 commits September 13, 2017 16:02

Add some tests

88c122b

try to get coverage working on travis

c083941

fix postgres user on travis

abd177b

evansiroky requested a review from abyrd September 15, 2017 17:53

abyrd approved these changes Sep 18, 2017

View reviewed changes

evansiroky added 5 commits September 18, 2017 16:13

Add comments to pom.xml about new libraries

2f5b704

correct case and write comment about sql that had problems

d17a6af

Refactor to allow for isolated db testing

efe0d5e

Enhance VanillaExtract test with more docs and extract test

c9f2839

Add comments to PostgresTest

b35ad3b

abyrd reviewed Sep 19, 2017

View reviewed changes

refactor method that generates test database names

bede65e

refactor VanillaExtract to move logic out of main method

9b2abff

evansiroky assigned abyrd Oct 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some more tests #33

Add some more tests #33

evansiroky commented Sep 15, 2017

codecov-io commented Sep 15, 2017 •

edited

Loading

abyrd left a comment

abyrd Sep 18, 2017

evansiroky Sep 18, 2017

abyrd Sep 19, 2017

abyrd Sep 19, 2017

abyrd Sep 19, 2017

abyrd Sep 18, 2017

abyrd Sep 18, 2017

evansiroky Sep 18, 2017

abyrd Sep 18, 2017

evansiroky Sep 19, 2017

abyrd Sep 19, 2017

evansiroky Sep 19, 2017

abyrd Sep 21, 2017

abyrd Sep 18, 2017

abyrd Sep 18, 2017

abyrd Sep 18, 2017

abyrd Sep 18, 2017

abyrd Sep 18, 2017

abyrd Sep 18, 2017

evansiroky Sep 19, 2017

abyrd Sep 19, 2017

abyrd Sep 19, 2017

abyrd Sep 19, 2017

evansiroky Sep 19, 2017 •

edited

Loading

abyrd commented Sep 21, 2017

evansiroky commented Dec 17, 2018


		// assert that the response is not an error response
		assertThat(response, not(containsString("URI format")));

Add some more tests #33

Are you sure you want to change the base?

Add some more tests #33

Conversation

evansiroky commented Sep 15, 2017

codecov-io commented Sep 15, 2017 • edited Loading

Codecov Report

abyrd 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

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

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

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

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

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

evansiroky Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

abyrd commented Sep 21, 2017

evansiroky commented Dec 17, 2018

codecov-io commented Sep 15, 2017 •

edited

Loading

evansiroky Sep 19, 2017 •

edited

Loading