Bug 168572 – Tests that always pass when run alone, but fail in the bots

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Carlos Garcia Campos]

intersection-observer/intersection-observer-entry-interface.html
js/dom/regress-157246.html

Both work when they are run alone.

[Claudio Saavedra]

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.

[Claudio Saavedra]

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.

[Claudio Saavedra]

Created attachment 309895 [details]
Patch

[Claudio Saavedra]

I checked and the first test is also affected by the same, so it could be unskipped as well with this patch.

[Claudio Saavedra]

Moving since it's not a GTK specific issue.

[Michael Catanzaro]

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.

[Claudio Saavedra]

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.

[Michael Catanzaro]

(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.

[Claudio Saavedra]

(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.

[Claudio Saavedra]

Created attachment 310252 [details]
Patch

[Michael Catanzaro]

(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.

[WebKit Commit Bot]

Comment on attachment 310252 [details]
Patch

Clearing flags on attachment: 310252

Committed r216931: <http://trac.webkit.org/changeset/216931>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.