Closed Bug 1057689 Opened 5 years ago Closed 4 years ago

per-app-offline: test app-offline notifications

Categories

(Core :: Networking, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jduell.mcbugs, Assigned: valentin.gosu)

References

Details

(Whiteboard: [necko-would-take])

Attachments

(1 file, 2 obsolete files)

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.
Changes NeckoParent to send the ipdl message if !UsingNeckoIPCSecurity() because xpcshell tests don't have TabParents.
Attachment #8482297 - Flags: review?(jduell.mcbugs)
Whiteboard: [necko-would-take]
rebased. test passes locally
Attachment #8728698 - Flags: review?(jduell.mcbugs)
Attachment #8482297 - Attachment is obsolete: true
Attachment #8482297 - Flags: review?(jduell.mcbugs)
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-
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)
Attachment #8728698 - Attachment is obsolete: true
Attachment #8728698 - Flags: review?(jduell.mcbugs)
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1686be775813bb687d3e015313f85957ee28af59
Bug 1057689 - Add xpcshell test checking correct notifications and app-offline state r=jduell
https://hg.mozilla.org/mozilla-central/rev/1686be775813
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.