Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: philikon, Assigned: ferjm)

Tracking

Trunk
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

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.
Summary: B2G SMS: Enable test_smsdatabaseservice.js → B2G SMS DB tests
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.
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.
Assignee: nobody → ferjmoreno
(Assignee)

Comment 4

5 years ago
Sure! I'll take it.
(Assignee)

Comment 5

5 years ago
Created attachment 604892 [details] [diff] [review]
WIP v7

There is still a TEST-KNOWN-FAIL related to Bug 733268
Attachment #603522 - Attachment is obsolete: true
Attachment #604892 - Flags: review?(philipp)
(Reporter)

Updated

5 years ago
Depends on: 735536
(Reporter)

Updated

5 years ago
No longer depends on: 735536
(Assignee)

Comment 6

5 years ago
Created attachment 606156 [details] [diff] [review]
WIP v7-1

Bug 735094 requirements addressed
Attachment #604892 - Attachment is obsolete: true
Attachment #606156 - Flags: review?(philipp)
Attachment #604892 - Flags: review?(philipp)
(Assignee)

Comment 7

5 years ago
Created attachment 606243 [details] [diff] [review]
WIP v7-1

Sorry, the patch was corrupted and wouldn´t apply.
Attachment #606156 - Attachment is obsolete: true
Attachment #606243 - Flags: review?(philipp)
Attachment #606156 - Flags: review?(philipp)
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()?
Attachment #606243 - Flags: review?(philipp)
(Assignee)

Comment 9

5 years ago
Created attachment 607503 [details] [diff] [review]
WIP v8
Attachment #606243 - Attachment is obsolete: true
Attachment #607503 - Flags: review?(philipp)
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 ;)
Attachment #607503 - Flags: review?(philipp) → review+
(Assignee)

Comment 11

5 years ago
Created attachment 607931 [details] [diff] [review]
WIP v9
Attachment #607503 - Attachment is obsolete: true
Attachment #607931 - Flags: review?(philipp)
(Reporter)

Updated

5 years ago
Attachment #607931 - Flags: review?(philipp) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c63b21280c2
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...
(Assignee)

Comment 14

5 years ago
Created attachment 610484 [details] [diff] [review]
WIP 10

Those lines are now within #ifdef MOZ_B2G_RIL
Attachment #607931 - Attachment is obsolete: true
Attachment #610484 - Flags: review?(philipp)
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.
Attachment #610484 - Flags: review?(philipp) → review-
(Assignee)

Comment 16

5 years ago
Created attachment 611764 [details] [diff] [review]
WIP v11
Attachment #610484 - Attachment is obsolete: true
Attachment #611764 - Flags: review?(philipp)
(Reporter)

Updated

5 years ago
Attachment #611764 - Flags: review?(philipp) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/3780b09790a0
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

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/b44a23917a99
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
(Assignee)

Comment 22

5 years ago
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!
(Assignee)

Comment 24

5 years ago
(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.
You need to log in before you can comment on or make changes to this bug.