733265 - B2G SMS DB tests

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

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

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

Attachment: 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]]

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

Sure! I'll take it.

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP v7

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

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP v7-1

Bug 735094 requirements addressed

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP v7-1

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

Comment 8

[Philipp von Weitershausen [:philikon]]

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

Attachment: WIP v8

Comment 10

[Philipp von Weitershausen [:philikon]]

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

Attachment: WIP v9

Comment 12

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c63b21280c2

Comment 13

[Philipp von Weitershausen [:philikon]]

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

Attachment: WIP 10

Those lines are now within #ifdef MOZ_B2G_RIL

Comment 15

[Philipp von Weitershausen [:philikon]]

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

Attachment: WIP v11

Comment 17

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3780b09790a0

Comment 18

[Philipp von Weitershausen [:philikon]]

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]

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

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

Comment 21

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/b44a23917a99

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

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

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

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

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