Skip to content

Conversation

@wantell
Copy link
Contributor

@wantell wantell commented Oct 29, 2020

  • Migrated find to findAddressCandidates
  • Added support for token in conjunction with geocodeAddresses
  • Updated JSON parsing
  • Modified buildQuery to conditionally add parameters for findAddressCandidates

@jbelien
Copy link
Member

jbelien commented Oct 30, 2020

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
Copy link
Member

jbelien commented Oct 30, 2020

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

@jbelien jbelien linked an issue Oct 30, 2020 that may be closed by this pull request
@wantell
Copy link
Contributor Author

wantell commented Oct 30, 2020

@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
Copy link
Member

jbelien commented Oct 30, 2020

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
Copy link
Contributor Author

wantell commented Oct 30, 2020

@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:

  • testGeocodeWithToken
  • testGeocodeWithInvalidToken
    I was unable to get the ARCGIS_TOKEN value to pull from the cached query when I removed the token in my local and re-ran the test. I'm hoping that is a difference in how Travis CI is able to run it, but I can follow up on it if there are still failing tests.

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
Copy link
Member

jbelien commented Nov 1, 2020

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
Copy link
Contributor Author

wantell commented Nov 2, 2020

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 jbelien merged commit 5913784 into geocoder-php:master Nov 3, 2020
@jbelien
Copy link
Member

jbelien commented Nov 3, 2020

@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
Copy link
Member

jbelien commented Nov 3, 2020

@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: f2fa3c7

@wantell
Copy link
Contributor Author

wantell commented Nov 3, 2020

Fixed: f2fa3c7

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

@wantell wantell deleted the arcGISupdate-1089 branch November 3, 2020 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Alternative for ArcGIS provider

2 participants