Last Comment Bug 733265 - B2G SMS DB tests
: B2G SMS DB tests
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Fernando Jiménez Moreno [:ferjm] (PTO until August)
:
Mentors:
Depends on: 732414
Blocks: 712809
  Show dependency treegraph
 
Reported: 2012-03-05 19:09 PST by Philipp von Weitershausen [:philikon]
Modified: 2012-04-17 13:55 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP 6 (11.63 KB, patch)
2012-03-06 17:02 PST, Philipp von Weitershausen [:philikon]
no flags Details | Diff | Splinter Review
WIP v7 (29.58 KB, patch)
2012-03-12 05:01 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
WIP v7-1 (29.59 KB, patch)
2012-03-15 04:11 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
WIP v7-1 (29.59 KB, patch)
2012-03-15 09:04 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
WIP v8 (28.94 KB, patch)
2012-03-20 03:40 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
philipp: review+
Details | Diff | Splinter Review
WIP v9 (29.29 KB, patch)
2012-03-21 06:36 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
philipp: review+
Details | Diff | Splinter Review
WIP 10 (29.29 KB, patch)
2012-03-29 01:48 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
philipp: review-
Details | Diff | Splinter Review
WIP v11 (29.34 KB, patch)
2012-04-03 03:32 PDT, Fernando Jiménez Moreno [:ferjm] (PTO until August)
philipp: review+
Details | Diff | Splinter Review

Description Philipp von Weitershausen [:philikon] 2012-03-05 19:09:14 PST
The test_smsdatabaseservice.js xpcshell test is currently perma-disabled because (a) it depends on bug 732414 and (b) it should only ever run against the RIL backend of nsISmsDatabaseService which is normally not configured. We should figure out a way to run this test on tinderboxes.
Comment 1 Philipp von Weitershausen [:philikon] 2012-03-06 12:42:09 PST
Morphing this bug into addressing tests in general. There's a WIP as xpcshell tests in bug 712809, but as Mounir points out in bug 732414 comment 5, it might make more sense to do this as a mochitest.

Whatever we do, we'll do it in this bug.
Comment 2 Philipp von Weitershausen [:philikon] 2012-03-06 17:02:35 PST
Created attachment 603522 [details] [diff] [review]
WIP 6

Converted the test v5 WIP patch from bug 712809 to a chrome mochitest. You can run the test by executing

  TEST_PATH="dom/sms/tests" make mochitest-chrome

in your obj dir. The test passes but it leaks, so overall it would fail. But it's not enabled by default anyway.

To keep it from leaking, I suspect we will have to make SmsDatabaseService close its IndexedDB connection on browser shutdown, and things like that.
Comment 3 Philipp von Weitershausen [:philikon] 2012-03-06 17:45:36 PST
Fernando, do you think you could complete these tests? I'm going to tentatively assign you. Feel free to remove yourself if you don't want to work on it.
Comment 4 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-03-07 00:04:31 PST
Sure! I'll take it.
Comment 5 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-03-12 05:01:21 PDT
Created attachment 604892 [details] [diff] [review]
WIP v7

There is still a TEST-KNOWN-FAIL related to Bug 733268
Comment 6 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-03-15 04:11:02 PDT
Created attachment 606156 [details] [diff] [review]
WIP v7-1

Bug 735094 requirements addressed
Comment 7 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-03-15 09:04:08 PDT
Created attachment 606243 [details] [diff] [review]
WIP v7-1

Sorry, the patch was corrupted and wouldn´t apply.
Comment 8 Philipp von Weitershausen [:philikon] 2012-03-15 18:00:13 PDT
Comment on attachment 606243 [details] [diff] [review]
WIP v7-1

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

Nice work! A few general comments/questions:

* We can afford the few characters to make meaningful variable names. Of couse I realize `requestId` is already taken up as a parameter name, but we can at least call it `reqId` instead of `rId`.
* Can you explain the 111 factor in `let rId = Math.floor(Math.random()*111);`? Why not 100, for instance?
* You're repeating `let rId = Math.floor(Math.random()*111);` a lot. Please make a helper, e.g. newRequestId(). Also, spaces around all operators!
* Why are you wrapping everything in SimpleTest.executeSoon()?
Comment 9 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-03-20 03:40:18 PDT
Created attachment 607503 [details] [diff] [review]
WIP v8
Comment 10 Philipp von Weitershausen [:philikon] 2012-03-20 18:53:03 PDT
Comment on attachment 607503 [details] [diff] [review]
WIP v8

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

Gave it a quick review. I'm busy trying to finish up some stuff before I take a few days off, so I didn't look at the individual tests too closely. Some of them I already looked at fairly closely before. So I think we can commit this with the nits below addressed, and then handle any potential issues in follow-ups.

::: dom/sms/tests/test_smsdatabaseservice.xul
@@ +170,5 @@
> +  });
> +}
> +
> +function newRandomId() {
> +   return Math.floor(Math.random());

Math.random() returns a random number between 0 and 1. I think you want some sort of multiplier here, e.g. LONG_MAX = 2147483647 (because requestId is a long in the IDL).

Also, nit: indent by two spaces.

@@ +244,5 @@
> + * nsISmsDatabaseService.getMessage
> + */
> +add_test(function test_getMessage_success() {
> +  info("test_getMessage_success");
> +  let aRequestId = newRandomId();

The 'a' prefix has a special meaning in Mozilla code. It stands for "argument". So this is a bit confusing that the thing that *isn't* an argument is called `aRequestId` and the thing that actually is an argument (to one of the SmsRequestManager methods) is called 'requestId'. Just do s/aRequestId/fakeRequestId/, that seems the easiest ;)
Comment 11 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-03-21 06:36:18 PDT
Created attachment 607931 [details] [diff] [review]
WIP v9
Comment 12 Philipp von Weitershausen [:philikon] 2012-03-28 20:47:56 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c63b21280c2
Comment 13 Philipp von Weitershausen [:philikon] 2012-03-28 21:24:04 PDT
Comment on attachment 607931 [details] [diff] [review]
WIP v9

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

::: dom/sms/tests/Makefile.in
@@ +62,5 @@
>  libs:: $(_TEST_FILES)
>  	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/tests/$(relativesrcdir)
>  
> +libs:: $(_CHROME_TEST_FILES)
> +	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/chrome/$(relativesrcdir)

This was backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/fa695fa4f819 because these lines arent' ifdef'ed with MOZ_B2G_RIL...
Comment 14 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-03-29 01:48:34 PDT
Created attachment 610484 [details] [diff] [review]
WIP 10

Those lines are now within #ifdef MOZ_B2G_RIL
Comment 15 Philipp von Weitershausen [:philikon] 2012-03-29 14:48:03 PDT
Comment on attachment 610484 [details] [diff] [review]
WIP 10

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

::: dom/sms/tests/Makefile.in
@@ +62,5 @@
>  	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/tests/$(relativesrcdir)
>  
> +libs:: $(_CHROME_TEST_FILES)
> +	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/chrome/$(relativesrcdir)
> +endif

Now you're ifdefing the regular _TEST_FILES installation which means those thests aren't getting installed anymore! Also, we should allow future chrome tests that don't depend on MOZ_B2G_RIL to be installed, so I suggest this:

  _CHROME_TEST_FILES =
  ifdef MOZ_B2G_RIL
  _CHROME_TEST_FILES += test_smsdatabaseservice.xul
  endif

and then the two libs rules without any ifdefing... I think that should work.
Comment 16 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-04-03 03:32:58 PDT
Created attachment 611764 [details] [diff] [review]
WIP v11
Comment 17 Philipp von Weitershausen [:philikon] 2012-04-05 14:17:33 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/3780b09790a0
Comment 18 Philipp von Weitershausen [:philikon] 2012-04-05 15:01:01 PDT
And backed out again: https://hg.mozilla.org/integration/mozilla-inbound/rev/c6fe3d26bece

Guess my suggested fix didn't quite work... Fernando, did you even test this?
Comment 19 Wan-Teh Chang 2012-04-05 16:12:43 PDT
I ran into this build failure.  I suggest adding ifneq
protection around the makefile rule:

ifneq (,$(_CHROME_TEST_FILES))
libs:: $(_CHROME_TEST_FILES)
        $(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/chrome/$(relativesrcdir)
endif
Comment 20 Philipp von Weitershausen [:philikon] 2012-04-10 22:59:49 PDT
Third time's a charm: https://hg.mozilla.org/integration/mozilla-inbound/rev/b44a23917a99
Comment 21 Philipp von Weitershausen [:philikon] 2012-04-12 16:23:37 PDT
https://hg.mozilla.org/mozilla-central/rev/b44a23917a99
Comment 22 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-04-15 13:06:09 PDT
Philipp, thanks for fixing the makefile issue!
Is there any reason why you didn´t pushed all the test cases from my patch?
Comment 23 Philipp von Weitershausen [:philikon] 2012-04-15 13:27:39 PDT
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #22)
> Philipp, thanks for fixing the makefile issue!
> Is there any reason why you didn´t pushed all the test cases from my patch?

Did I miss any? Maybe I pushed an older version of your patch by accident? If so, I'm sorry! Please feel free to file a follow up and attach a patch for the missing test cases. Thanks!
Comment 24 Fernando Jiménez Moreno [:ferjm] (PTO until August) 2012-04-15 13:34:47 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #23)
> Did I miss any? Maybe I pushed an older version of your patch by accident?
> If so, I'm sorry! Please feel free to file a follow up and attach a patch
> for the missing test cases. Thanks!
No problem! :)
I´ll be uploading new WIP patches to bug 733320 followed by new sms db tests, so maybe I can just add the missing tests there instead of filling a new bug, if that is ok for you.
Comment 25 Philipp von Weitershausen [:philikon] 2012-04-17 13:06:59 PDT
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #24)
> (In reply to Philipp von Weitershausen [:philikon] from comment #23)
> > Did I miss any? Maybe I pushed an older version of your patch by accident?
> > If so, I'm sorry! Please feel free to file a follow up and attach a patch
> > for the missing test cases. Thanks!
> No problem! :)
> I´ll be uploading new WIP patches to bug 733320 followed by new sms db
> tests, so maybe I can just add the missing tests there instead of filling a
> new bug, if that is ok for you.

Sure ok. Perhaps in a separate patch, though.

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