Summary: | Tests that always pass when run alone, but fail in the bots | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||
Component: | Tools / Tests | Assignee: | Claudio Saavedra <csaavedra> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bugs-noreply, commit-queue, csaavedra, lforschler, mcatanzaro | ||||||
Priority: | P2 | Keywords: | Gtk, LayoutTestFailure | ||||||
Version: | WebKit Local Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2017-02-19 03:12:43 PST
At least the first one is because of bug 165133. From what I understand the intersection observer entry test enables an option that is usually not used, and we fail to check in the test runner for that. We end up reusing a webview for which the option is disabled, and hence the test fail. When the test is ran standalone all needed options are set so it passes. Also I find it weird that PlatformWebView::ViewSupportsOptions() needs to be implemented by each platform, when it's just a boolean check for equivalence in options. This way it's easy to forget to update the implementations when new options are added. I'm making a patch to remove the platform implementations and put it in one place alone. Created attachment 309895 [details]
Patch
I checked and the first test is also affected by the same, so it could be unskipped as well with this patch. Moving since it's not a GTK specific issue. Comment on attachment 309895 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309895&action=review Wow, excellent! I wonder how many other tests this is going to fix. > Tools/ChangeLog:20 > + tests individually. See bug #165133 for other example fo the same of > Tools/ChangeLog:30 > + Too many blank lines in the changelog here > Tools/WebKitTestRunner/TestOptions.h:56 > + inline bool operator==(const TestOptions& options) const Why did we have to add this? I see you copied this from the Mac port, but why is the default equivalence operator insufficient? It would be desirable to avoid defining this, so all fields of the TestOptions will are checked. Also, the function body is large enough that using inline seems inadvisable. Also, are you sure this is the right WebKit style? Indenting the braces so much seems weird. Comment on attachment 309895 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309895&action=review >> Tools/WebKitTestRunner/TestOptions.h:56 >> + inline bool operator==(const TestOptions& options) const > > Why did we have to add this? I see you copied this from the Mac port, but why is the default equivalence operator insufficient? It would be desirable to avoid defining this, so all fields of the TestOptions will are checked. > > Also, the function body is large enough that using inline seems inadvisable. > > Also, are you sure this is the right WebKit style? Indenting the braces so much seems weird. Is it really possible to compare structs? Notice that there are also other properties in the struct that I think can be changed during runtime. As far as I understand this is about the options that can only be set up during initialization (and yes, if this is truly the case it would make sense to separate them, truth be told this as it stands now is pretty confusing). Style-wise the checker didn't complain, but I don't know if the format is right or not. (In reply to Claudio Saavedra from comment #7) > Is it really possible to compare structs? Yes. If you remove the custom equivalence operator, then all the fields of the struct will be compared when checking for equivalence. With it present, then the fields you don't list will be ignored. This is usually fragile and unexpected, so it's good practice to avoid whenever possible. > Notice that there are also other > properties in the struct that I think can be changed during runtime. As far > as I understand this is about the options that can only be set up during > initialization (and yes, if this is truly the case it would make sense to > separate them, truth be told this as it stands now is pretty confusing). OK, if this is needed then I would add a new method to perform this comparison and call it explicitly where it is needed, rather than overriding the equivalence operator. > Style-wise the checker didn't complain, but I don't know if the format is > right or not. I would use one fewer level of indentation. (In reply to Michael Catanzaro from comment #8) > (In reply to Claudio Saavedra from comment #7) > > Is it really possible to compare structs? > > Yes. If you remove the custom equivalence operator, then all the fields of > the struct will be compared when checking for equivalence. Are you sure about this? I did remove the overload and the compiler fails: ../../Tools/WebKitTestRunner/PlatformWebView.h: In member function ‘bool WTR::PlatformWebView::viewSupportsOptions(const WTR::TestOptions&) const’: ../../Tools/WebKitTestRunner/PlatformWebView.h:98:83: error: no match for ‘operator==’ (operand types are ‘const WTR::TestOptions’ and ‘const WTR::TestOptions’) bool viewSupportsOptions(const TestOptions& options) const { return m_options == options; } ~~~~~~~~~~^~~~~~~~~~ All I have read on the topic indicates that you cannot compare two struct instances without defining the ==operator for it. In any case, in second thought it's not a good idea to change the semantics of the operator== to a partial equivalence, as we're doing, so I'll just make it a different method and write a good comment reminding developers to keep it up to date. Created attachment 310252 [details]
Patch
(In reply to Claudio Saavedra from comment #9) > Are you sure about this? I was pretty confident, but looks like I was wrong! > In any case, in second thought it's not a good idea to change the semantics > of the operator== to a partial equivalence, as we're doing, so I'll just > make it a different method and write a good comment reminding developers to > keep it up to date. Agreed. Comment on attachment 310252 [details] Patch Clearing flags on attachment: 310252 Committed r216931: <http://trac.webkit.org/changeset/216931> All reviewed patches have been landed. Closing bug. |