geocoder-php / geocoder-extra

Geocoder extra features.
MIT License
52 stars 13 forks source link

Updating and enhancing unit tests #41

Closed vicchi closed 8 years ago

vicchi commented 8 years ago

The current tests are based on PHPUnit ^4.8. PHPUnit's current version is 5.5. We should update to the latest version, with the caveat that a lot of the current test code will need to be fixed as the support for mocking in PHPUnit has changed significantly between v4.x and v5.x. I'm quite happy to do this though.

Also, a lot of the tests are skipped as we don't have API keys for the providers. I'd like to suggest that for free services we register a specific email address (I'm happy to host this on my domain) to create accounts and use Travis's support for encrypted credentials/environment variables so we can expand the number of tests that run by default (10 are currently skipped in this repo). For paid services, it's also worth approaching the providers to see if we can get a free key for this purpose; some may be amenable to this, some may not be, but it's worth asking.

Throwing this open for comments and general discussion, with the additional comment that this also applies equally to the main Geocoder repo as well as this repo.

Nyholm commented 8 years ago

There is an issue when updating phpunit since we support php 5.4. I believe we could support pphunit 4.x and 5.x.

About the skipped tests: sure, that sounds great. Not sure about the paid services but go for it!

vicchi commented 8 years ago

There is an issue when updating phpunit since we support php 5.4. I believe we could support pphunit 4.x and 5.x.

Homebrew on my Mac makes it pretty easy to have multiple versions of packages such as PHP and PHPUnit installed concurrently and to switch between them. As long as I can ensure we can test consistently across all supported versions we should be good. But there's only one way to find out. I'll park this until after I've finished my new provider PRs and then take a look.

About the skipped tests: sure, that sounds great. Not sure about the paid services but go for it!

It's been my experience that some do, some don't. It's always worth asking and even if one provider says "yes" then that's one less skipped test.

Nyholm commented 8 years ago

Good. I'm :+1:

Keep up the good job!

vicchi commented 8 years ago

After doing a bit of research, whilst it's conceptually possible to have multiple versions of PHP and PHPUnit installed, both Composer (https://github.com/composer/composer/issues/2167) and Travis (https://github.com/travis-ci/travis-ci/issues/406) do not and have no plans to support multiple versions of a dependency.

There are some workarounds to this, but all of the ones I've toyed with mentally end up as horrible kludges and aren't something I'd like to see implemented.

Currently, PHPUnit 4.x supports PHP 5.3 through 5.6, whilst PHPUnit 5.x only supports PHP 5.6 and 7. So while we continue to support PHP 5.3 through 5.5, it looks like there's very little alternative but to continue using PHPUnit 4.x.

Which brings to mind 2 questions ...

  1. PHPUnit 4.x doesn't seem to officially support PHP 7, which we do test against. When a PHP 7.x release comes along which breaks PHPUnit 4.x, we'll either have to migrate to PHPUnit 5.x and thus drop support for PHP < 5.6, or find an alternative test suite. Both of which are unattractive propositions.
  2. PHP 5.4 was released in March 2012 and end-of-life'd in September 2015. PHP 5.5 was released in June 2013 and end-of-life'd in July of this year. With PHP 7.1 being slated for release in November of this year, how long should we continue to support these obsoleted versions of PHP?
willdurand commented 8 years ago

Honestly, I am fine with dropping support for PHP 5.4 and 5.5.

vicchi commented 8 years ago

But that would still leave PHP 5.5 supported and thus we can't migrate to PHPUnit 5.x, yet. Perhaps we could consider obsoleting support for PHP 5.4 and 5.5 when PHP 7.1 is released later this year? That would allow us to continue to support 5.6 and 7.0 as well as 7.1. Otherwise we'll have to assume that PHPUnit 4.x will continue to run under PHP 7.1 as well, but we can only wait to find out on that count.

willdurand commented 8 years ago

I have edited my comment before you sent yours ;-)

vicchi commented 8 years ago

I can see that! In that case, I'll make my next piece of work updating the unit tests to support PHPUnit 5.x and also to make a first pass at getting official geocoder-php accounts for those providers that require authentication and to have those accounts used, in encrypted form, in the Travis builds.

vicchi commented 8 years ago

I'll leave this issue open until this is actually implemented by the way!

willdurand commented 8 years ago

👍

willdurand commented 8 years ago

@vicchi Ah wait.

People at @geocoder-php/geocoder, are you ok with dropping PHP 5.4/5.5 support for Geocoder 4?

Geocoder 3 will still support 5.4/5.5, which means we should release a version of geocoder-extra for Geocoder 3 with support of both PHP 5.4 and 5.5.

Next major version of this package will require Geocoder 4 and drop PHP 5.4/5.5 support.

Ok?

vicchi commented 8 years ago

In that case, can we come up with a branching strategy to handle the upcoming release for v3 of the main repo and an aligned version number for this repo, so that I can then branch from that release branch (a sort of hybrid git flow model if you will)?

willdurand commented 8 years ago

Do we want to align major version numbers by the way?

vicchi commented 8 years ago

I think it would be a good idea as geocoder-extras relies on Geocoder for pretty much everything apart from the provider specific implementation. It would also make these two repos seems to be part of the same project, from a perception basis. There may well be version drift again in the future, but that's part of life, but it does no harm to align them now.

vicchi commented 8 years ago

I've implemented a modified version of git flow at work which is working well with a medium sized, distributed, development team. Happy to share that via email as a starting point?

Baachi commented 8 years ago

@willdurand why not bump to PHP >=7?

Nyholm commented 8 years ago

why not bump to PHP >=7?

That would be great... But PHP 5.6 is still supported. There is no rush for us to update.

vicchi commented 8 years ago

I'm working on this as a side project but for now I'll close this issue and reference it in any upcoming PR.