-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add acceptance tests for Esteli, Nicaragua #136
Conversation
Not sure if I'm wrong, but isn't this testing exactly the same (standard) creator as the already accepted PR #131 ? Instead of adding the same test for each one of the creators which use the standard creator's approach, wouldn't it be more straightforward and simple to only add a unit test for the standard creator once? Of course it's important to add unit test for all creators that don't use the standard creator such as #137 and #135. |
Good point! I don't have a strong opinion (yet). We could just have one test for a provider using the standard creators, or a test for each provider, and it doesn't matter, if it uses a custom or a standard creator. I see advantages and disadvantages for both approaches. Having tests for all, assures a bit more test coverage (as providers can still be different even when using the standard creators), but it is clearly adding up time to the tests to run. |
Having an own test also allows providers to extend the class for particular tests. At the end, the more I think about it, the more I opt for having tests for each provider. |
#129 is another use case where separate tests would be useful, wouldn't it? |
Updated based on latest upstream. Now this PR only includes the respective commit and is easier to review, for your convenience. |
Rebased on latest |
This is awaiting review from @AltNico it seems. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the changes but did not test the code locally. Thank you for that tests, @xamanu, especially for the Overpass magic in queries.txt which I feel like I will never be able to do on my own... Highly appreciated!
Thanks @xamanu and @AltNico ! |
This PR is based on #134. It introduces a non-regression, acceptance tests for the Esteli provider. A review of at least one the owners of the creator (@ialokim and @AltNico) is highly appreciated and a precondition to get this merged in.