Closed Bug 1115299 Opened 7 years ago Closed 7 years ago

B2G RIL: add test case for tethering with dun

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S5 (6feb)

People

(Reporter: jessica, Assigned: jessica)

References

Details

Attachments

(2 files, 3 obsolete files)

Since dun is not widely used and not tested on every release, we should definitely have a test case for it.
Assignee: nobody → jjong
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...
Attached patch WIP. (obsolete) — Splinter Review
Attachment #8548670 - Attachment is obsolete: true
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.
Attachment #8549424 - Flags: review?(echen)
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!
Attachment #8549425 - Flags: review?(echen)
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Attachment #8549424 - Flags: review?(echen) → review+
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
Attachment #8549425 - Flags: review?(echen) → review+
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.
Address nits in comment 7.
Attachment #8549425 - Attachment is obsolete: true
Attachment #8551711 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ab8ce2289ce2
https://hg.mozilla.org/mozilla-central/rev/6c54902867a3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S5 (6feb)
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.
Blocks: 1159132
You need to log in before you can comment on or make changes to this bug.