B2G RIL: add test case for tethering with dun

RESOLVED FIXED in 2.2 S5 (6feb)

Status

Firefox OS
RIL
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jessica, Assigned: jessica)

Tracking

unspecified
2.2 S5 (6feb)
ARM
Gonk (Firefox OS)
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

4 years ago
Since dun is not widely used and not tested on every release, we should definitely have a test case for it.
(Assignee)

Updated

4 years ago
Assignee: nobody → jjong
(Assignee)

Comment 1

4 years ago
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...
(Assignee)

Comment 3

4 years ago
Created attachment 8549424 [details] [diff] [review]
Part 1: use dun for tethering if dun network is registered.
(Assignee)

Comment 4

4 years ago
Created attachment 8549425 [details] [diff] [review]
Part 2: add test case for tethering with dun.
Attachment #8548670 - Attachment is obsolete: true
(Assignee)

Comment 5

4 years ago
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)
(Assignee)

Comment 6

4 years ago
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)

Updated

4 years ago
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM

Updated

4 years ago
Attachment #8549424 - Flags: review?(echen) → review+

Comment 7

4 years ago
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+
(Assignee)

Comment 8

4 years ago
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.
(Assignee)

Comment 9

4 years ago
Created attachment 8551709 [details] [diff] [review]
[final] Part 1: use dun for tethering if dun network is registered. r=echen
Attachment #8549424 - Attachment is obsolete: true
Attachment #8551709 - Flags: review+
(Assignee)

Comment 10

4 years ago
Created attachment 8551711 [details] [diff] [review]
[final] Part 2: add test case for tethering with dun. r=echen

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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S5 (6feb)
(Assignee)

Comment 14

3 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 1159132
You need to log in before you can comment on or make changes to this bug.