Abstract test classes to share common logic between creators #133
Conversation
| amount_of_stops = len(stops['regular']) + len(stops['stations']) | ||
| self.assertEqual( | ||
| # amount_of_stops, self.count_stops, # WORKS FOR ACCRA | ||
| amount_of_stops, self.count_stops + self.count_stations, # WORKS FOR MANAGUA |
There was a problem hiding this comment.
Here we have an incompatibility between Accra and Managua and I would like to ask for your help on this, @prhod and @nlehuby.
For some reason the GTFS for Managua counts stops and stations (stop_areas), whereas Accra only counts the stops and not the stations. However in line 99 explicitly stops and stations are added up. So I guess Managua is following the correct behaviour. But I'm really not sure on this one and how to fix it for Accra.
Do you have an idea?
There was a problem hiding this comment.
Because of some other changes, the code moved: The important part is now at line 141-143
| """ | ||
|
|
||
| # Define selector for this tests | ||
| self.selector = "YOUR SELECTOR" |
There was a problem hiding this comment.
Maybe this would be better with an abstract method?
There was a problem hiding this comment.
This is where my knowledge on Python ends currently. I gave it a try here, but it breaks with error message
TypeError: Can't instantiate abstract class TestCreatorsAbstract with abstract methods setUp
My very wild guess: There seems to be some deep-shit going on because setUp is the initiator function.
Can anybody help me or point me to documentation that helps me? Thanks.
There was a problem hiding this comment.
You would declare a new method get_selector() which throws a NotImplemented exception, so all subclasses need to implement it. Then where you need the selector, you call the method.
There was a problem hiding this comment.
ok, it is implemented in this PR as suggested, with two abstract functions:
_get_selector()_get_required_values()
And optionally can be overridden:
_get_standard_values()_override_configuration()
Is this how you meant it, @grote?
|
Automatic tests currently fail because of different behaviour on handling stations (stop_areas) between Accra and Managua, as described here:
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? |
|
Let's keep this more simple: I introduced a separate variable Now, tests work well (it actually fixes an error which lets the current |
| break | ||
| if not trip_point_found: | ||
| print( | ||
| "No corresponing stop_time for trip_id={} and stop_sequence={}".format( |
There was a problem hiding this comment.
*corresponding.
Maybe matching
There was a problem hiding this comment.
There has been no change on this one in this PR. But I'm happy to improve the wording as suggested.
| "Wrong stop_points count in the generated GTFS") | ||
| self.assertEqual(gtfs_infos["stop_areas_count"], self.required_values['stations_count'], | ||
| "Wrong stop_areas count in the generated GTFS") | ||
| self.assertEqual(gtfs_infos["routes_count"], self.required_values['routes_count'], |
There was a problem hiding this comment.
Can we switch the order of the tests ?
The tests about the required values provide much more useful info to debug and to inspect the potential differences than the is_identical_gtfs one.
There was a problem hiding this comment.
Of course, but probably better to do in a separate PR.
| """ | ||
| raise NotImplementedError | ||
|
|
||
| def _get_standard_values(self): |
There was a problem hiding this comment.
I don't like values. Maybe variables or test_config ?
There was a problem hiding this comment.
Sure, I can rename it to variables.
|
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. |
|
@grote Any chance to get this in at some point? |
|
Thanks for the work and the reminder! |
|
Thank you |
As a follow up to #131, this PR proposes to create a class
CreatorsTestsAbstractwith the general testing logic for providers/creators, which was before implemented very similarily in thegh_accraandni_managuaproviders. With this PR, the providers just have to extend from this class and define the necessary variables through overridable methods:_get_selector()(required)_get_required_variables()(required)_get_standard_variables()(optional)_override_configuration()(optional)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.