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

Refactoring our test suites: use HTTP requests in one or two system tests, not every system test #6473

Closed
4 tasks
diosmosis opened this issue Oct 20, 2014 · 2 comments
Labels
Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. wontfix If you can reproduce this issue, please reopen the issue or create a new one describing it.
Milestone

Comments

@diosmosis
Copy link
Member

Currently system tests (through Fixtures) will invoke many HTTP requests via curl to track visits. Instead of doing this in EVERY system test, we should create two new system tests that use HTTP (one for the normal HTTP tracker API, and another for bulk tracking). Then change all fixtures to use LocalTracker. This will hopefully speed up tests and make it easier to maintain the old system tests.

TODO:

  • create HttpTrackingTest. We should look through all existing fixtures and see how the HTTP API is tested and make sure those tests that require being tested through an HTTP request are replicated in the new test.
  • create BulkTrackingTest. Do the same as above just for bulk tracking requests only.
  • switch to LocalTracker in all Fixtures. Should probably be enough to change Fixture::getTracker
  • make sure there are no major drops in code coverage

Notes:

  • LocalTracker was written a long time ago as a hack to improve test speed. Sometime in the future when the tracker is refactored, we should switch to doing something like:
$tracker = new \Piwik\Tracker();
$tracker->track(...);

CC-ing @tsteur since you've been thinking about refactoring tests for a while now.

@diosmosis diosmosis added the Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. label Oct 20, 2014
@diosmosis diosmosis added this to the Short term milestone Oct 20, 2014
@tsteur
Copy link
Member

tsteur commented Oct 20, 2014

What I have in mind is getting rid of all SystemTests except maybe 10 or 50 ;) Long term...

Not sure if LocalTracker would make it much faster but curious. Maybe you can test with one or two tests easily whether there is a big difference?

@diosmosis
Copy link
Member Author

I doubt it will make it that much faster, in my past experience it was a tiny bit faster than using a bulk tracking request. Anyway that's more of a side effect, the main improvement is better scoped tests.

I don't consider this the only thing to do to the tests, but I think it's a good first step to take to clean up the test suites, w/o losing our existing coverage.

@mattab mattab modified the milestones: Mid term, Short term Dec 1, 2014
@mattab mattab closed this as completed Jul 8, 2016
@mattab mattab added the wontfix If you can reproduce this issue, please reopen the issue or create a new one describing it. label Jul 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. wontfix If you can reproduce this issue, please reopen the issue or create a new one describing it.
Projects
None yet
Development

No branches or pull requests

3 participants