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

Try to avoid ui test failures due to CSS loading failures #16122

Merged
merged 11 commits into from Jun 29, 2020

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Jun 25, 2020

There are a lot builds failing because the CSS seems not to be loaded. In most cases the test logs even don't have any notes, that loading the CSS failed. The reason for that is simple. We always return a HTTP 200 on the error page (as long as the message does not include database keywords). So if merging the CSS throws an exception the error page is served instead (with HTTP Status 200).

To avoid too many test failures when the CSS isn't loaded correctly I've build some code that automatically tries to add another style tag to the page to load the CSS again. In most cases that helps and the tests are running, but sometimes even that reload fails.

Nevertheless I think that might help to have a more stable build.

@sgiehl sgiehl 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. Needs Review PRs that need a code review labels Jun 25, 2020
@sgiehl sgiehl added this to the 4.0.0 milestone Jun 25, 2020
@diosmosis
Copy link
Member

can we print out the response body (if any) when these methods fail? I'm curious what the error behind it is.

@sgiehl sgiehl force-pushed the uitestscssfailure branch 2 times, most recently from 27f6e8f to facc69e Compare June 26, 2020 07:18
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.

LGTM

@sgiehl sgiehl force-pushed the uitestscssfailure branch 3 times, most recently from 71e643c to 60ccc48 Compare June 26, 2020 12:03
@tsteur
Copy link
Member

tsteur commented Jun 28, 2020

Be great to merge @sgiehl

We can otherwise also further look into it towards the end of year if needed and maybe the logged error messages help us find over time the root cause and find a solution

@sgiehl
Copy link
Member Author

sgiehl commented Jun 29, 2020

Was about to merge it when I realized, that even reloading the CSS didn't work in most cases. Logging the reposes and it header lead me to the right direction I guess. The response was always an empty response with status 200. Actually for both requests, even with an additional parameter. So I've tried with some debugging code, and it seems for some reason compressing the css fails and so an empty compressed file is stored. That empty file is automatically served with the next request.

@sgiehl
Copy link
Member Author

sgiehl commented Jun 29, 2020

I've now added an exception to be thrown when compressing a file does not work. That might actually solve the travis problem, as it should prevent storing an empty file and a direct regeneration of that file, as merging the file will be triggered a second time here

@diosmosis @tsteur could someone have a quick look at that again and merge afterwards?

@@ -286,6 +286,10 @@ private static function compressFile($fileToCompress, $compressedFilePath, $comp
$data = gzencode($data, 9);
}

if (false === $data) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if we maybe should also check if the response looks valid, like checking if it has at least 50 chars or something like that 🤔

Copy link
Member

Choose a reason for hiding this comment

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

If it's gzipped, that would be kind of hard, right?

@diosmosis diosmosis merged commit 8280b85 into 4.x-dev Jun 29, 2020
@diosmosis diosmosis deleted the uitestscssfailure branch June 29, 2020 14:37
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. Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants