Closed
Bug 1057689
Opened 10 years ago
Closed 8 years ago
per-app-offline: test app-offline notifications
Categories
(Core :: Networking, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jduell.mcbugs, Assigned: valentin)
References
Details
(Whiteboard: [necko-would-take])
Attachments
(1 file, 2 obsolete files)
7.58 KB,
patch
|
jduell.mcbugs
:
review+
|
Details | Diff | Splinter Review |
Make sure they are sent only once, in both parent and child, and only when there's an actual change of online/offline state. Should be easy to add to existing xpcshell test.
Assignee | ||
Comment 1•10 years ago
|
||
Changes NeckoParent to send the ipdl message if !UsingNeckoIPCSecurity() because xpcshell tests don't have TabParents.
Attachment #8482297 -
Flags: review?(jduell.mcbugs)
Updated•8 years ago
|
Whiteboard: [necko-would-take]
Assignee | ||
Comment 2•8 years ago
|
||
rebased. test passes locally
Attachment #8728698 -
Flags: review?(jduell.mcbugs)
Assignee | ||
Updated•8 years ago
|
Attachment #8482297 -
Attachment is obsolete: true
Attachment #8482297 -
Flags: review?(jduell.mcbugs)
Reporter | ||
Comment 3•8 years ago
|
||
Comment on attachment 8728698 [details] [diff] [review] Add xpcshell test checking correct notifications and app-offline state Review of attachment 8728698 [details] [diff] [review]: ----------------------------------------------------------------- The overall test logic looks fine but I'm confused about how how you're using do_test_pending/do_test_finished, particularly in the child process. ::: netwerk/test/unit_ipc/child_app_offline_notifications.js @@ +20,5 @@ > +// Add observer for the app-offline notification > +function run_test() { > + do_test_pending(); > + Services.obs.addObserver(observer, "network:app-offline-status-changed", false); > + do_test_finished(); I don't understand the do_test_finished() here. According to https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests a balanced set of do_test_{pending/finished} calls like this ought to result in the test exiting once run_test() exits, but you clearly want it to stay alive. Am I missing something? @@ +28,5 @@ > +function check_status(appId, status) > +{ > + do_test_pending(); > + do_check_eq(is_app_offline(appId), status == Ci.nsIAppOfflineInfo.OFFLINE); > + do_test_finished(); I don't see why the pending/finished is needed here (and in most other places in this file). Shouldn't you just have a single pending() call in run_test, and then a finished() call at the end of the test? That's the way most of our unit tests are written, but perhaps there's something about the parent-child interactions you have going on that I'm not understanding.
Attachment #8728698 -
Flags: feedback-
Assignee | ||
Comment 4•8 years ago
|
||
So, the child actually doesn't need any calls to pending/finished in this case, as we don't do any async operations. The parent doesn't need to call do_test_pending, as run_test_in_child does that for us.
Attachment #8732940 -
Flags: review?(jduell.mcbugs)
Assignee | ||
Updated•8 years ago
|
Attachment #8728698 -
Attachment is obsolete: true
Attachment #8728698 -
Flags: review?(jduell.mcbugs)
Reporter | ||
Comment 5•8 years ago
|
||
Comment on attachment 8732940 [details] [diff] [review] Add xpcshell test checking correct notifications and app-offline state Review of attachment 8732940 [details] [diff] [review]: ----------------------------------------------------------------- Ah, I didn't know that about run_test_in_child(). ::: netwerk/test/unit_ipc/test_app_offline_notifications.js @@ +97,5 @@ > +function finished() { > + dump("parent: RUNNING: finished\n"); > + Services.obs.removeObserver(observer, "network:app-offline-status-changed"); > + do_check_eq(events_observed_no, 2); > + do_test_finished(); Add comment above do_test_finished: // This do_test_finished() balances out the do_test_pending that was called by run_test_in_child()
Attachment #8732940 -
Flags: review?(jduell.mcbugs) → review+
Assignee | ||
Comment 6•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1686be775813bb687d3e015313f85957ee28af59 Bug 1057689 - Add xpcshell test checking correct notifications and app-offline state r=jduell
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1686be775813
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•