Bug 168572 - Tests that always pass when run alone, but fail in the bots
Summary: Tests that always pass when run alone, but fail in the bots
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Claudio Saavedra
URL:
Keywords: Gtk, LayoutTestFailure
Depends on:
Blocks:
 
Reported: 2017-02-19 03:12 PST by Carlos Garcia Campos
Modified: 2017-05-16 08:08 PDT (History)
5 users (show)

See Also:


Attachments
Patch (10.64 KB, patch)
2017-05-12 08:21 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (11.05 KB, patch)
2017-05-16 03:58 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2017-02-19 03:12:43 PST
intersection-observer/intersection-observer-entry-interface.html
js/dom/regress-157246.html

Both work when they are run alone.
Comment 1 Claudio Saavedra 2017-05-12 06:29:51 PDT
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.
Comment 2 Claudio Saavedra 2017-05-12 07:04:07 PDT
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.
Comment 3 Claudio Saavedra 2017-05-12 08:21:34 PDT
Created attachment 309895 [details]
Patch
Comment 4 Claudio Saavedra 2017-05-12 08:49:58 PDT
I checked and the first test is also affected by the same, so it could be unskipped as well with this patch.
Comment 5 Claudio Saavedra 2017-05-14 04:13:07 PDT
Moving since it's not a GTK specific issue.
Comment 6 Michael Catanzaro 2017-05-15 10:51:51 PDT
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 7 Claudio Saavedra 2017-05-15 11:56:22 PDT
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.
Comment 8 Michael Catanzaro 2017-05-15 14:44:56 PDT
(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.
Comment 9 Claudio Saavedra 2017-05-16 03:48:21 PDT
(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.
Comment 10 Claudio Saavedra 2017-05-16 03:58:54 PDT
Created attachment 310252 [details]
Patch
Comment 11 Michael Catanzaro 2017-05-16 07:40:25 PDT
(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 12 WebKit Commit Bot 2017-05-16 08:08:47 PDT
Comment on attachment 310252 [details]
Patch

Clearing flags on attachment: 310252

Committed r216931: <http://trac.webkit.org/changeset/216931>
Comment 13 WebKit Commit Bot 2017-05-16 08:08:49 PDT
All reviewed patches have been landed.  Closing bug.