Closed Bug 1028791 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S6 (18july)

People

(Reporter: bevis, Assigned: bevis)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
No longer blocks: 813893
Depends on: 813893
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!
Attachment #8449347 - Flags: review?(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?
Attachment #8449347 - Flags: review?(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);

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. :)
Attachment #8449347 - Flags: feedback?(vyang)
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) 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.
Attachment #8449347 - Flags: feedback?(vyang) → review+
Address the suggestions in comment 3 and comment 5:
1. Move variables into each test script.
2. Move decodeGsmDataCodingScheme() into head.js.
Attachment #8449954 - Flags: review+
Attachment #8449347 - Attachment is obsolete: true
expand TIMEOUT to 60 seconds to prevent timeout before test complete.
Attachment #8449954 - Attachment is obsolete: true
Attachment #8451529 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e2e2e7217ce2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S6 (18july)
You need to log in before you can comment on or make changes to this bug.