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

Jump to bottom

Phpunit speedtrap #12763

Closed
wants to merge 7 commits into from
Closed

Phpunit speedtrap #12763

wants to merge 7 commits into from

Conversation

szepeviktor
Copy link
Contributor

@szepeviktor szepeviktor commented Apr 23, 2018 •
edited

The purpose of this Pr is to measure long-running unit tests.

Was #12760

@szepeviktor
Copy link
Contributor Author

Results

SystemTests

You should really fix these slow tests (>500ms)...
 1. 215204ms to run Piwik\Tests\System\ArchiveCronTest:testArchivePhpCron
 2. 59605ms to run Piwik\Tests\System\TwoVisitorsTwoWebsitesDifferentDaysTest:testApi with data set #7
 3. 54062ms to run Piwik\Tests\System\AutoSuggestAPITest:testAnotherApi with data set #92
 4. 51416ms to run Piwik\Tests\System\AutoSuggestAPITest:testAnotherApi with data set #96
 5. 51372ms to run Piwik\Tests\System\AutoSuggestAPITest:testAnotherApi with data set #90
 6. 50215ms to run Piwik\Tests\System\AutoSuggestAPITest:testAnotherApi with data set #74
 7. 48881ms to run Piwik\Tests\System\AutoSuggestAPITest:testAnotherApi with data set #72
 8. 48671ms to run Piwik\Tests\System\AutoSuggestAPITest:testAnotherApi with data set #94
 9. 47777ms to run Piwik\Tests\System\AutoSuggestAPITest:testAnotherApi with data set #66
 10. 47513ms to run Piwik\Tests\System\AutoSuggestAPITest:testAnotherApi with data set #58
...and there are 202 more above your threshold hidden from view
Time: 32.84 minutes, Memory: 368.00MB

@szepeviktor
Copy link
Contributor Author

szepeviktor commented Apr 23, 2018 •
edited

Something very fundamental takes ~50 seconds per unit test

@mattab mattab added the Needs Review PRs that need a code review label Apr 24, 2018
@mattab mattab added this to the 3.6.0 milestone Apr 24, 2018
@mattab mattab modified the milestones: 3.6.0, 3.5.1 May 8, 2018
@mattab mattab added the c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. label May 8, 2018
@mattab
Copy link
Member

mattab commented May 8, 2018 •
edited

Thank you @szepeviktor for creating your first PR, that's really useful 👍 making our tests faster has become a top priority as the System tests often fail as they exceed 40min.

From this output it looks like possibly Piwik\Tests\System\AutoSuggestAPITest:testAnotherApi should be tackled first as this causes probably 15+min or more in itself. (these tests check that each dimension can be used as segment, and have at least a visit matching in the fixture.)

You should really fix these slow tests (>500ms)...
 1. 215204ms to run Piwik\Tests\System\ArchiveCronTest:testArchivePhpCron
 2. 59605ms to run Piwik\Tests\System\TwoVisitorsTwoWebsitesDifferentDaysTest:testApi with data set #7
 3. 54062ms to run Piwik\Tests\System\AutoSuggestAPITest:testAnotherApi with data set #92
 4. 51416ms to run Piwik\Tests\System\AutoSuggestAPITest:testAnotherApi with data set #96
 5. 51372ms to run Piwik\Tests\System\AutoSuggestAPITest:testAnotherApi with data set #90
 6. 50215ms to run Piwik\Tests\System\AutoSuggestAPITest:testAnotherApi with data set #74
 7. 48881ms to run Piwik\Tests\System\AutoSuggestAPITest:testAnotherApi with data set #72
 8. 48671ms to run Piwik\Tests\System\AutoSuggestAPITest:testAnotherApi with data set #94
 9. 47777ms to run Piwik\Tests\System\AutoSuggestAPITest:testAnotherApi with data set #66
 10. 47513ms to run Piwik\Tests\System\AutoSuggestAPITest:testAnotherApi with data set #58

edit: @szepeviktor would it be maybe possible to display the top 100 slow tests instead of top 10?

@szepeviktor
Copy link
Contributor Author

szepeviktor commented May 8, 2018 •
edited

@mattab Do you have a 1 page summary on how to get from cloning matomo repo to running tests?
Maybe a Dockerfile??

@sgiehl
Copy link
Member

sgiehl commented May 8, 2018

this might help: https://github.com/matomo-org/matomo/blob/3.x-dev/tests/README.md

@szepeviktor
Copy link
Contributor Author

I think I need to create a Dockerfile to be able to breath

@szepeviktor
Copy link
Contributor Author

szepeviktor commented May 8, 2018 •
edited

Could you help me how piwik_tests db gets populated on Travis?

@szepeviktor
Copy link
Contributor Author

Adding comments to the testing system (travis.yml, shell scripts) may help a lot.

@diosmosis
Copy link
Member

@szepeviktor I use docker half of the time for development: https://github.com/diosmosis/matomo-docker-dev . It's definitely not official, so it may or may not work for you, I can't promise any real support and I will make changes to it as I need them.

Use docker-compose up to start it up (and all the other containers I use when developing). Install matomo by visiting localhost:3000, and run commands via the console script in the readme, eg, /path/to/matomo-docker-dev/console tests:run ./tests/PHPUnit/System/OneVisitorTwoVisitsTest.php (this will run the console php script in the matomo container).

As for how the test database is populated, it is created, setup & filled by the https://github.com/matomo-org/matomo/blob/3.x-dev/tests/PHPUnit/Framework/Fixture.php class and its descendants. (Fair warning, it sets up the entire matomo test environment, and so is a very large, hard to read class, and may make your eyes bleed.) Some test setup code is located elsewhere like in the https://github.com/matomo-org/matomo/blob/3.x-dev/tests/PHPUnit/Framework/TestingEnvironmentManipulator.php class.

@szepeviktor
Copy link
Contributor Author

Thank you.

diosmosis
diosmosis requested changes May 22, 2018
View reviewed changes
Copy link
Member

@diosmosis diosmosis left a comment

Choose a reason for hiding this comment

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

  • The minimum execution time threshold needs to be increased for system tests, as they will always be slower than unit tests. Not sure what the best value is since some will always be long running. Perhaps it would be good to make the speed trap listener optional in some way.
  • There's a conflict in the composer file that needs to be fixed.

.travis.yml
sudo: false
- php: 5.5.33
env: TEST_SUITE=UITests MYSQL_ADAPTER=PDO_MYSQL UITEST_EXTRA_OPTIONS="--run-second-half-only"
sudo: false
Copy link
Member

Choose a reason for hiding this comment

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

These changes need to be reverted before this PR can be merged.

@szepeviktor
Copy link
Contributor Author

szepeviktor commented May 22, 2018 •
edited

Thank you @diosmosis!

This PR is a rather like a debugging session.
It's goal is to find out what takes ~50 second per test.
Up to now I wasn't able to find out.

@diosmosis
Copy link
Member

@szepeviktor Gotcha, will remove this from the current milestone then, feel free to create PRs when you find some optimizations :)

@diosmosis diosmosis removed this from the 3.5.1 milestone May 24, 2018
@mattab mattab mentioned this pull request Jun 26, 2018
Closed
@mattab
Copy link
Member

mattab commented Jun 26, 2018

Thanks @szepeviktor for creating this PR. I've added a comment in the related issue here: #12691 (comment)
I'll close this PR now, but feel free to re-open it as we could in theory add the speedtrap by default in Matomo.

@mattab mattab closed this Jun 26, 2018
@szepeviktor szepeviktor deleted the phpunit-speedtrap branch June 27, 2018 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diosmosis diosmosis diosmosis requested changes

Assignees
No one assigned
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. Needs Review PRs that need a code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@szepeviktor @mattab @sgiehl @diosmosis