Closed Bug 813596 Opened 7 years ago Closed 7 years ago

B2G RIL: add Marionette test cases for Cell Broadcast

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: vicamo, Assigned: vicamo)

References

Details

Attachments

(3 files, 4 obsolete files)

Emulator parts[1][2] of Cell Broadcast functions is still not reviewed. Let's land core functions first.

[1]: https://github.com/mozilla-b2g/platform_external_qemu/pull/5
[2]: https://github.com/mozilla-b2g/platform_hardware_ril/pull/3
1) Rebase
2) Comment out event constructor checking because it crashes content process.
3) Correct message class values
4) Correct test cases for GSM CBS messages with UCS2 encoded body string.
Attachment #684421 - Attachment is obsolete: true
Attached file Github Pull Request for external/qemu (obsolete) —
Hi Marshall, feel free to cancel this review if you're busy on other stuff, I'll find somebody else.
Attachment #685011 - Flags: review?(marshall)
Attached file Github Pull Request for hardware/ril (obsolete) —
Attachment #685012 - Flags: review?(marshall)
Assignee: nobody → vyang
Attachment #684950 - Flags: review?(htsai)
Comment on attachment 684950 [details] [diff] [review]
Part 1: add marionette test cases

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

Nice!

::: dom/cellbroadcast/tests/marionette/test_cellbroadcast_etws.js
@@ +31,5 @@
> +function sendCellBroadcastMessage(pdu, callback) {
> +  pendingEmulatorCmdCount++;
> +
> +  let cmd = "cbs pdu " + pdu;
> +  //log("Sending emulator command: " + cmd);

Please remove it if you think it's unnecessary.

@@ +95,5 @@
> +    cbs.removeEventListener("received", onreceived);
> +
> +    // TODO: this check causes b2g crash
> +    //ok(event instanceof MozCellBroadcastEvent,
> +    //  "event is instanceof " + event.constructor);

TODO comment should be followed by a filed bug number, right?

I think we could add 'ok(event, "event is valid")' here.

@@ +191,5 @@
> +
> +  // Here we use a simple ETWS message for test.
> +  let pdu = buildHexStr(0, 12); // 6 octets
> +  doTestHelper(pdu, testReceiving_ETWS_WarningType, function (message) {
> +    // Cell Broadcast messages do not contains a timestamp field (however, ETWS

nit: s/contains/contain

@@ +192,5 @@
> +  // Here we use a simple ETWS message for test.
> +  let pdu = buildHexStr(0, 12); // 6 octets
> +  doTestHelper(pdu, testReceiving_ETWS_WarningType, function (message) {
> +    // Cell Broadcast messages do not contains a timestamp field (however, ETWS
> +    // does). We only check the timestamp doesn't go to far (60 seconds) here.

Seems grammatical error here? Did you mean 'We only check the timestamp that doesn't go too far...'? Anyway, please check this sentence again. :)

::: dom/cellbroadcast/tests/marionette/test_cellbroadcast_gsm.js
@@ +28,5 @@
> +
> +const CB_MESSAGE_SIZE_GSM  = 88;
> +
> +const CB_GSM_MESSAGEID_ETWS_BEGIN = 0x1100;
> +const CB_GSM_MESSAGEID_ETWS_END   = 0x1108;

In your final patch, you already changed the value to '0x1107.' Please make it consistent.

@@ +86,5 @@
> +function sendCellBroadcastMessage(pdu, callback) {
> +  pendingEmulatorCmdCount++;
> +
> +  let cmd = "cbs pdu " + pdu;
> +  //log("Sending emulator command: " + cmd);

ditto.

@@ +149,5 @@
> +  cbs.addEventListener("received", function onreceived(event) {
> +    cbs.removeEventListener("received", onreceived);
> +
> +    // TODO: this check causes b2g crash
> +    //ok(event instanceof MozCellBroadcastEvent,

ditto.

@@ +150,5 @@
> +    cbs.removeEventListener("received", onreceived);
> +
> +    // TODO: this check causes b2g crash
> +    //ok(event instanceof MozCellBroadcastEvent,
> +    //  "event is instanceof " + event.constructor);

How about adding 'ok(event, "event is valid");' ?

@@ +356,5 @@
> +  log("Test receiving GSM Cell Broadcast - Timestamp");
> +
> +  let pdu = buildHexStr(0, CB_MESSAGE_SIZE_GSM * 2);
> +  doTestHelper(pdu, testReceiving_GSM_WarningType, function (message) {
> +    // Cell Broadcast messages do not contains a timestamp field (however, ETWS

nit: s/contains/contain

@@ +357,5 @@
> +
> +  let pdu = buildHexStr(0, CB_MESSAGE_SIZE_GSM * 2);
> +  doTestHelper(pdu, testReceiving_GSM_WarningType, function (message) {
> +    // Cell Broadcast messages do not contains a timestamp field (however, ETWS
> +    // does). We only check the timestamp doesn't go to far (60 seconds) here.

ditto.

@@ +368,5 @@
> +function testReceiving_GSM_WarningType() {
> +  log("Test receiving GSM Cell Broadcast - Warning Type");
> +
> +  let messageIdsToTest = [];
> +  for (let i = CB_GSM_MESSAGEID_ETWS_BEGIN; i < CB_GSM_MESSAGEID_ETWS_END; i++) {

Please make the condition consistent with your implementation patch.
Attachment #684950 - Flags: review?(htsai) → review+
Attachment #685011 - Flags: review?(marshall) → review?(htsai)
Attachment #685012 - Flags: review?(marshall) → review?(htsai)
Hey Vicamo,

I saw you closed the pull requests in github. What's the status now? Where's the stuff I should help review?
Moved to new pull request
Attachment #685011 - Attachment is obsolete: true
Attachment #685011 - Flags: review?(htsai)
Attachment #696929 - Flags: review?(htsai)
Moved to new pull request
Attachment #685012 - Attachment is obsolete: true
Attachment #685012 - Flags: review?(htsai)
Attachment #696930 - Flags: review?(htsai)
Comment on attachment 696929 [details] [review]
Github Pull Request for external/qemu

Looks great!
Attachment #696929 - Flags: review?(htsai) → review+
Comment on attachment 696929 [details] [review]
Github Pull Request for external/qemu

Looks great, only a nit there. Please see my comment in github :)
Comment on attachment 696930 [details] [review]
Github Pull Request for hardware/ril

Looks good to me!
Attachment #696930 - Flags: review?(htsai) → review+
Depends on: 828186
Address review comments.
Attachment #684950 - Attachment is obsolete: true
Attachment #701690 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/830177b40e86
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.