Bug 175212 – Add test coverage for sendBeacon() keepalive flag

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

[Chris Dumez]

Add test coverage for sendBeacon() keepalive flag.

[Chris Dumez]

Created attachment 317294 [details]
Patch

[youenn fablet]

Comment on attachment 317294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317294&action=review

> LayoutTests/http/wpt/beacon/keepalive-after-navigation.html:25
> +    }), 1000);

1000 may be too long in most cases and too short in others.
Why not trying to fetch every 50 ms until you have the correct content type and fail after a longer time like 5 seconds?

> LayoutTests/http/wpt/beacon/keepalive-after-navigation.html:35
> +    return pollResult(test, id) .then(result => {

s/ .//

[Chris Dumez]

Created attachment 317299 [details]
Patch

[youenn fablet]

Forgot to say, why wpt and not imported?
Are you planning to upstream these?

[youenn fablet]

Comment on attachment 317299 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317299&action=review

> LayoutTests/http/wpt/beacon/keepalive-after-navigation.html:15
> +function pollResult(test, id) {

Maybe I am missing something but this is not really polling in the sense that you try until you either succeed or time out?
If there is no need for polling and the fetch request will always happen after the beacon one, we just need to increase the timeout to 5000, right?

[Chris Dumez]

This function is copied from other Beacon tests in WPT.

(In reply to youenn fablet from comment #5)
> Comment on attachment 317299 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317299&action=review
> 
> > LayoutTests/http/wpt/beacon/keepalive-after-navigation.html:15
> > +function pollResult(test, id) {
> 
> Maybe I am missing something but this is not really polling in the sense
> that you try until you either succeed or time out?

We keep trying at regular intervals. I believe this is polling.

> If there is no need for polling and the fetch request will always happen
> after the beacon one, we just need to increase the timeout to 5000, right?

I do not understand this part. A beacon may be sent after a while, so we need to keep polling until it comes.

[youenn fablet]

Comment on attachment 317299 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317299&action=review

>>> LayoutTests/http/wpt/beacon/keepalive-after-navigation.html:15
>>> +function pollResult(test, id) {
>> 
>> Maybe I am missing something but this is not really polling in the sense that you try until you either succeed or time out?
>> If there is no need for polling and the fetch request will always happen after the beacon one, we just need to increase the timeout to 5000, right?
> 
> We keep trying at regular intervals. I believe this is polling.

It seems there is only one fetch being ever called with this code.
Is step_timeout expected to call fetch every 50 ms?

[Chris Dumez]

(In reply to youenn fablet from comment #4)
> Forgot to say, why wpt and not imported?
> Are you planning to upstream these?

I will try to upstream it, yes.

[Chris Dumez]

(In reply to youenn fablet from comment #7)
> Comment on attachment 317299 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317299&action=review
> 
> >>> LayoutTests/http/wpt/beacon/keepalive-after-navigation.html:15
> >>> +function pollResult(test, id) {
> >> 
> >> Maybe I am missing something but this is not really polling in the sense that you try until you either succeed or time out?
> >> If there is no need for polling and the fetch request will always happen after the beacon one, we just need to increase the timeout to 5000, right?
> > 
> > We keep trying at regular intervals. I believe this is polling.
> 
> It seems there is only one fetch being ever called with this code.
> Is step_timeout expected to call fetch every 50 ms?

Yes, we need to call fetch() at regular interval to ask the server if it received the beacon.

[Chris Dumez]

Comment on attachment 317299 [details]
Patch

Clearing flags on attachment: 317299

Committed r220304: <http://trac.webkit.org/changeset/220304>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33734986>

[youenn fablet]

> Yes, we need to call fetch() at regular interval to ask the server if it
> received the beacon.

Looking at step_timeout, it does not use setInterval and is calling setTimeout once.
It seems to me there will be only one fetch being called, fetch being done after waiting for 50 ms.

Also, even if fetch is called several times, it would seem better to wait for the completion of the first fetch before triggering a new fetch.

[Chris Dumez]

(In reply to youenn fablet from comment #13)
> > Yes, we need to call fetch() at regular interval to ask the server if it
> > received the beacon.
> 
> Looking at step_timeout, it does not use setInterval and is calling
> setTimeout once.
> It seems to me there will be only one fetch being called, fetch being done
> after waiting for 50 ms.
> 
> Also, even if fetch is called several times, it would seem better to wait
> for the completion of the first fetch before triggering a new fetch.

oh, the switch to 50ms is likely going to cause flakiness then. I'll follow-up :(