Skip to content

Conversation

@arubacao
Copy link
Contributor

Use phpunit from vendor to prevent issues
don't suggest to rename phpunit.xml.dist
https://phpunit.de/manual/current/en/organizing-tests.html#organizing-tests.xml-configuration
If there is no phpunit.xml then phpunit will use phpunit.xml.dist , so whats the point in renaming it?

Use phpunit from vendor to prevent issues
don't suggest to rename `phpunit.xml.dist` 
https://phpunit.de/manual/current/en/organizing-tests.html#organizing-tests.xml-configuration
If there is no `phpunit.xml` then phpunit will use `phpunit.xml.dist` , so whats the point in renaming it?

You'll obtain some _skipped_ unit tests due to the need of API keys.

Rename the `phpunit.xml.dist` file to `phpunit.xml`, then uncomment the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One should rename the phpunit.xml.dist file to phpunit.xml to avoid accidentally checking in your API keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, didn't see that in the gitignore. Elegant way to ensure security

README.md Outdated

```
$ phpunit
$ ./vendor/bin/phpunit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better: We should use a composer script so we can run:

composer test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add this tomorrow

- use composer script for unittests
- align readme
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Thank you!

@Nyholm Nyholm merged commit 0a1c75b into geocoder-php:master Dec 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants