Allow to test all API endpoints using SystemTestCase::runApiTests

matomo-org / matomo

Empowering People Ethically with the leading open source alternative to Google Analytics that gives you full control over your data. Matomo lets you easily collect data from websites & apps and visualise this data and extract insights. Privacy is built-in. Liberating Web Analytics. Star us on Github? +1. And we love Pull Requests!

https://matomo.org/

GNU General Public License v3.0

19.94k stars 2.66k forks source link

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

Closed kaz231 closed 9 years ago

kaz231 commented 9 years ago

Steps to reproduce:

Create new test, that extends SystemTestCase
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 commented 9 years ago

Would be also useful to:

remove other hacks like hard-coded functions which are actually excluded ( https://github.com/piwik/piwik/blob/master/tests/PHPUnit/Framework/TestRequest/Collection.php#L308 )
change the logic to not load all available API methods but just test those defined in test cases ( https://github.com/piwik/piwik/blob/master/tests/PHPUnit/Framework/TestRequest/Collection.php#L281 )

mattab commented 9 years ago

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.

kaz231 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

@kaz231 I did a small fix in https://github.com/piwik/piwik/commit/f9f42f670b7d6861d35daf8efcc89e387552f5db - hopefully this would prevent someone having same problem in the future. Please keep these suggestions coming!