Closed pantierra closed 6 years ago
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.
Updated based on latest upstream. Now this PR only includes the respective commit and is easier to review, for your convenience.
Rebased on latest master
.
This is awaiting review from @AltNico it seems.
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.