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
Conversation
can we print out the response body (if any) when these methods fail? I'm curious what the error behind it is. |
27f6e8f
to
facc69e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
71e643c
to
60ccc48
Compare
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 |
60ccc48
to
9d339e3
Compare
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. |
3def861
to
3a92c20
Compare
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) { |
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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?
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.