1028791 - [B2G][CBS] Re-write Marionette Test Cases of CellBroadcast with Promise.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

CBS-related head.js with Promise has been provided in bug 921326.
Fire this bug to rewrite these test cases with Promise after bug 921326 is landed.

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch v1: Re-write Marionette Test Cases of CellBroadcast with Promise. r=vyang.

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8449347 [details] [diff] [review]
Patch v1: Re-write Marionette Test Cases of CellBroadcast with Promise. r=vyang.

1. Move Constants & Utilities to head.js.
2. Rewrite test cases with Promise.

Hi Vicamo,
May I have your review for this patch? Thanks!

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8449347 [details] [diff] [review]
Patch v1: Re-write Marionette Test Cases of CellBroadcast with Promise. r=vyang.

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

::: dom/cellbroadcast/tests/marionette/head.js
@@ +70,5 @@
> +                + "\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000"
> +                + "\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000"
> +                + "\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000"
> +                + "\u0000"; // 41 unicode chars.
> +const BODY_UCS2_IND = BODY_UCS2.substr(1);

"BODY_7BITS" looks like a variable that should only exist in per test script.  Please try to be more specific to keep them in head.js.

::: dom/cellbroadcast/tests/marionette/test_cellbroadcast_gsm.js
@@ -242,5 @@
> -  repeat(do_test, messageIdsToTest, testReceiving_GSM_Language_and_Body);
> -}
> -
> -// Copied from GsmPDUHelper.readCbDataCodingScheme
> -function decodeDataCodingScheme(dcs) {

I really like the way it was. Any strong reason to move it into another function?

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8449347 [details] [diff] [review]
Patch v1: Re-write Marionette Test Cases of CellBroadcast with Promise. r=vyang.

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

::: dom/cellbroadcast/tests/marionette/head.js
@@ +70,5 @@
> +                + "\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000"
> +                + "\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000"
> +                + "\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000"
> +                + "\u0000"; // 41 unicode chars.
> +const BODY_UCS2_IND = BODY_UCS2.substr(1);

My original purpose is to put common-used CB related constants and variables into head.js.
"BODY_7BITS" is used in both test_cellbroadcast_multi_sim.js and test_cellbroadcast_gsm.js.

It seems more clear to put CB-related constants in head.js and move these variables into the each test script instead.

I'll move these variables like BODY_7BITS, BODY_7BITS_IND, BODY_UCS2, BODY_UCS2_IND to each test script in next update.

::: dom/cellbroadcast/tests/marionette/test_cellbroadcast_gsm.js
@@ -242,5 @@
> -  repeat(do_test, messageIdsToTest, testReceiving_GSM_Language_and_Body);
> -}
> -
> -// Copied from GsmPDUHelper.readCbDataCodingScheme
> -function decodeDataCodingScheme(dcs) {

My reasons are
1. To make the 1st-level functions be the test cases to be executed to improve the readability.
2. To move common-used utilities into head.js.
3. To convert functions only used by specific test case into inner functions of that test case.

decodeDataCodingScheme() is only used in testReceiving_GSM_Language_and_Body(), so I convert it as an inner function.

If decodeDataCodingScheme() is more likely to be a utility, I can move it into head.js instead. :)

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Bevis Tseng [:bevistseng] ([e-mail]) from comment #4)
> Comment on attachment 8449347 [details] [diff] [review]
> I'll move these variables like BODY_7BITS, BODY_7BITS_IND, BODY_UCS2,
> BODY_UCS2_IND to each test script in next update.

I'm not going to stop you from pulling common variables into head.js, but their names have to be more clear and specific.  BODY_7BIT doesn't reveal much about itself. DUMMY_GSM_7BITS_BODY, DUMMY_GSM_7BITS_BODY_WITH_LANG_INDICATOR may do a better job when being promoted as shared common constants.

> ::: dom/cellbroadcast/tests/marionette/test_cellbroadcast_gsm.js
> @@ -242,5 @@
> > -  repeat(do_test, messageIdsToTest, testReceiving_GSM_Language_and_Body);
> > -}
> > -
> > -// Copied from GsmPDUHelper.readCbDataCodingScheme
> > -function decodeDataCodingScheme(dcs) {
> 
> My reasons are
> 1. To make the 1st-level functions be the test cases to be executed to
> improve the readability.
> 2. To move common-used utilities into head.js.
> 3. To convert functions only used by specific test case into inner functions
> of that test case.
> 
> decodeDataCodingScheme() is only used in
> testReceiving_GSM_Language_and_Body(), so I convert it as an inner function.
> 
> If decodeDataCodingScheme() is more likely to be a utility, I can move it
> into head.js instead. :)

Okay, I'm fine with any way you take. Please rename it to decodeGsmDataCodingScheme if you choose to move it into head.js.

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch v2: Re-write Marionette Test Cases of CellBroadcast with Promise. r=vyang.

Address the suggestions in comment 3 and comment 5:
1. Move variables into each test script.
2. Move decodeGsmDataCodingScheme() into head.js.

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch v4: Re-write Marionette Test Cases of CellBroadcast with Promise. r=vyang.

expand TIMEOUT to 60 seconds to prevent timeout before test complete.

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

try server is green:
https://tbpl.mozilla.org/?tree=Try&rev=a44e74be94c4

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/e2e2e7217ce2

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e2e2e7217ce2