Last Comment Bug 710092 - Tests for the RIL worker
: Tests for the RIL worker
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: ---
Assigned To: Philipp von Weitershausen [:philikon]
:
Mentors:
Depends on: 723244
Blocks: b2g-ril
  Show dependency treegraph
 
Reported: 2011-12-12 17:31 PST by Philipp von Weitershausen [:philikon]
Modified: 2012-02-23 06:47 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
XPCShell tests wrapper and some tests for the RIL worker (10.11 KB, patch)
2012-01-04 04:11 PST, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
xpcshell skeleton (6.54 KB, patch)
2012-01-27 11:22 PST, Philipp von Weitershausen [:philikon]
no flags Details | Diff | Splinter Review

Description Philipp von Weitershausen [:philikon] 2011-12-12 17:31:06 PST
We should write unit tests for the RIL worker. I *think* these could be xpcshell-tests.
Comment 1 Philipp von Weitershausen [:philikon] 2011-12-12 17:57:57 PST
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.
Comment 2 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-01-03 09:12:50 PST
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.
Comment 3 Philipp von Weitershausen [:philikon] 2012-01-03 15:49:54 PST
(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.
Comment 4 Philipp von Weitershausen [:philikon] 2012-01-03 15:55:35 PST
(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.
Comment 5 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-01-04 04:08:21 PST
(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.
Comment 6 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-01-04 04:11:19 PST
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.
Comment 7 Philipp von Weitershausen [:philikon] 2012-01-04 18:55:30 PST
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...
Comment 8 Philipp von Weitershausen [:philikon] 2012-01-04 18:56:58 PST
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.
Comment 9 Philipp von Weitershausen [:philikon] 2012-01-27 11:22:36 PST
Created attachment 592188 [details] [diff] [review]
xpcshell skeleton

Here's an updated version that takes the recent renames into account and uses nsISubScriptLoader.
Comment 10 Philipp von Weitershausen [:philikon] 2012-02-23 06:47:07 PST
Bug 723244 landed stuff based on my skeleton patch.

Note You need to log in before you can comment on or make changes to this bug.