From c5faeff0d270afd18b379467ee2c9ab1fc47773d Mon Sep 17 00:00:00 2001 From: Evan Siroky Date: Tue, 19 May 2020 00:47:28 -0700 Subject: [PATCH 1/3] fix(snapshot): properly load/export/snapshot feeds w/ mixed calendar defs Fixes https://github.com/ibi-group/datatools-server/issues/313 --- .../gtfs/loader/JdbcGtfsSnapshotter.java | 78 ++-- src/test/java/com/conveyal/gtfs/GTFSTest.java | 341 +++++++++++++++--- .../gtfs/storage/PersistenceExpectation.java | 17 + .../gtfs/storage/RecordExpectation.java | 8 + .../agency.txt | 2 + .../calendar.txt | 3 + .../calendar_dates.txt | 3 + .../fare_attributes.txt | 2 + .../fare_rules.txt | 2 + .../feed_info.txt | 2 + .../frequencies.txt | 2 + .../routes.txt | 2 + .../shapes.txt | 8 + .../stop_times.txt | 7 + .../stops.txt | 3 + .../transfers.txt | 1 + .../trips.txt | 4 + 17 files changed, 406 insertions(+), 79 deletions(-) create mode 100755 src/test/resources/fake-agency-mixture-of-calendar-definitions/agency.txt create mode 100755 src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar.txt create mode 100755 src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar_dates.txt create mode 100755 src/test/resources/fake-agency-mixture-of-calendar-definitions/fare_attributes.txt create mode 100755 src/test/resources/fake-agency-mixture-of-calendar-definitions/fare_rules.txt create mode 100644 src/test/resources/fake-agency-mixture-of-calendar-definitions/feed_info.txt create mode 100755 src/test/resources/fake-agency-mixture-of-calendar-definitions/frequencies.txt create mode 100755 src/test/resources/fake-agency-mixture-of-calendar-definitions/routes.txt create mode 100755 src/test/resources/fake-agency-mixture-of-calendar-definitions/shapes.txt create mode 100755 src/test/resources/fake-agency-mixture-of-calendar-definitions/stop_times.txt create mode 100755 src/test/resources/fake-agency-mixture-of-calendar-definitions/stops.txt create mode 100755 src/test/resources/fake-agency-mixture-of-calendar-definitions/transfers.txt create mode 100755 src/test/resources/fake-agency-mixture-of-calendar-definitions/trips.txt diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java index 85653889c..1dd305f16 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java @@ -18,7 +18,9 @@ import java.time.format.DateTimeFormatter; import java.util.Arrays; import java.util.HashMap; +import java.util.HashSet; import java.util.Map; +import java.util.Set; import static com.conveyal.gtfs.loader.JdbcGtfsLoader.createFeedRegistryIfNotExists; import static com.conveyal.gtfs.loader.JdbcGtfsLoader.createSchema; @@ -274,42 +276,52 @@ private TableLoadResult createScheduleExceptionsTable() { } scheduleExceptionsTracker.executeRemaining(); - // determine if we appear to be working with a calendar_dates-only feed. - // If so, we must also add dummy entries to the calendar table - if ( - feedIdToSnapshot != null && - !tableExists(feedIdToSnapshot, "calendar") && - calendarDatesReader.getRowCount() > 0 - ) { - sql = String.format( - "insert into %s (service_id, description, start_date, end_date, " + - "monday, tuesday, wednesday, thursday, friday, saturday, sunday)" + - "values (?, ?, ?, ?, 0, 0, 0, 0, 0, 0, 0)", - tablePrefix + "calendar" + // fetch all entries in the calendar table to generate set of serviceIds that exist in the calendar + // table. + JDBCTableReader calendarReader = new JDBCTableReader( + Table.CALENDAR, + dataSource, + feedIdToSnapshot + ".", + EntityPopulator.CALENDAR + ); + Set calendarServiceIds = new HashSet<>(); + for (Calendar calendar : calendarReader.getAll()) { + calendarServiceIds.add(calendar.service_id); + } + + // add auto-generated entries to the calendar table that only existed in the calendar_dates table + sql = String.format( + "insert into %s (service_id, description, start_date, end_date, " + + "monday, tuesday, wednesday, thursday, friday, saturday, sunday)" + + "values (?, ?, ?, ?, 0, 0, 0, 0, 0, 0, 0)", + tablePrefix + "calendar" + ); + PreparedStatement calendarStatement = connection.prepareStatement(sql); + final BatchTracker calendarsTracker = new BatchTracker( + "calendar", + calendarStatement + ); + for (Calendar calendar : calendarsByServiceId.values()) { + if (calendarServiceIds.contains(calendar.service_id)) { + // This service_id already exists in the calendar table. No need to create auto-generated entry. + continue; + } + calendarStatement.setString(1, calendar.service_id); + calendarStatement.setString( + 2, + String.format("%s (auto-generated)", calendar.service_id) ); - PreparedStatement calendarStatement = connection.prepareStatement(sql); - final BatchTracker calendarsTracker = new BatchTracker( - "calendar", - calendarStatement + calendarStatement.setString( + 3, + calendar.start_date.format(DateTimeFormatter.BASIC_ISO_DATE) ); - for (Calendar calendar : calendarsByServiceId.values()) { - calendarStatement.setString(1, calendar.service_id); - calendarStatement.setString( - 2, - String.format("%s (auto-generated)", calendar.service_id) - ); - calendarStatement.setString( - 3, - calendar.start_date.format(DateTimeFormatter.BASIC_ISO_DATE) - ); - calendarStatement.setString( - 4, - calendar.end_date.format(DateTimeFormatter.BASIC_ISO_DATE) - ); - calendarsTracker.addBatch(); - } - calendarsTracker.executeRemaining(); + calendarStatement.setString( + 4, + calendar.end_date.format(DateTimeFormatter.BASIC_ISO_DATE) + ); + calendarsTracker.addBatch(); } + calendarsTracker.executeRemaining(); connection.commit(); } catch (Exception e) { diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index eedaaafa9..3113b0535 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -307,6 +307,159 @@ public void canLoadAndExportSimpleAgencyWithOnlyCalendarDates() { ); } + /** + * Tests whether the simple gtfs can be loaded and exported if it has a mixture of service_id definitions in both + * the calendar.txt and calendar_dates.txt files. + */@Test + public void canLoadAndExportSimpleAgencyWithMixtureOfCalendarDefinitions() { + PersistenceExpectation[] persistenceExpectations = new PersistenceExpectation[]{ + new PersistenceExpectation( + "agency", + new RecordExpectation[]{ + new RecordExpectation("agency_id", "1"), + new RecordExpectation("agency_name", "Fake Transit"), + new RecordExpectation("agency_timezone", "America/Los_Angeles") + } + ), + // calendar.txt-only expectation + new PersistenceExpectation( + "calendar", + new RecordExpectation[]{ + new RecordExpectation("service_id", "only-in-calendar-txt"), + new RecordExpectation("start_date", 20170915), + new RecordExpectation("end_date", 20170917) + } + ), + // calendar.txt and calendar-dates.txt expectation + new PersistenceExpectation( + "calendar", + new RecordExpectation[]{ + new RecordExpectation("service_id", "in-both-calendar-txt-and-calendar-dates"), + new RecordExpectation("start_date", 20170918), + new RecordExpectation("end_date", 20170920) + } + ), + new PersistenceExpectation( + "calendar_dates", + new RecordExpectation[]{ + new RecordExpectation( + "service_id", "in-both-calendar-txt-and-calendar-dates" + ), + new RecordExpectation("date", 20170920), + new RecordExpectation("exception_type", 2) + } + ), + // calendar-dates.txt-only expectation + new PersistenceExpectation( + "calendar", + new RecordExpectation[]{ + new RecordExpectation( + "service_id", "only-in-calendar-dates-txt" + ), + new RecordExpectation("start_date", 20170916), + new RecordExpectation("end_date", 20170916) + }, + true + ), + new PersistenceExpectation( + "calendar_dates", + new RecordExpectation[]{ + new RecordExpectation( + "service_id", "only-in-calendar-dates-txt" + ), + new RecordExpectation("date", 20170916), + new RecordExpectation("exception_type", 1) + } + ), + new PersistenceExpectation( + "stop_times", + new RecordExpectation[]{ + new RecordExpectation( + "trip_id", "non-frequency-trip" + ), + new RecordExpectation("arrival_time", 25200, "07:00:00"), + new RecordExpectation("departure_time", 25200, "07:00:00"), + new RecordExpectation("stop_id", "4u6g"), + new RecordExpectation("stop_sequence", 1), + new RecordExpectation("pickup_type", 0), + new RecordExpectation("drop_off_type", 0), + new RecordExpectation("shape_dist_traveled", 0.0, 0.01) + } + ), + // calendar-dates only expectation + new PersistenceExpectation( + "trips", + new RecordExpectation[]{ + new RecordExpectation( + "trip_id", "non-frequency-trip" + ), + new RecordExpectation( + "service_id", "only-in-calendar-dates-txt" + ), + new RecordExpectation("route_id", "1"), + new RecordExpectation("direction_id", 0), + new RecordExpectation( + "shape_id", "5820f377-f947-4728-ac29-ac0102cbc34e" + ), + new RecordExpectation("bikes_allowed", 0), + new RecordExpectation("wheelchair_accessible", 0) + } + ), + // calendar-only expectation + new PersistenceExpectation( + "trips", + new RecordExpectation[]{ + new RecordExpectation( + "trip_id", "non-frequency-trip-2" + ), + new RecordExpectation( + "service_id", "only-in-calendar-txt" + ), + new RecordExpectation("route_id", "1"), + new RecordExpectation("direction_id", 0), + new RecordExpectation( + "shape_id", "5820f377-f947-4728-ac29-ac0102cbc34e" + ), + new RecordExpectation("bikes_allowed", 0), + new RecordExpectation("wheelchair_accessible", 0) + } + ), + // calendar-dates and calendar expectation + new PersistenceExpectation( + "trips", + new RecordExpectation[]{ + new RecordExpectation( + "trip_id", "frequency-trip" + ), + new RecordExpectation( + "service_id", "in-both-calendar-txt-and-calendar-dates" + ), + new RecordExpectation("route_id", "1"), + new RecordExpectation("direction_id", 0), + new RecordExpectation( + "shape_id", "5820f377-f947-4728-ac29-ac0102cbc34e" + ), + new RecordExpectation("bikes_allowed", 0), + new RecordExpectation("wheelchair_accessible", 0) + } + ) + }; + ErrorExpectation[] errorExpectations = ErrorExpectation.list( + new ErrorExpectation(NewGTFSErrorType.MISSING_FIELD), + new ErrorExpectation(NewGTFSErrorType.ROUTE_LONG_NAME_CONTAINS_SHORT_NAME), + new ErrorExpectation(NewGTFSErrorType.FEED_TRAVEL_TIMES_ROUNDED) + ); + assertThat( + runIntegrationTestOnFolder( + "fake-agency-mixture-of-calendar-definitions", + nullValue(), + persistenceExpectations, + errorExpectations + ), + equalTo(true) + ); + } + /** * Tests that a GTFS feed with long field values generates corresponding * validation errors per MTC guidelines. @@ -396,26 +549,53 @@ private boolean runIntegrationTestOnZipFile( // load and validate feed LOG.info("load and validate GTFS file {}", zipFileName); FeedLoadResult loadResult = GTFS.load(zipFileName, dataSource); - ValidationResult validationResult = GTFS.validate(loadResult.uniqueIdentifier, dataSource, customValidators); + ValidationResult validationResult = GTFS.validate( + loadResult.uniqueIdentifier, + dataSource, + customValidators + ); assertThat(validationResult.fatalException, is(fatalExceptionExpectation)); namespace = loadResult.uniqueIdentifier; - assertThatImportedGtfsMeetsExpectations(connection, namespace, persistenceExpectations, errorExpectations); - } catch (SQLException e) { - TestUtils.dropDB(testDBName); - e.printStackTrace(); - return false; - } catch (AssertionError e) { - TestUtils.dropDB(testDBName); - throw e; - } + assertThatImportedGtfsMeetsExpectations( + connection, + namespace, + persistenceExpectations, + errorExpectations, + false + ); - // Verify that exporting the feed (in non-editor mode) completes and data is outputted properly - try { + // Verify that exporting the feed (in non-editor mode) completes and data is outputted properly LOG.info("export GTFS from created namespace"); File tempFile = exportGtfs(namespace, dataSource, false); assertThatExportedGtfsMeetsExpectations(tempFile, persistenceExpectations, false); - } catch (IOException e) { + + // Verify that making a snapshot from an existing feed database, then exporting that snapshot to a GTFS zip + // file works as expected + boolean snapshotIsOk = assertThatSnapshotIsSuccessful( + connection, + namespace, + dataSource, + testDBName, + persistenceExpectations, + false + ); + if (!snapshotIsOk) return false; + // Also, verify that if we're normalizing stop_times#stop_sequence, the stop_sequence values conform with + // our expectations (zero-based, incrementing values). + PersistenceExpectation[] expectationsWithNormalizedStopTimesSequence = + updatePersistenceExpectationsWithNormalizedStopTimesSequence(persistenceExpectations); + boolean normalizedSnapshotIsOk = assertThatSnapshotIsSuccessful( + connection, + namespace, + dataSource, + testDBName, + expectationsWithNormalizedStopTimesSequence, + true + ); + if (!normalizedSnapshotIsOk) return false; + } catch (IOException | SQLException e) { + LOG.error("An error occurred while loading/snapshotting the database!"); TestUtils.dropDB(testDBName); e.printStackTrace(); return false; @@ -424,25 +604,10 @@ private boolean runIntegrationTestOnZipFile( throw e; } - // Verify that making a snapshot from an existing feed database, then exporting that snapshot to a GTFS zip file - // works as expected - boolean snapshotIsOk = assertThatSnapshotIsSuccessful(namespace, dataSource, testDBName, persistenceExpectations, false); - if (!snapshotIsOk) return false; - // Also, verify that if we're normalizing stop_times#stop_sequence, the stop_sequence values conform with our - // expectations (zero-based, incrementing values). - PersistenceExpectation[] expectationsWithNormalizedStopTimesSequence = - updatePersistenceExpectationsWithNormalizedStopTimesSequence(persistenceExpectations); - boolean normalizedSnapshotIsOk = assertThatSnapshotIsSuccessful( - namespace, - dataSource, - testDBName, - expectationsWithNormalizedStopTimesSequence, - true - ); - if (!normalizedSnapshotIsOk) return false; - - // Verify that deleting a feed works as expected. + // Get a new connection here, because sometimes the old connection causes hanging issues upon trying to drop a + // schema (via deleting a GTFS namespace). try (Connection connection = dataSource.getConnection()) { + // Verify that deleting a feed works as expected. LOG.info("Deleting GTFS feed from database."); GTFS.delete(namespace, dataSource); @@ -465,6 +630,7 @@ private boolean runIntegrationTestOnZipFile( // There should be no schema records matching the deleted namespace. assertThat(schemaCount, is(0)); } catch (SQLException | InvalidNamespaceException e) { + LOG.error("An error occurred while deleting a schema!"); e.printStackTrace(); TestUtils.dropDB(testDBName); return false; @@ -518,7 +684,12 @@ private ValuePair (Object expected, Object found) { } } + /** + * Creates a snapshot, and asserts persistence expectations on the newly-created database of that snapshot. Then, + * exports that snapshot to a GTFS and asserts persistence expectations on the newly-exported GTFS. + */ private boolean assertThatSnapshotIsSuccessful( + Connection connection, String namespace, DataSource dataSource, String testDBName, @@ -529,10 +700,17 @@ private boolean assertThatSnapshotIsSuccessful( LOG.info("copy GTFS from created namespace"); SnapshotResult copyResult = GTFS.makeSnapshot(namespace, dataSource, normalizeStopTimes); assertThatSnapshotIsErrorFree(copyResult); + assertThatImportedGtfsMeetsExpectations( + connection, + copyResult.uniqueIdentifier, + persistenceExpectations, + null, + true + ); LOG.info("export GTFS from copied namespace"); File tempFile = exportGtfs(copyResult.uniqueIdentifier, dataSource, true); assertThatExportedGtfsMeetsExpectations(tempFile, persistenceExpectations, true); - } catch (IOException e) { + } catch (IOException | SQLException e) { e.printStackTrace(); TestUtils.dropDB(testDBName); return false; @@ -551,19 +729,23 @@ private void assertThatImportedGtfsMeetsExpectations( Connection connection, String namespace, PersistenceExpectation[] persistenceExpectations, - ErrorExpectation[] errorExpectations + ErrorExpectation[] errorExpectations, + boolean editorDatabase ) throws SQLException { // Store field mismatches here (to provide assertion statements with more details). Multimap fieldsWithMismatches = ArrayListMultimap.create(); - // Check that no validators failed during validation. - assertThat( - "One or more validators failed during GTFS import.", - countValidationErrorsOfType(connection, namespace, NewGTFSErrorType.VALIDATOR_FAILED), - equalTo(0) - ); + // Check that no validators failed during validation in non-editor databases. + if (!editorDatabase) { + assertThat( + "One or more validators failed during GTFS import.", + countValidationErrorsOfType(connection, namespace, NewGTFSErrorType.VALIDATOR_FAILED), + equalTo(0) + ); + } // run through testing expectations LOG.info("testing expectations of record storage in the database"); for (PersistenceExpectation persistenceExpectation : persistenceExpectations) { + if (persistenceExpectation.appliesToEditorDatabaseOnly && !editorDatabase) continue; // select all entries from a table String sql = String.format( "select * from %s.%s", @@ -627,8 +809,15 @@ private void assertThatImportedGtfsMeetsExpectations( LOG.error("Persistence mismatch on record {}", numRecordsSearched); } } - assertThatPersistenceExpectationRecordWasFound(numRecordsSearched, foundRecord, fieldsWithMismatches); + assertThatDatabasePersistenceExpectationRecordWasFound( + persistenceExpectation, + numRecordsSearched, + foundRecord, + fieldsWithMismatches + ); } + // Skip error expectation analysis on editor database + if (editorDatabase) return; // Expect zero errors if errorExpectations is null. if (errorExpectations == null) errorExpectations = new ErrorExpectation[]{}; // Check that error expectations match errors stored in database. @@ -695,6 +884,7 @@ private void assertThatExportedGtfsMeetsExpectations( // iterate through all expectations for (PersistenceExpectation persistenceExpectation : persistenceExpectations) { + if (persistenceExpectation.appliesToEditorDatabaseOnly) continue; // No need to check that errors were exported because it is an internal table only. if ("errors".equals(persistenceExpectation.tableName)) continue; final String tableFileName = persistenceExpectation.tableName + ".txt"; @@ -755,7 +945,12 @@ private void assertThatExportedGtfsMeetsExpectations( foundRecord = true; } } - assertThatPersistenceExpectationRecordWasFound(numRecordsSearched, foundRecord, null); + assertThatCSVPersistenceExpectationRecordWasFound( + persistenceExpectation, + tableFileName, + numRecordsSearched, + foundRecord + ); } } @@ -775,33 +970,83 @@ private boolean equalsWithNumericDelta(String val, RecordExpectation recordExpec return Math.abs(d - recordExpectation.doubleExpectation) < recordExpectation.acceptedDelta; } + /** + * Helper that calls assertion with corresponding context for better error reporting. + */ + private void assertThatDatabasePersistenceExpectationRecordWasFound( + PersistenceExpectation persistenceExpectation, + int numRecordsSearched, + boolean foundRecord, + Multimap fieldsWithMismatches + ) { + assertThatPersistenceExpectationRecordWasFound( + persistenceExpectation, + String.format("Database table `%s`", persistenceExpectation.tableName), + numRecordsSearched, + foundRecord, + fieldsWithMismatches + ); + } + + /** + * Helper that calls assertion with corresponding context for better error reporting. + */ + private void assertThatCSVPersistenceExpectationRecordWasFound( + PersistenceExpectation persistenceExpectation, + String tableFileName, + int numRecordsSearched, + boolean foundRecord + ) { + assertThatPersistenceExpectationRecordWasFound( + persistenceExpectation, + String.format("CSV file `%s`", tableFileName), + numRecordsSearched, + foundRecord, + null + ); + } + /** * Helper method to make sure a persistence expectation was actually found after searching through records */ private void assertThatPersistenceExpectationRecordWasFound( + PersistenceExpectation persistenceExpectation, + String contextDescription, int numRecordsSearched, boolean foundRecord, Multimap mismatches ) { + contextDescription = String.format("in the %s", contextDescription); assertThat( - "No records found in the ResultSet/CSV file", + String.format("No records found %s", contextDescription), numRecordsSearched, ComparatorMatcherBuilder.usingNaturalOrdering().greaterThan(0) ); - if (mismatches != null) { + if (!foundRecord && mismatches != null) { for (String field : mismatches.keySet()) { Collection valuePairs = mismatches.get(field); for (ValuePair valuePair : valuePairs) { assertThat( - String.format("The value expected for %s was not found", field), + String.format( + "The value expected for %s was not found %s. NOTE: there could be other values, but the first found value is shown.", + field, + contextDescription + ), valuePair.found, equalTo(valuePair.expected) ); } } } else { + if (!foundRecord) { + LOG.error("Unfound Record:"); + LOG.error(persistenceExpectation.toString()); + } assertThat( - "The record as defined in the PersistenceExpectation was not found.", + String.format( + "The record as defined in the PersistenceExpectation was not found %s.", + contextDescription + ), foundRecord, equalTo(true) ); @@ -964,7 +1209,11 @@ private PersistenceExpectation[] updatePersistenceExpectationsWithNormalizedStop } // Once cloning/updating has been done for all record expectations, add the new table expectation to the // array. - persistenceExpectations[i] = new PersistenceExpectation(inputExpectation.tableName, newRecordExpectations); + persistenceExpectations[i] = new PersistenceExpectation( + inputExpectation.tableName, + newRecordExpectations, + inputExpectation.appliesToEditorDatabaseOnly + ); } return persistenceExpectations; } diff --git a/src/test/java/com/conveyal/gtfs/storage/PersistenceExpectation.java b/src/test/java/com/conveyal/gtfs/storage/PersistenceExpectation.java index ee870b77d..1f133f12e 100644 --- a/src/test/java/com/conveyal/gtfs/storage/PersistenceExpectation.java +++ b/src/test/java/com/conveyal/gtfs/storage/PersistenceExpectation.java @@ -1,5 +1,7 @@ package com.conveyal.gtfs.storage; +import java.util.Arrays; + /** * A helper class to verify that data got stored in a particular table. */ @@ -11,14 +13,29 @@ public class PersistenceExpectation { * tableName. */ public RecordExpectation[] recordExpectations; + public boolean appliesToEditorDatabaseOnly; public PersistenceExpectation(String tableName, RecordExpectation[] recordExpectations) { + this(tableName, recordExpectations, false); + } + + public PersistenceExpectation( + String tableName, + RecordExpectation[] recordExpectations, + boolean appliesToEditorDatabaseOnly + ) { this.tableName = tableName; this.recordExpectations = recordExpectations; + this.appliesToEditorDatabaseOnly = appliesToEditorDatabaseOnly; } public static PersistenceExpectation[] list (PersistenceExpectation... expectations) { return expectations; } + + @Override public String toString() { + return "PersistenceExpectation{" + "tableName='" + tableName + '\'' + ", recordExpectations=" + Arrays + .toString(recordExpectations) + ", appliesToEditorDatabaseOnly=" + appliesToEditorDatabaseOnly + '}'; + } } diff --git a/src/test/java/com/conveyal/gtfs/storage/RecordExpectation.java b/src/test/java/com/conveyal/gtfs/storage/RecordExpectation.java index f43c246d5..52610c911 100644 --- a/src/test/java/com/conveyal/gtfs/storage/RecordExpectation.java +++ b/src/test/java/com/conveyal/gtfs/storage/RecordExpectation.java @@ -96,4 +96,12 @@ public RecordExpectation clone() { copy.doubleExpectation = this.doubleExpectation; return copy; } + + @Override public String toString() { + return "RecordExpectation{" + "acceptedDelta=" + acceptedDelta + ", doubleExpectation=" + doubleExpectation + + ", editorExpectation='" + editorExpectation + '\'' + ", expectedFieldType=" + expectedFieldType + + ", fieldName='" + fieldName + '\'' + ", intExpectation=" + intExpectation + ", stringExpectation='" + + stringExpectation + '\'' + ", stringExpectationInCSV=" + stringExpectationInCSV + + ", editorStringExpectation=" + editorStringExpectation + '}'; + } } diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/agency.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/agency.txt new file mode 100755 index 000000000..a916ce91b --- /dev/null +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/agency.txt @@ -0,0 +1,2 @@ +agency_id,agency_name,agency_url,agency_lang,agency_phone,agency_email,agency_timezone,agency_fare_url,agency_branding_url +1,Fake Transit,,,,,America/Los_Angeles,, diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar.txt new file mode 100755 index 000000000..785f553dc --- /dev/null +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar.txt @@ -0,0 +1,3 @@ +service_id,monday,tuesday,wednesday,thursday,friday,saturday,sunday,start_date,end_date +only-in-calendar-txt,1,1,1,1,1,1,1,20170915,20170917 +in-both-calendar-txt-and-calendar-dates,1,1,1,1,1,1,1,20170918,20170920 diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar_dates.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar_dates.txt new file mode 100755 index 000000000..ea060f019 --- /dev/null +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/calendar_dates.txt @@ -0,0 +1,3 @@ +service_id,date,exception_type +in-both-calendar-txt-and-calendar-dates,20170920,2 +only-in-calendar-dates-txt,20170916,1 \ No newline at end of file diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/fare_attributes.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/fare_attributes.txt new file mode 100755 index 000000000..3173d016d --- /dev/null +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/fare_attributes.txt @@ -0,0 +1,2 @@ +fare_id,price,currency_type,payment_method,transfers,transfer_duration +route_based_fare,1.23,USD,0,0,0 \ No newline at end of file diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/fare_rules.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/fare_rules.txt new file mode 100755 index 000000000..05d2aadf8 --- /dev/null +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/fare_rules.txt @@ -0,0 +1,2 @@ +fare_id,route_id,origin_id,destination_id,contains_id +route_based_fare,1,,, \ No newline at end of file diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/feed_info.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/feed_info.txt new file mode 100644 index 000000000..b617faf49 --- /dev/null +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/feed_info.txt @@ -0,0 +1,2 @@ +feed_publisher_name,feed_publisher_url,feed_lang,feed_version +Conveyal,http://www.conveyal.com,en,1.0 \ No newline at end of file diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/frequencies.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/frequencies.txt new file mode 100755 index 000000000..9baceff3a --- /dev/null +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/frequencies.txt @@ -0,0 +1,2 @@ +trip_id,start_time,end_time,headway_secs,exact_times +frequency-trip,08:00:00,09:00:00,1800,0 \ No newline at end of file diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/routes.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/routes.txt new file mode 100755 index 000000000..35ea7aa67 --- /dev/null +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/routes.txt @@ -0,0 +1,2 @@ +agency_id,route_id,route_short_name,route_long_name,route_desc,route_type,route_url,route_color,route_text_color,route_branding_url +1,1,1,Route 1,,3,,7CE6E7,FFFFFF, diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/shapes.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/shapes.txt new file mode 100755 index 000000000..3f2e3fd13 --- /dev/null +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/shapes.txt @@ -0,0 +1,8 @@ +shape_id,shape_pt_lat,shape_pt_lon,shape_pt_sequence,shape_dist_traveled +5820f377-f947-4728-ac29-ac0102cbc34e,37.0612132,-122.0074332,1,0.0000000 +5820f377-f947-4728-ac29-ac0102cbc34e,37.0611720,-122.0075000,2,7.4997067 +5820f377-f947-4728-ac29-ac0102cbc34e,37.0613590,-122.0076830,3,33.8739075 +5820f377-f947-4728-ac29-ac0102cbc34e,37.0608780,-122.0082780,4,109.0402932 +5820f377-f947-4728-ac29-ac0102cbc34e,37.0603590,-122.0088280,5,184.6078298 +5820f377-f947-4728-ac29-ac0102cbc34e,37.0597610,-122.0093540,6,265.8053023 +5820f377-f947-4728-ac29-ac0102cbc34e,37.0590660,-122.0099190,7,357.8617018 diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/stop_times.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/stop_times.txt new file mode 100755 index 000000000..fe0a9ad12 --- /dev/null +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/stop_times.txt @@ -0,0 +1,7 @@ +trip_id,arrival_time,departure_time,stop_id,stop_sequence,stop_headsign,pickup_type,drop_off_type,shape_dist_traveled,timepoint +non-frequency-trip,07:00:00,07:00:00,4u6g,1,,0,0,0.0000000, +non-frequency-trip,07:01:00,07:01:00,johv,2,,0,0,341.4491961, +non-frequency-trip-2,08:00:00,08:00:00,4u6g,1,,0,0,0.0000000, +non-frequency-trip-2,08:01:00,08:01:00,johv,2,,0,0,341.4491961, +frequency-trip,09:00:00,09:00:00,4u6g,1,,0,0,0.0000000, +frequency-trip,09:01:00,09:01:00,johv,2,,0,0,341.4491961, diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/stops.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/stops.txt new file mode 100755 index 000000000..8e71f7b2e --- /dev/null +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/stops.txt @@ -0,0 +1,3 @@ +stop_id,stop_code,stop_name,stop_desc,stop_lat,stop_lon,zone_id,stop_url,location_type,parent_station,stop_timezone,wheelchair_boarding +4u6g,,Butler Ln,,37.0612132,-122.0074332,,,0,,, +johv,,Scotts Valley Dr & Victor Sq,,37.0590172,-122.0096058,,,0,,, diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/transfers.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/transfers.txt new file mode 100755 index 000000000..357103c47 --- /dev/null +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/transfers.txt @@ -0,0 +1 @@ +from_stop_id,to_stop_id,transfer_type,min_transfer_time diff --git a/src/test/resources/fake-agency-mixture-of-calendar-definitions/trips.txt b/src/test/resources/fake-agency-mixture-of-calendar-definitions/trips.txt new file mode 100755 index 000000000..077253974 --- /dev/null +++ b/src/test/resources/fake-agency-mixture-of-calendar-definitions/trips.txt @@ -0,0 +1,4 @@ +route_id,trip_id,trip_headsign,trip_short_name,direction_id,block_id,shape_id,bikes_allowed,wheelchair_accessible,service_id +1,non-frequency-trip,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,only-in-calendar-dates-txt +1,non-frequency-trip-2,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,only-in-calendar-txt +1,frequency-trip,,,0,,5820f377-f947-4728-ac29-ac0102cbc34e,0,0,in-both-calendar-txt-and-calendar-dates \ No newline at end of file From e659d14e1beeef6ddbc492a4de16eb13f3247de0 Mon Sep 17 00:00:00 2001 From: Landon Reed Date: Tue, 19 May 2020 10:03:23 -0400 Subject: [PATCH 2/3] refactor: suggestions to improve PR #287 --- .../gtfs/loader/JdbcGtfsSnapshotter.java | 21 ++++++++++--------- src/test/java/com/conveyal/gtfs/GTFSTest.java | 17 ++++++++------- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java index 1dd305f16..610a4f8ac 100644 --- a/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java +++ b/src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java @@ -227,7 +227,7 @@ private TableLoadResult createScheduleExceptionsTable() { Iterable calendarDates = calendarDatesReader.getAll(); // Keep track of calendars by service id in case we need to add dummy calendar entries. - Map calendarsByServiceId = new HashMap<>(); + Map dummyCalendarsByServiceId = new HashMap<>(); // Iterate through calendar dates to build up to get maps from exceptions to their dates. Multimap removedServiceForDate = HashMultimap.create(); @@ -243,7 +243,7 @@ private TableLoadResult createScheduleExceptionsTable() { addedServiceForDate.put(date, calendarDate.service_id); // create (if needed) and extend range of dummy calendar that would need to be created if we are // copying from a feed that doesn't have the calendar.txt file - Calendar calendar = calendarsByServiceId.getOrDefault(calendarDate.service_id, new Calendar()); + Calendar calendar = dummyCalendarsByServiceId.getOrDefault(calendarDate.service_id, new Calendar()); calendar.service_id = calendarDate.service_id; if (calendar.start_date == null || calendar.start_date.isAfter(calendarDate.date)) { calendar.start_date = calendarDate.date; @@ -251,7 +251,7 @@ private TableLoadResult createScheduleExceptionsTable() { if (calendar.end_date == null || calendar.end_date.isBefore(calendarDate.date)) { calendar.end_date = calendarDate.date; } - calendarsByServiceId.put(calendarDate.service_id, calendar); + dummyCalendarsByServiceId.put(calendarDate.service_id, calendar); } else { removedServiceForDate.put(date, calendarDate.service_id); } @@ -289,7 +289,8 @@ private TableLoadResult createScheduleExceptionsTable() { calendarServiceIds.add(calendar.service_id); } - // add auto-generated entries to the calendar table that only existed in the calendar_dates table + // For service_ids that only existed in the calendar_dates table, insert auto-generated, "blank" + // (no days of week specified) calendar entries. sql = String.format( "insert into %s (service_id, description, start_date, end_date, " + "monday, tuesday, wednesday, thursday, friday, saturday, sunday)" + @@ -301,23 +302,23 @@ private TableLoadResult createScheduleExceptionsTable() { "calendar", calendarStatement ); - for (Calendar calendar : calendarsByServiceId.values()) { - if (calendarServiceIds.contains(calendar.service_id)) { + for (Calendar dummyCalendar : dummyCalendarsByServiceId.values()) { + if (calendarServiceIds.contains(dummyCalendar.service_id)) { // This service_id already exists in the calendar table. No need to create auto-generated entry. continue; } - calendarStatement.setString(1, calendar.service_id); + calendarStatement.setString(1, dummyCalendar.service_id); calendarStatement.setString( 2, - String.format("%s (auto-generated)", calendar.service_id) + String.format("%s (auto-generated)", dummyCalendar.service_id) ); calendarStatement.setString( 3, - calendar.start_date.format(DateTimeFormatter.BASIC_ISO_DATE) + dummyCalendar.start_date.format(DateTimeFormatter.BASIC_ISO_DATE) ); calendarStatement.setString( 4, - calendar.end_date.format(DateTimeFormatter.BASIC_ISO_DATE) + dummyCalendar.end_date.format(DateTimeFormatter.BASIC_ISO_DATE) ); calendarsTracker.addBatch(); } diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index 3113b0535..1734057fd 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -630,8 +630,7 @@ private boolean runIntegrationTestOnZipFile( // There should be no schema records matching the deleted namespace. assertThat(schemaCount, is(0)); } catch (SQLException | InvalidNamespaceException e) { - LOG.error("An error occurred while deleting a schema!"); - e.printStackTrace(); + LOG.error("An error occurred while deleting a schema!", e); TestUtils.dropDB(testDBName); return false; } catch (AssertionError e) { @@ -730,12 +729,13 @@ private void assertThatImportedGtfsMeetsExpectations( String namespace, PersistenceExpectation[] persistenceExpectations, ErrorExpectation[] errorExpectations, - boolean editorDatabase + boolean isEditorDatabase ) throws SQLException { // Store field mismatches here (to provide assertion statements with more details). Multimap fieldsWithMismatches = ArrayListMultimap.create(); - // Check that no validators failed during validation in non-editor databases. - if (!editorDatabase) { + // Check that no validators failed during validation in non-editor databases only (validators do not run + // when creating an editor database). + if (!isEditorDatabase) { assertThat( "One or more validators failed during GTFS import.", countValidationErrorsOfType(connection, namespace, NewGTFSErrorType.VALIDATOR_FAILED), @@ -745,7 +745,7 @@ private void assertThatImportedGtfsMeetsExpectations( // run through testing expectations LOG.info("testing expectations of record storage in the database"); for (PersistenceExpectation persistenceExpectation : persistenceExpectations) { - if (persistenceExpectation.appliesToEditorDatabaseOnly && !editorDatabase) continue; + if (persistenceExpectation.appliesToEditorDatabaseOnly && !isEditorDatabase) continue; // select all entries from a table String sql = String.format( "select * from %s.%s", @@ -817,7 +817,10 @@ private void assertThatImportedGtfsMeetsExpectations( ); } // Skip error expectation analysis on editor database - if (editorDatabase) return; + if (isEditorDatabase) { + LOG.info("Skipping error expectations for non-editor database."); + return; + } // Expect zero errors if errorExpectations is null. if (errorExpectations == null) errorExpectations = new ErrorExpectation[]{}; // Check that error expectations match errors stored in database. From c83f7e0351315923f6ea6f374c5ddccd35547b2d Mon Sep 17 00:00:00 2001 From: Evan Siroky Date: Tue, 19 May 2020 09:42:56 -0700 Subject: [PATCH 3/3] refactor(test): slight changes to improve reability --- src/test/java/com/conveyal/gtfs/GTFSTest.java | 13 +++++++------ .../gtfs/storage/PersistenceExpectation.java | 3 ++- .../conveyal/gtfs/storage/RecordExpectation.java | 3 ++- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/test/java/com/conveyal/gtfs/GTFSTest.java b/src/test/java/com/conveyal/gtfs/GTFSTest.java index 1734057fd..7ca06cef9 100644 --- a/src/test/java/com/conveyal/gtfs/GTFSTest.java +++ b/src/test/java/com/conveyal/gtfs/GTFSTest.java @@ -1020,11 +1020,14 @@ private void assertThatPersistenceExpectationRecordWasFound( Multimap mismatches ) { contextDescription = String.format("in the %s", contextDescription); + // Assert that more than 0 records were found assertThat( String.format("No records found %s", contextDescription), numRecordsSearched, ComparatorMatcherBuilder.usingNaturalOrdering().greaterThan(0) ); + // If the record wasn't found, but at least one mismatching record was found, return info about the record that + // was found to attempt to aid with debugging. if (!foundRecord && mismatches != null) { for (String field : mismatches.keySet()) { Collection valuePairs = mismatches.get(field); @@ -1041,14 +1044,12 @@ private void assertThatPersistenceExpectationRecordWasFound( } } } else { - if (!foundRecord) { - LOG.error("Unfound Record:"); - LOG.error(persistenceExpectation.toString()); - } + // Assert that the record was found assertThat( String.format( - "The record as defined in the PersistenceExpectation was not found %s.", - contextDescription + "The record as defined in the PersistenceExpectation was not found %s. Unfound Record: %s", + contextDescription, + persistenceExpectation.toString() ), foundRecord, equalTo(true) diff --git a/src/test/java/com/conveyal/gtfs/storage/PersistenceExpectation.java b/src/test/java/com/conveyal/gtfs/storage/PersistenceExpectation.java index 1f133f12e..f0e339252 100644 --- a/src/test/java/com/conveyal/gtfs/storage/PersistenceExpectation.java +++ b/src/test/java/com/conveyal/gtfs/storage/PersistenceExpectation.java @@ -34,7 +34,8 @@ public static PersistenceExpectation[] list (PersistenceExpectation... expectati return expectations; } - @Override public String toString() { + @Override + public String toString() { return "PersistenceExpectation{" + "tableName='" + tableName + '\'' + ", recordExpectations=" + Arrays .toString(recordExpectations) + ", appliesToEditorDatabaseOnly=" + appliesToEditorDatabaseOnly + '}'; } diff --git a/src/test/java/com/conveyal/gtfs/storage/RecordExpectation.java b/src/test/java/com/conveyal/gtfs/storage/RecordExpectation.java index 52610c911..f5470733f 100644 --- a/src/test/java/com/conveyal/gtfs/storage/RecordExpectation.java +++ b/src/test/java/com/conveyal/gtfs/storage/RecordExpectation.java @@ -97,7 +97,8 @@ public RecordExpectation clone() { return copy; } - @Override public String toString() { + @Override + public String toString() { return "RecordExpectation{" + "acceptedDelta=" + acceptedDelta + ", doubleExpectation=" + doubleExpectation + ", editorExpectation='" + editorExpectation + '\'' + ", expectedFieldType=" + expectedFieldType + ", fieldName='" + fieldName + '\'' + ", intExpectation=" + intExpectation + ", stringExpectation='"