Last Comment Bug 693341 - Add XPConnect test coverage for dependent params
: Add XPConnect test coverage for dependent params
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Bobby Holley (busy)
:
Mentors:
: 687662 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-10 10:11 PDT by Bobby Holley (busy)
Modified: 2011-10-28 04:33 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 - Use comparators for everything in test_params.js. v1 (1.83 KB, patch)
2011-10-19 22:22 PDT, Bobby Holley (busy)
khuey: review+
Details | Diff | Review
part 2 - Test arrays of arithmetic types. v1 (9.86 KB, patch)
2011-10-19 22:22 PDT, Bobby Holley (busy)
khuey: review+
Details | Diff | Review
part 3 - Test arrays of strings. v1 (9.84 KB, patch)
2011-10-19 22:23 PDT, Bobby Holley (busy)
khuey: review+
Details | Diff | Review
part 4 - Test sized strings. v1 (11.46 KB, patch)
2011-10-19 22:23 PDT, Bobby Holley (busy)
khuey: review+
Details | Diff | Review
part 5 - Introduce a few test interfaces. v1 (9.17 KB, patch)
2011-10-19 22:24 PDT, Bobby Holley (busy)
khuey: review+
Details | Diff | Review
part 6 - Test interface arrays. v1 (8.20 KB, patch)
2011-10-19 22:24 PDT, Bobby Holley (busy)
khuey: review+
Details | Diff | Review
part 7 - Test iid_is(). v1 (8.47 KB, patch)
2011-10-19 22:24 PDT, Bobby Holley (busy)
khuey: review+
Details | Diff | Review
part 8 - Improve array and interface comparisons. v1 (4.40 KB, patch)
2011-10-24 14:43 PDT, Bobby Holley (busy)
khuey: review+
Details | Diff | Review
part 9 - Test arrays of iid_is params. v1 (8.75 KB, patch)
2011-10-24 14:44 PDT, Bobby Holley (busy)
khuey: review+
Details | Diff | Review

Description Bobby Holley (busy) 2011-10-10 10:11:19 PDT
We currently have none, which is bad. Dependent params, especial arrays, have lots of special things about them that need to be handled.
Comment 1 Bobby Holley (busy) 2011-10-19 19:23:24 PDT
*** Bug 687662 has been marked as a duplicate of this bug. ***
Comment 2 Bobby Holley (busy) 2011-10-19 22:22:31 PDT
Created attachment 568306 [details] [diff] [review]
part 1 - Use comparators for everything in test_params.js. v1
Comment 3 Bobby Holley (busy) 2011-10-19 22:22:55 PDT
Created attachment 568307 [details] [diff] [review]
part 2 - Test arrays of arithmetic types. v1
Comment 4 Bobby Holley (busy) 2011-10-19 22:23:09 PDT
Created attachment 568308 [details] [diff] [review]
part 3 - Test arrays of strings. v1
Comment 5 Bobby Holley (busy) 2011-10-19 22:23:25 PDT
Created attachment 568309 [details] [diff] [review]
part 4 - Test sized strings. v1
Comment 6 Bobby Holley (busy) 2011-10-19 22:24:04 PDT
Created attachment 568310 [details] [diff] [review]
part 5 - Introduce a few test interfaces. v1
Comment 7 Bobby Holley (busy) 2011-10-19 22:24:27 PDT
Created attachment 568311 [details] [diff] [review]
part 6 - Test interface arrays. v1
Comment 8 Bobby Holley (busy) 2011-10-19 22:24:51 PDT
Created attachment 568312 [details] [diff] [review]
part 7 - Test iid_is(). v1
Comment 9 Bobby Holley (busy) 2011-10-19 22:25:37 PDT
Attached 7 patches. Flagging khuey for review.
Comment 10 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-24 05:19:12 PDT
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 [:khuey] (khuey@mozilla.com) 2011-10-24 05:20:47 PDT
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 [:khuey] (khuey@mozilla.com) 2011-10-24 05:23:36 PDT
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 [:khuey] (khuey@mozilla.com) 2011-10-24 05:25:09 PDT
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 [:khuey] (khuey@mozilla.com) 2011-10-24 05:25:35 PDT
Any plans to test arrays with iid_is?
Comment 15 Bobby Holley (busy) 2011-10-24 13:21:55 PDT
(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.
Comment 16 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-24 13:23:25 PDT
(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.
Comment 17 Bobby Holley (busy) 2011-10-24 14:43:59 PDT
Created attachment 569191 [details] [diff] [review]
part 8 - Improve array and interface comparisons. v1
Comment 18 Bobby Holley (busy) 2011-10-24 14:44:31 PDT
Created attachment 569192 [details] [diff] [review]
part 9 - Test arrays of iid_is params. v1
Comment 19 Bobby Holley (busy) 2011-10-24 14:45:27 PDT
(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 20 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-26 12:18:07 PDT
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 (busy) 2011-10-27 07:18:44 PDT
Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=40ee397bbc6e
Comment 22 Bobby Holley (busy) 2011-10-27 12:45:43 PDT
Pushed to mozilla-inbound:

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

Note You need to log in before you can comment on or make changes to this bug.