Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: philikon, Assigned: philikon)

Tracking

Trunk
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

We should write unit tests for the RIL worker. I *think* these could be xpcshell-tests.
With some guidance, this could be a good first bug for somebody to tackle. Just need to write some JS, really. I can help with the xpcshell wrapper and any other questions.
Whiteboard: [good first bug][lang=js][mentor=philikon]
(Assignee)

Updated

6 years ago
Blocks: 710489
No longer blocks: 699235
(Assignee)

Updated

6 years ago
Component: General → DOM: Device Interfaces
QA Contact: general → device-interfaces
I am currently working on this bug at https://github.com/ferjm/mozilla-central. I´ve already created the xpcshell wrapper and added some tests based on b2g-js-ril test.js.
(In reply to Fernando Jiménez [:ferjm] from comment #2)
> I am currently working on this bug at
> https://github.com/ferjm/mozilla-central.

Please let's stop working in GitHub for mozilla-central related patches. The way patches are coordinated for mozilla-central is, for better or for worse, Bugzilla. Please attach patches to this bug, even if they're work in progress.

Also, please assign a bug to yourself *before* you start work on something. This helps coordination. I already started on this, too :(

> I´ve already created the xpcshell
> wrapper and added some tests based on b2g-js-ril test.js.

Please attach patches to this bug so we can coordinate.
(In reply to Fernando Jiménez [:ferjm] from comment #2)
> I´ve already created the xpcshell
> wrapper and added some tests based on b2g-js-ril test.js.

FWIW, I'm not super happy with those tests in the first place. I think it'd almost be better to start from scratch.
(In reply to Philipp von Weitershausen [:philikon] from comment #3)
> Please let's stop working in GitHub for mozilla-central related patches. The
> way patches are coordinated for mozilla-central is, for better or for worse,
> Bugzilla. Please attach patches to this bug, even if they're work in
> progress.
> 
> Also, please assign a bug to yourself *before* you start work on something.
> This helps coordination. I already started on this, too :(
Ok. I still need to get used to the way you work. Also, I wasn´t sure about how to assign the bug to myself. Sorry :\

> Please attach patches to this bug so we can coordinate.
Sure. I´ll attach a patch right now.
Created attachment 585704 [details] [diff] [review]
XPCShell tests wrapper and some tests for the RIL worker

This is what I got for the moment. I guess the only usefull part might be the xpcshell wrapper.
Attachment #585704 - Flags: review?(philipp)
Comment on attachment 585704 [details] [diff] [review]
XPCShell tests wrapper and some tests for the RIL worker

Great start!

>diff --git a/dom/telephony/Makefile.in b/dom/telephony/Makefile.in
>--- a/dom/telephony/Makefile.in
>+++ b/dom/telephony/Makefile.in
>@@ -35,16 +35,18 @@
> #
> # ***** END LICENSE BLOCK *****
> 
> DEPTH            = ../..
> topsrcdir        = @top_srcdir@
> srcdir           = @srcdir@
> VPATH            = @srcdir@
> 
>+relativesrcdir = dom/telephony
>+

This shouldn't be necessary at this level, I think?

>diff --git a/dom/telephony/tests/Makefile.in b/dom/telephony/tests/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/dom/telephony/tests/Makefile.in
>@@ -0,0 +1,19 @@

Needs a license header.

>+libs::
>+	$(INSTALL) $(topsrcdir)/dom/telephony/ril_worker.js \
>+          $(DEPTH)/_tests/xpcshell/$(relativesrcdir)/unit/
>+	$(INSTALL) $(topsrcdir)/dom/telephony/ril_consts.js \
>+          $(DEPTH)/_tests/xpcshell/$(relativesrcdir)/unit/

I would like to avoid this. We can use mozIJSSubscriptLoader to load these files via their resource: URIs. That way we can even separate the namespaces, which is also nice.

>diff --git a/dom/telephony/tests/unit/test_ril_worker_buf.js b/dom/telephony/tests/unit/test_ril_worker_buf.js
>new file mode 100644
>--- /dev/null
>+++ b/dom/telephony/tests/unit/test_ril_worker_buf.js

This is basically an adaption of the hacked up test.js file we had in GitHub. I don't really like those tests. They're not very unit-y and lack explanation.

>diff --git a/dom/telephony/tests/unit/test_ril_worker_gsm_pdu_helper.js b/dom/telephony/tests/unit/test_ril_worker_gsm_pdu_helper.js
>new file mode 100644
>--- /dev/null
>+++ b/dom/telephony/tests/unit/test_ril_worker_gsm_pdu_helper.js
...
>+add_test(function test_0() {
...
>+add_test(function test_1() {
...
>+add_test(function test_2() {
...
>+add_test(function test_3() {
...

This is not very descriptive. What are we testing here in these individual tests? Somebody who refactors the code later will want to understand why their tests are breaking...
Attachment #585704 - Flags: review?(philipp)
I think it will be easier if I merge ferjm's initial patch with some of my work and get this started. We can build out the individual tests in parallel later in a separate bug.
Assignee: nobody → philipp
Whiteboard: [good first bug][lang=js][mentor=philikon]
Created attachment 592188 [details] [diff] [review]
xpcshell skeleton

Here's an updated version that takes the recent renames into account and uses nsISubScriptLoader.
Attachment #585704 - Attachment is obsolete: true
Bug 723244 landed stuff based on my skeleton patch.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Depends on: 723244
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.