693341 - Add XPConnect test coverage for dependent params

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

We currently have none, which is bad. Dependent params, especial arrays, have lots of special things about them that need to be handled.

Comment 2

[Bobby Holley (:bholley)]

Attachment: part 1 - Use comparators for everything in test_params.js. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: part 2 - Test arrays of arithmetic types. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: part 3 - Test arrays of strings. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: part 4 - Test sized strings. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: part 5 - Introduce a few test interfaces. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: part 6 - Test interface arrays. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: part 7 - Test iid_is(). v1

Comment 9

[Bobby Holley (:bholley)]

Attached 7 patches. Flagging khuey for review.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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?)

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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?

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Any plans to test arrays with iid_is?

Comment 15

[Bobby Holley (:bholley)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Bobby Holley (:bholley) from comment #15)
> (In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 17

[Bobby Holley (:bholley)]

Attachment: part 8 - Improve array and interface comparisons. v1

Comment 18

[Bobby Holley (:bholley)]

Attachment: part 9 - Test arrays of iid_is params. v1

Comment 19

[Bobby Holley (:bholley)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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:
  }
};

Comment 21

[Bobby Holley (:bholley)]

Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=40ee397bbc6e

Comment 22

[Bobby Holley (:bholley)]

Pushed to mozilla-inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/c5863a0662de
(and ancestors)

Comment 23

[Ed Morley [:emorley]]

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