Repair Shared Stops Validator #584
Conversation
src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java
Outdated
Show resolved
Hide resolved
src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java
Outdated
Show resolved
Hide resolved
src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java
Outdated
Show resolved
Hide resolved
src/main/java/com/conveyal/datatools/manager/jobs/validation/SharedStopsValidator.java
Outdated
Show resolved
Hide resolved
| int FEED_ID_INDEX = -1; | ||
|
|
||
| try { | ||
| configReader.setHeaders(new String[]{"stop_group_id", "feed_id", "stop_id", "is_primary"}); |
There was a problem hiding this comment.
My understanding is that you are setting the headers (and presumably the order) and then checking the index of each. I think the index will always be the same for each header as defined here.
Also, if the order in project.sharedStopsConfig doesn't match this, the headers will be assigned to the wrong data column. To fix (assuming my understanding of this is correct) is to remove this line and therefore use the headers as they are defined (rightly or wrongly) in project.sharedStopsConfig. A unit test or two around this would be really useful.
There was a problem hiding this comment.
Do we have examples of unit testing other validators? I've manually created some tests to fix the behavior issues you're talking about but I can't find a good way to split things to reasonably test them. Either way the issues you've mentioned have been fixed. Using split.
There was a problem hiding this comment.
@miles-grant-ibigroup Look to move the contents of validate() method into a static method or two so these can be called from a unit test to avoid having to go through the whole feed/validation process.
See https://github.com/ibi-group/gtfs-lib/blob/dev-flex/src/main/java/com/conveyal/gtfs/validator/FlexValidator.java and https://github.com/ibi-group/gtfs-lib/blob/dev-flex/src/test/java/com/conveyal/gtfs/validator/FlexValidatorTest.java.
There was a problem hiding this comment.
I've given this an attempt. Let me know what you think
There was a problem hiding this comment.
@miles-grant-ibigroup I hope you don't mind, but I have created a PR off of this PR: #585 and made a few changes. Let me know if you have any questions. If not, you can merge that back into this PR.
|
Thanks for the feedback! All addressed |
Updates to enum use and parameterized unit tests
| @@ -33,10 +33,10 @@ jobs: | |||
| java-version: 19 | |||
| distribution: 'temurin' | |||
| # Install node 14 for running e2e tests (and for maven-semantic-release). | |||
There was a problem hiding this comment.
Nit update to 20 or remove 14.
Checklist
devbefore they can be merged tomaster)Description
The shared stops validator was built on incomplete data. As a result, most of the validators didn't work correctly. This PR resolves these, adding support for the file in the way it was meant to be used.
Also adds support for the columns being in any order