1115299 - B2G RIL: add test case for tethering with dun

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Jessica Jong [:jessica] (inactive)]

Since dun is not widely used and not tested on every release, we should definitely have a test case for it.

Comment 1

[Jessica Jong [:jessica] (inactive)]

Test case is almost ready, however, we can not set 'ro.tethering.dun_required' dynamically since it's an 'ro' system property; we can not set 'ro.tethering.dun_required' in makefile either cause it will affect other tethering test cases. I am thinking maybe letting dun be required if 'dun' network type gets registered on NetworkManager? Another option is to use settings api instead...

Comment 2

[Jessica Jong [:jessica] (inactive)]

Attachment: WIP.

Comment 3

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: use dun for tethering if dun network is registered.

Comment 4

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: add test case for tethering with dun.

Comment 5

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8549424 [details] [diff] [review]
Part 1: use dun for tethering if dun network is registered.

Edgar, as discussed, we will refer to ro.tethering.dun_required or dun network registration to decide whether dun tethering is needed. May I have your review on this? Thanks.

Comment 6

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8549425 [details] [diff] [review]
Part 2: add test case for tethering with dun.

Edgar, here is the test case for dun. Thanks!

Comment 7

[Edgar Chen [:edgar]]

Comment on attachment 8549425 [details] [diff] [review]
Part 2: add test case for tethering with dun.

Review of attachment 8549425 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, r=me with below comments addressed. Thank you.

::: dom/tethering/tests/marionette/head.js
@@ +15,5 @@
>  const TETHERING_SETTING_END_IP = "192.168.1.30";
>  const TETHERING_SETTING_DNS1 = "8.8.8.8";
>  const TETHERING_SETTING_DNS2 = "8.8.4.4";
>  
> +const TETHERING_NETWORK_ADDR = "192.168.1.0/24";

Nit: Add one blank line here.

@@ +459,5 @@
> +              let tokens = aLines[i].split(/\s+/);
> +              if (-1 != tokens.indexOf(TETHERING_NETWORK_ADDR)) {
> +                let lookupIndex = tokens.indexOf('lookup');
> +                if (lookupIndex < 0 || lookupIndex + 1 >= tokens.length) {
> +                  return;

Use `continue` to jump to next run?

::: dom/tethering/tests/marionette/test_wifi_tethering_dun.js
@@ +26,5 @@
> +      return gTestSuite.ensureWifiEnabled(false)
> +        .then(() => gTestSuite.setWifiTetheringEnabled(true, true))
> +        .then(() => gTestSuite.setWifiTetheringEnabled(false, true));
> +    }))
> +	  // Restore apn settings.

Nit: Indention

Comment 8

[Jessica Jong [:jessica] (inactive)]

Thanks Edgar for the review effort!

(In reply to Edgar Chen [:edgar][:echen] from comment #7)
> Comment on attachment 8549425 [details] [diff] [review]
> Part 2: add test case for tethering with dun.
> 
> Review of attachment 8549425 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, r=me with below comments addressed. Thank you.
> 
> ::: dom/tethering/tests/marionette/head.js
> @@ +15,5 @@
> >  const TETHERING_SETTING_END_IP = "192.168.1.30";
> >  const TETHERING_SETTING_DNS1 = "8.8.8.8";
> >  const TETHERING_SETTING_DNS2 = "8.8.4.4";
> >  
> > +const TETHERING_NETWORK_ADDR = "192.168.1.0/24";
> 
> Nit: Add one blank line here.

Will do.

> 
> @@ +459,5 @@
> > +              let tokens = aLines[i].split(/\s+/);
> > +              if (-1 != tokens.indexOf(TETHERING_NETWORK_ADDR)) {
> > +                let lookupIndex = tokens.indexOf('lookup');
> > +                if (lookupIndex < 0 || lookupIndex + 1 >= tokens.length) {
> > +                  return;
> 
> Use `continue` to jump to next run?

Oh, cause there should be only one rule for network address in this case and it should have a 'lookup' token, so I use 'return' here.

> 
> ::: dom/tethering/tests/marionette/test_wifi_tethering_dun.js
> @@ +26,5 @@
> > +      return gTestSuite.ensureWifiEnabled(false)
> > +        .then(() => gTestSuite.setWifiTetheringEnabled(true, true))
> > +        .then(() => gTestSuite.setWifiTetheringEnabled(false, true));
> > +    }))
> > +	  // Restore apn settings.
> 
> Nit: Indention

Will do.

Comment 9

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 1: use dun for tethering if dun network is registered. r=echen

Comment 10

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 2: add test case for tethering with dun. r=echen

Address nits in comment 7.

Comment 11

[Jessica Jong [:jessica] (inactive)]

try looks good!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28e9fb137a8b

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ab8ce2289ce2
https://hg.mozilla.org/integration/b2g-inbound/rev/6c54902867a3

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ab8ce2289ce2
https://hg.mozilla.org/mozilla-central/rev/6c54902867a3

Comment 14

[Jessica Jong [:jessica] (inactive)]

We found that in bug 1112048, a large list of apns were updated, many of them contains the "dun" apn. As a result, this patch impacts more carriers than we thought. We are concerned that using the "dun" apn when it is not really needed, will cost extra charge to users, so we are considering reverting the changes in Part 1.
I will file a separate bug for this.