grote / osm2gtfs

Turn OpenStreetMap data and schedule information into GTFS
GNU General Public License v3.0
98 stars 31 forks source link

Abstract test classes to share common logic between creators #133

Closed pantierra closed 6 years ago

pantierra commented 6 years ago

As a follow up to #131, this PR proposes to create a class CreatorsTestsAbstract with the general testing logic for providers/creators, which was before implemented very similarily in the gh_accra and ni_managua providers. With this PR, the providers just have to extend from this class and define the necessary variables through overridable methods:

Unfortunately the comparing diff tool seems to be quite overwhelmed by these kind of changes, and it is difficult to see the changes. Looking at the code, without using the diff tool is probably easier.

pantierra commented 6 years ago

Automatic tests currently fail because of different behaviour on handling stations (stop_areas) between Accra and Managua, as described here:

AssertionError: Wrong count of stops in the cache file

I'd prefer to come to a consistent solution for both providers. Alternatively we would have to add another variable to be set to use in the critical position. What do you think how we can solve this best?

pantierra commented 6 years ago

Let's keep this more simple: I introduced a separate variable stops_osm_count for this case, where Accra and Managua differ. The reason seems to be: Managua uses OSM stop_areas to create stations, while Accra does a proximity search to define stations. I think there is no way out but defining this other variable. Or such improvement should happen in a separate PR.

Now, tests work well (it actually fixes an error which lets the current master fail).

pantierra commented 6 years ago

Thanks for your reviews @prhod and @nlehuby!

pantierra commented 6 years ago

Rerolled with the latest suggestions. Further I included the query print-outs from #134, because it makes much more sense to have them in this PR.

From my side, this seems to be ready to merge in.

pantierra commented 6 years ago

@grote Any chance to get this in at some point?

grote commented 6 years ago

Thanks for the work and the reminder!

pantierra commented 6 years ago

Thank you