geocoder-php / Geocoder

The most featured Geocoder library written in PHP.
https://geocoder-php.org
MIT License
3.94k stars 517 forks source link

Updated ArcGISOnline to migrate , add support, and update response handling. #1090

Closed wantell closed 3 years ago

wantell commented 3 years ago
jbelien commented 3 years ago

Thanks @wantell !

Could you make sure the previous tests still pass so we are sure there is no breaking change ? But also update the tests related to /find are updated (only if the /findAddressCandidates response structure is different than the /find response structure) ?

jbelien commented 3 years ago

Could you also add a test for the /geocodeAddresses query (and commit the cached response) ? Thanks.

wantell commented 3 years ago

@jbelien

Could you also add a test for the /geocodeAddresses query (and commit the cached response) ? Thanks.

I don't work with tests normally so am unclear what you mean by committing the cached response. Can you please clarify that?

As for the query, I can create and add a token and store it for the test, but once the token expires the test will no longer work. Is this ok? Do you want a test for when an invalid token is passed?

jbelien commented 3 years ago

I don't work with tests normally so am unclear what you mean by committing the cached response. Can you please clarify that?

You will have to run the test locally with your token. Check how it's done in the Google Maps provider, for instance : test + config. Set your valid token in phpunit.xml.dist file but do not commit your token of course (you can commit the file with YOUR_ARCGIS_TOKEN value for instance). Once you have added your test and set your token in phpunit.xml.dist, you can run composer test to run the tests locally (you need to be in the provider folder). It will query the API, update the cached response, and create the new cached response (of your new test).

As for the query, I can create and add a token and store it for the test, but once the token expires the test will no longer work. > Is this ok? Do you want a test for when an invalid token is passed?

This is why we cache the API response, so the test works without needing a token. Once the API response is cached, the test with use that response instead of requesting the API.

wantell commented 3 years ago

@jbelien,

I have updated and run all the tests in my local. Mostly these were changes to the order of results and specific values in those results for the migration of find to findAddressCandidates.

I did add these two tests for the token service:

I also updated the Readme, using similar language to the Google Maps for Business Readme.

Please let me know if there are any further items you need me to address with this request, and I thank you for your help and patience.

jbelien commented 3 years ago

There seems to be too many cached response (15 new files, none updated, none removed). Could you delete all the files in Tests/.cached_responses, run the tests locally, and commit the updated cached responses ?

I also see 2 504 Gateway Time-out responses, could you make sure all cached responses are valid ?

Thanks.

wantell commented 3 years ago

There seems to be too many cached response (15 new files, none updated, none removed). Could you delete all the files in Tests/.cached_responses, run the tests locally, and commit the updated cached responses ?

I also see 2 504 Gateway Time-out responses, could you make sure all cached responses are valid ?

Thanks.

will do.

jbelien commented 3 years ago

@wantell Apparently, you forgot to commit the cached response for ArcGISOnlineTest::testGeocodeWithToken.

Could you do it so the test can pass ? See https://travis-ci.org/github/geocoder-php/arcgis-online-provider/builds/741060641#L398

EDIT: Seems to be commited (geocode.arcgis.com_1231a8e79c63a3e1f4a4e650d43739204679e436) but when I run it locally it looks for geocode.arcgis.com_95d04d6e8cecf0ff143350d487232c139a2c4c9e. I'll investigate! 🤔

jbelien commented 3 years ago

@wantell Apparently, you forgot to commit the cached response for ArcGISOnlineTest::testGeocodeWithToken.

Could you do it so the test can pass ? See https://travis-ci.org/github/geocoder-php/arcgis-online-provider/builds/741060641#L398

EDIT: Seems to be commited (geocode.arcgis.com_1231a8e79c63a3e1f4a4e650d43739204679e436) but when I run it locally it looks for geocode.arcgis.com_95d04d6e8cecf0ff143350d487232c139a2c4c9e. I'll investigate! 🤔

Fixed: https://github.com/geocoder-php/Geocoder/commit/f2fa3c788af5c11079b71ce43bb85188d46db3d3

wantell commented 3 years ago

Fixed: f2fa3c7

Thanks for taking care of that, I don't think I would've been able to identify the problem.