Closed
Bug 693341
Opened 13 years ago
Closed 13 years ago
Add XPConnect test coverage for dependent params
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(9 files)
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
We currently have none, which is bad. Dependent params, especial arrays, have lots of special things about them that need to be handled.
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #568306 -
Flags: review?(khuey)
Assignee | ||
Comment 3•13 years ago
|
||
Attachment #568307 -
Flags: review?(khuey)
Assignee | ||
Comment 4•13 years ago
|
||
Attachment #568308 -
Flags: review?(khuey)
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #568309 -
Flags: review?(khuey)
Assignee | ||
Comment 6•13 years ago
|
||
Attachment #568310 -
Flags: review?(khuey)
Assignee | ||
Comment 7•13 years ago
|
||
Attachment #568311 -
Flags: review?(khuey)
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #568312 -
Flags: review?(khuey)
Assignee | ||
Comment 9•13 years ago
|
||
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?
Assignee | ||
Comment 15•13 years ago
|
||
(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.
Assignee | ||
Comment 17•13 years ago
|
||
Attachment #569191 -
Flags: review?(khuey)
Assignee | ||
Comment 18•13 years ago
|
||
Attachment #569192 -
Flags: review?(khuey)
Assignee | ||
Comment 19•13 years ago
|
||
(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+
Assignee | ||
Comment 21•13 years ago
|
||
Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=40ee397bbc6e
Assignee | ||
Comment 22•13 years ago
|
||
Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c5863a0662de
(and ancestors)
Target Milestone: --- → mozilla10
Comment 23•13 years ago
|
||
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
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•