Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to test all API endpoints using SystemTestCase::runApiTests #8188

Closed
kaz231 opened this issue Jun 23, 2015 · 6 comments
Closed

Allow to test all API endpoints using SystemTestCase::runApiTests #8188

kaz231 opened this issue Jun 23, 2015 · 6 comments
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues.
Milestone

Comments

@kaz231
Copy link

kaz231 commented Jun 23, 2015

Steps to reproduce:

  1. Create new test, that extends SystemTestCase
  2. Use runApiTest($apiMethod, queryParams) where $apiMethod should start with different prefix than "get"

Result:

Exception: Only generated 0 API calls to test but was expecting more for this test.
Want to test APIs: $apiMethod)
But only generated these URLs: 
)

I found that problem is in class https://github.com/piwik/piwik/blob/master/tests/PHPUnit/Framework/TestRequest/Collection.php#L305. There's is condition, that checks if API method starts with "get" and if not, then skips it.

@kaz231
Copy link
Author

kaz231 commented Jun 23, 2015

Would be also useful to:

@mattab
Copy link
Member

mattab commented Jul 9, 2015

Thanks @kaz231 for the report!

We can definitely improve things, in particular:

  • when a developer writes system test for a method not starting with get then write a clear message indicating to use integration test instead.
    • (currently our system test are designed to collect data via fixture and then call reporting API get* methods. all other API methods are tested with integration tests)

regarding removing the hacks: sure it would be nice to remove such technical debt, but if it's not an issue then we don't need to do it yet. Similarly we don't need to change the logic to not load all available API methods, since that's not a performance issue.

@mattab mattab added this to the 2.14.1 milestone Jul 9, 2015
@mattab mattab added the c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. label Jul 9, 2015
@kaz231
Copy link
Author

kaz231 commented Jul 9, 2015

@mattab thanks for answer !

Re usage of IntegrationTestCase instead of SystemTestCase won't solve the problem, because IntegrationTestCase extends over SystemTestCase.

Re hacks: I totally agree, that's a minor. But it will be great to progressively eliminate them.

@mattab
Copy link
Member

mattab commented Jul 9, 2015

What I meant is that to test an API method, we should not use runApiTests but rather write normal integration tests (like you would write integration tests usually), see for example: https://github.com/piwik/plugin-CustomAlerts/blob/master/tests/Integration/ApiTest.php

@mattab
Copy link
Member

mattab commented Jul 9, 2015

But it will be great to progressively eliminate them.

Definitely agree :-) feel free to submit pull requests when you stumble upon and want / can easily fix one tech debt. Reporting them is not directly helpful as it's never ending story :-)

@mattab mattab closed this as completed in f9f42f6 Jul 13, 2015
@mattab
Copy link
Member

mattab commented Jul 13, 2015

@kaz231 I did a small fix in f9f42f6 - hopefully this would prevent someone having same problem in the future. Please keep these suggestions coming!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues.
Projects
None yet
Development

No branches or pull requests

2 participants