1311739 - Push H/2 test improvements

Status: RESOLVED FIXED

(Core :: DOM: Push Notifications, defect)

Description

[Lina Cambridge [:lina] (ex-Mozilla)]

:hurley has a Try run where several of the Push H/2 tests time out. A few things stand out from the log at https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-b66f2859f3a291570367cffeb08f47af94cf0ce9/try-macosx64/try_yosemite_r7_test-xpcshell-bm136-tests1-macosx-build678.txt.gz:

* It looks like the MOZHTTP2_PORT env var is empty, causing the tests to try to connect to "https://localhost:/". That looks like a different bug, but I think we should fail noisily instead of retrying and eventually timing out.

* `PushDB.update` returns undefined instead of throwing if a record doesn't exist, or the new record is invalid. I can't remember why it did that, but it's broken because callers don't null-check the result. This causes us to swallow rejections and print errors like '[JavaScript Error: "TypeError: record is undefined" {file: "resource://gre/modules/PushServiceHttp2.jsm" line: 667}]'.

* We don't observe 'serverURL' in tests, so there's lots of 'JavaScript Error: "Attempt to stop observing a preference "serverURL" that's not being observed" {file: "resource://gre/modules/Preferences.jsm" line: 323}' logspam.

Comment 1

[Lina Cambridge [:lina] (ex-Mozilla)]

Attachment: Bug 1311739 - Fail push tests with an informative error if the H/2 server port is missing.

Review commit: https://reviewboard.mozilla.org/r/87220/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87220/

Comment 2

[Lina Cambridge [:lina] (ex-Mozilla)]

Attachment: Bug 1311739 - `PushDB.update` should reject instead of returning `undefined` for invalid records.

Review commit: https://reviewboard.mozilla.org/r/87222/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87222/

Comment 3

[Lina Cambridge [:lina] (ex-Mozilla)]

Attachment: Bug 1311739 - Silence `serverURL` pref warnings in push tests.

Review commit: https://reviewboard.mozilla.org/r/87224/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87224/

Comment 4

[Dragana Damjanovic [:dragana]]

Comment on attachment 8802967 [details]
Bug 1311739 - Fail push tests with an informative error if the H/2 server port is missing.

https://reviewboard.mozilla.org/r/87220/#review87034

Comment 5

[Dragana Damjanovic [:dragana]]

Comment on attachment 8802968 [details]
Bug 1311739 - `PushDB.update` should reject instead of returning `undefined` for invalid records.

https://reviewboard.mozilla.org/r/87222/#review87036

Comment 6

[Dragana Damjanovic [:dragana]]

Comment on attachment 8802969 [details]
Bug 1311739 - Silence `serverURL` pref warnings in push tests.

https://reviewboard.mozilla.org/r/87224/#review87050

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/357a4a25901a
Fail push tests with an informative error if the H/2 server port is missing. r=dragana
https://hg.mozilla.org/integration/autoland/rev/c0c3b590372c
`PushDB.update` should reject instead of returning `undefined` for invalid records. r=dragana
https://hg.mozilla.org/integration/autoland/rev/8fd3c7221c70
Silence `serverURL` pref warnings in push tests. r=dragana

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/357a4a25901a
https://hg.mozilla.org/mozilla-central/rev/c0c3b590372c
https://hg.mozilla.org/mozilla-central/rev/8fd3c7221c70