Closed
Bug 710092
Opened 13 years ago
Closed 13 years ago
Tests for the RIL worker
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: philikon, Assigned: philikon)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review |
We should write unit tests for the RIL worker. I *think* these could be xpcshell-tests.
Assignee | ||
Comment 1•13 years ago
|
||
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•13 years ago
|
Assignee | ||
Updated•13 years ago
|
Component: General → DOM: Device Interfaces
QA Contact: general → device-interfaces
Comment 2•13 years ago
|
||
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.
Assignee | ||
Comment 3•13 years ago
|
||
(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.
Assignee | ||
Comment 4•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
This is what I got for the moment. I guess the only usefull part might be the xpcshell wrapper.
Attachment #585704 -
Flags: review?(philipp)
Assignee | ||
Comment 7•13 years ago
|
||
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)
Assignee | ||
Comment 8•13 years ago
|
||
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]
Assignee | ||
Comment 9•13 years ago
|
||
Here's an updated version that takes the recent renames into account and uses nsISubScriptLoader.
Attachment #585704 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•