The default bug view has changed. See this FAQ.

Add XPConnect test coverage for dependent params

RESOLVED FIXED in mozilla10

Status

()

Core
XPConnect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments)

We currently have none, which is bad. Dependent params, especial arrays, have lots of special things about them that need to be handled.
Duplicate of this bug: 687662
Created attachment 568306 [details] [diff] [review]
part 1 - Use comparators for everything in test_params.js. v1
Attachment #568306 - Flags: review?(khuey)
Created attachment 568307 [details] [diff] [review]
part 2 - Test arrays of arithmetic types. v1
Attachment #568307 - Flags: review?(khuey)
Created attachment 568308 [details] [diff] [review]
part 3 - Test arrays of strings. v1
Attachment #568308 - Flags: review?(khuey)
Created attachment 568309 [details] [diff] [review]
part 4 - Test sized strings. v1
Attachment #568309 - Flags: review?(khuey)
Created attachment 568310 [details] [diff] [review]
part 5 - Introduce a few test interfaces. v1
Attachment #568310 - Flags: review?(khuey)
Created attachment 568311 [details] [diff] [review]
part 6 - Test interface arrays. v1
Attachment #568311 - Flags: review?(khuey)
Created attachment 568312 [details] [diff] [review]
part 7 - Test iid_is(). v1
Attachment #568312 - Flags: review?(khuey)
Attached 7 patches. Flagging khuey for review.
Attachment #568306 - Flags: review?(khuey) → review+
Comment on attachment 568307 [details] [diff] [review]
part 2 - Test arrays of arithmetic types. v1

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

I should note that I'm mostly skimming these.

::: js/xpconnect/tests/idl/xpctest_params.idl
@@ +77,5 @@
> +                                       inout unsigned long bLength, [array, size_is(bLength)] inout short b,
> +                                       out unsigned long rvLength, [retval, array, size_is(rvLength)] out short rv);
> +  void                  testLongLongArray(in unsigned long aLength, [array, size_is(aLength)] in long long a,
> +                                          inout unsigned long bLength, [array, size_is(bLength)] inout long long b,
> +                                          out unsigned long rvLength, [retval, array, size_is(rvLength)] out long long rv);

Can we test both orders (in other words, have another copy with the array before the size parameter?)
Attachment #568307 - Flags: review?(khuey) → review+
Comment on attachment 568308 [details] [diff] [review]
part 3 - Test arrays of strings. v1

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

::: js/xpconnect/tests/unit/test_params.js
@@ +129,5 @@
>    doIsTest("testLongLongArray", [-10000000000], 1, [1, 3, 1234511234551], 3, arrayComparator);
> +  doIsTest("testStringArray", ["mary", "hat", "hey", "lid", "tell", "lam"], 6,
> +                              ["ids", "fleas", "woes", "wide", "has", "know", "!"], 7, arrayComparator);
> +  doIsTest("testWstringArray", ["沒有語言", "的偉大嗎?]"], 2,
> +                               ["we", "are", "being", "sooo", "international", "right", "now"], 7, arrayComparator);

yay unicode
Attachment #568308 - Flags: review?(khuey) → review+
Attachment #568309 - Flags: review?(khuey) → review+
Attachment #568310 - Flags: review?(khuey) → review+
Comment on attachment 568311 [details] [diff] [review]
part 6 - Test interface arrays. v1

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

::: js/xpconnect/tests/idl/xpctest_params.idl
@@ +42,5 @@
>   * covered by the intersection of return values and inout).
>   */
>  
>  #include "nsISupports.idl"
> +#include "xpctest_interfaces.idl"

Just forward declare the interfaces here.

::: js/xpconnect/tests/unit/test_params.js
@@ +135,5 @@
>                                ["ids", "fleas", "woes", "wide", "has", "know", "!"], 7, arrayComparator);
>    doIsTest("testWstringArray", ["沒有語言", "的偉大嗎?]"], 2,
>                                 ["we", "are", "being", "sooo", "international", "right", "now"], 7, arrayComparator);
> +  doIsTest("testInterfaceArray", [makeA(), makeA()], 2,
> +                                 [makeA(), makeA(), makeA(), makeA(), makeA(), makeA()], 6, arrayComparator);

Why aren't we using any makeB()s here?
Attachment #568311 - Flags: review?(khuey) → review+
Comment on attachment 568312 [details] [diff] [review]
part 7 - Test iid_is(). v1

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

::: js/xpconnect/tests/components/native/xpctest_params.cpp
@@ +320,5 @@
> +    // by XPConnect for the duration of the call. If we snatch it away from b
> +    // and leave no trace, XPConnect won't Release it. Since we also need to
> +    // return an already-AddRef'd pointer in rv, we don't need to do anything
> +    // special here.
> +    *rv = *b;

This is actually pretty scary stuff.
Attachment #568312 - Flags: review?(khuey) → review+
Any plans to test arrays with iid_is?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10)
> Can we test both orders (in other words, have another copy with the array
> before the size parameter?)

I'd rather not add the extra goop that this would entail (since we save a lot of code by making assumptions about the ordering of arguments). But I'm open to being convinced if you think it's important.

> Why aren't we using any makeB()s here?

Because we're not using iid_is here, so the interfaces are statically typed, and the 'in' stuff needs to come out in the 'inout' position.

> Any plans to test arrays with iid_is?

That seems like a good thing to test, though it might require more goop. Let me investigate.
(In reply to Bobby Holley (:bholley) from comment #15)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10)
> > Can we test both orders (in other words, have another copy with the array
> > before the size parameter?)
> 
> I'd rather not add the extra goop that this would entail (since we save a
> lot of code by making assumptions about the ordering of arguments). But I'm
> open to being convinced if you think it's important.

I don't think it's that important, was just wondering.

> > Why aren't we using any makeB()s here?
> 
> Because we're not using iid_is here, so the interfaces are statically typed,
> and the 'in' stuff needs to come out in the 'inout' position.

Ok.

> > Any plans to test arrays with iid_is?
> 
> That seems like a good thing to test, though it might require more goop. Let
> me investigate.

This is probably worth it.  Nothing in m-c uses it but there's some calendar stuff that depends on it ...

I learned that the hard way.
Created attachment 569191 [details] [diff] [review]
part 8 - Improve array and interface comparisons. v1
Attachment #569191 - Flags: review?(khuey)
Created attachment 569192 [details] [diff] [review]
part 9 - Test arrays of iid_is params. v1
Attachment #569192 - Flags: review?(khuey)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #16)

> This is probably worth it.  Nothing in m-c uses it but there's some calendar
> stuff that depends on it ...
> 
> I learned that the hard way.

Fair enough. Patches attached, flagged for review.
Comment on attachment 569191 [details] [diff] [review]
part 8 - Improve array and interface comparisons. v1

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

::: js/xpconnect/tests/unit/test_params.js
@@ +65,5 @@
> +      for (var i = 0; i < a.length; ++i)
> +        if (!innerComparator(a[i], b[i]))
> +          return false;
> +      return true;
> +    } };

Nit:
  }
};
Attachment #569191 - Flags: review?(khuey) → review+
Attachment #569192 - Flags: review?(khuey) → review+
Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=40ee397bbc6e
Pushed to mozilla-inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/c5863a0662de
(and ancestors)
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/a1781299f27d
https://hg.mozilla.org/mozilla-central/rev/c1ec0a4d6048
https://hg.mozilla.org/mozilla-central/rev/d6bdd9875c40
https://hg.mozilla.org/mozilla-central/rev/5839ab91c873
https://hg.mozilla.org/mozilla-central/rev/20c5952de8d6
https://hg.mozilla.org/mozilla-central/rev/b4e6d15b836e
https://hg.mozilla.org/mozilla-central/rev/5aa96243e15d
https://hg.mozilla.org/mozilla-central/rev/cb81dcf21c85
https://hg.mozilla.org/mozilla-central/rev/c5863a0662de
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.