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.

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.

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.

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.

Sure! I'll take it.

There is still a TEST-KNOWN-FAIL related to Bug 733268

Bug 735094 requirements addressed

Sorry, the patch was corrupted and wouldn´t apply.

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 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 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...

Those lines are now within #ifdef MOZ_B2G_RIL

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.

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?

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

Third time's a charm: https://hg.mozilla.org/integration/mozilla-inbound/rev/b44a23917a99

Philipp, thanks for fixing the makefile issue! Is there any reason why you didn´t pushed all the test cases from my patch?

(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!

(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.

(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.