Skip to content

[PHP] fix PHPUnit invocation, add basic phpunit.xml.dist#3864

Merged
wing328 merged 2 commits intoswagger-api:masterfrom
dkarlovi:fix/phpunit-setup
Sep 28, 2016
Merged

[PHP] fix PHPUnit invocation, add basic phpunit.xml.dist#3864
wing328 merged 2 commits intoswagger-api:masterfrom
dkarlovi:fix/phpunit-setup

Conversation

@dkarlovi
Copy link
Copy Markdown
Contributor

PR checklist

  • Read the contribution guildelines.
  • Ran the shell/batch script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates)
  • Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

This PR fixes the PHPUnit setup in PHP client. The new setup provides a basic phpunit.xml file which allows for more simple invocation.

Also, .travis used a hardcoded path to test folder which is not necessarily valid with directory name config changes.

To verify it works, just run:

# swagger-codegen as usual

# install Composer depencencies
composer install

# run PHPUnit with predefined config in phpunit.xml.dist
vendor/bin/phpunit

@dkarlovi dkarlovi changed the title feature(phpunit) fix PHPUnit invocation, add basic phpunit.xml.dist [PHP] fix PHPUnit invocation, add basic phpunit.xml.dist Sep 24, 2016
@wing328 wing328 added this to the v2.2.2 milestone Sep 26, 2016
@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Sep 26, 2016

@dkarlovi thanks for the contribution. The change looks good to me.

cc @arnested

stopOnFailure="false">
<testsuites>
<testsuite>
<directory>SwaggerClient-php/test/Api</directory>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmmm...

If I run vendor/bin/phpunit no tests are executed.

If I drop SwaggerClient-php/ from the paths in this file the tests run fine.

A change to phpunit.xml.mustache could be to prefix {{apiTestPath}} etc. with ../ -- but maybe you have a bette alternative.

Overall this looks like a good improvement!

We should probably adjust samples/client/petstore/php/SwaggerClient-php/pom.xml as well:

diff --git a/samples/client/petstore/php/SwaggerClient-php/pom.xml b/samples/client/petstore/php/SwaggerClient-php/pom.xml
index faaeba8..5a0007c 100644
--- a/samples/client/petstore/php/SwaggerClient-php/pom.xml
+++ b/samples/client/petstore/php/SwaggerClient-php/pom.xml
@@ -47,9 +47,6 @@
                         </goals>
                         <configuration>
                             <executable>vendor/bin/phpunit</executable>
-                            <arguments>
-                                <argument>test</argument>
-                            </arguments>
                         </configuration>
                     </execution>
                 </executions>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see the problem, I've tested it with my own config file which sets srcBasePath: . so getPackagePath() is not required (as phpunit.xml.dist gets created inside it, not beside it). I'll fix it, along with the file you suggested.

Thanks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@arnested seems alright now, I've tested it with all kinds of permutations, should work fine.

Copy link
Copy Markdown
Contributor

@arnested arnested left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Sep 28, 2016

Did a test with a new PHP SDK, which works fine as expected.

@wing328 wing328 merged commit 10d3dea into swagger-api:master Sep 28, 2016
@dkarlovi dkarlovi deleted the fix/phpunit-setup branch September 28, 2016 11:58
acramatte added a commit to comerge/swagger-codegen that referenced this pull request Oct 4, 2016
* upstream/master: (79 commits)
  add undertow
  Add a new cli command to output version information (2nd attempt) swagger-api#3892 (swagger-api#3899)
  fix python flask controller without tag (default_controller)
  [aspnet5] Fix basePath application to operations (swagger-api#3911)
  Bugfix/issue 3723 (swagger-api#3726)
  Cgardens nested object regex (swagger-api#3879)
  [Cpprest] Fixing issue swagger-api#3773 (swagger-api#3876)
  escape callback parameter for java(okhttp) and python
  fix warning in html generator
  [PHP] fix PHPUnit invocation, add basic phpunit.xml.dist (swagger-api#3864)
  [Java] Remove duplicated model description in Spring, JAX-RS models (swagger-api#3887)
  [PHP] Better PSR2 compatibility (swagger-api#3863)
  Mention security script in pull request template
  [Swift] Use thread safe manager dictionary
  Replace ^M with new line (\r) in mustache template (swagger-api#3865)
  [swfit] fix url param with base name
  [JaxRS]Show correct default value on CLI option description (swagger-api#3862)
  add title, description to HTML output (swagger-api#3860)
  fix trailing comma in go api client
  fix typescript-fetch base path by removing ending slash
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants