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.

Sign up for GitHub

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

Tests slow since 4.2.1 #17290

Closed
tsteur opened this issue Mar 2, 2021 · 4 comments · Fixed by #17340
Closed

Tests slow since 4.2.1 #17290

tsteur opened this issue Mar 2, 2021 · 4 comments · Fixed by #17340
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Milestone
4.3.0

Comments

@tsteur
Copy link
Member

tsteur commented Mar 2, 2021 •
edited

The tests are slow since 4.2.1 release. For example some tests in Matomo for WordPress the performance got worse from 1s per test to 20s. Also various developers reported slow tests.

The reason is https://github.com/matomo-org/matomo/pull/17225/files which is executed on every update.

In particular https://github.com/matomo-org/matomo/blob/4.2.1/plugins/Installation/ServerFilesGenerator.php#L310 is the problem.

I wonder if we could remove this call to deleteHtAccessFiles () and if we ever remove an .htaccess file that we no longer need instead rather have a specific update script for it? Maybe this way it be even better like what if deletion of htaccess works but maybe for some reasons the writing of htaccess files fails or because of a bug it is not rewritten? Also technically it means there's for a very short time no .htaccess file.

Alternatively, another way to fix it could be to check for a PIWIK_TEST_MODE constant and not execute it during tests but ideally we're always having it the same way.
@tsteur tsteur added c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Regression Indicates a feature used to work in a certain way but it no longer does even though it should. labels Mar 2, 2021
@tsteur tsteur added this to the 4.3.0 milestone Mar 2, 2021
@diosmosis
Copy link
Member

I guess we could simply remove the call and later add a system check to make sure the .htaccess files are correct or something?

@tsteur
Copy link
Member Author

tsteur commented Mar 2, 2021

fyi this one is called in a loop https://github.com/matomo-org/matomo/pull/17225/files#diff-b8b358078043274d41d6a5a6afa7ddade12b2db2d472b52b8fd5a8a513e839bfR334 and not needed actually since it's called anyway.

regarding removing it: not sure why we delete them in the first place

@diosmosis
Copy link
Member

@tsteur I assume it's in case the logic in the code is wrong and we want to make sure we have the correct .htaccess files. I can't think of another at least.

@tsteur
Copy link
Member Author

tsteur commented Mar 2, 2021

Just realise independent on a fix the globr in https://github.com/matomo-org/matomo/blob/4.2.1/plugins/Installation/ServerFilesGenerator.php#L310-L333 is not even needed since it's trying to remove the .htaccess files for these hard coded paths anyway.

I reckon too though that we should simply remove that call to deleteHtAccessFiles and instead delete files manually if they are no longer needed. We wouldn't really want to delete the htaccess files temporarily.

@sgiehl sgiehl mentioned this issue Mar 12, 2021
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Projects
None yet
Milestone
4.3.0
Development

Successfully merging a pull request may close this issue.

Do not try to remove webserver config files matomo-org/matomo
2 participants
@diosmosis @tsteur