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)
1.83 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
9.86 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
9.84 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
11.46 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
9.17 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
8.20 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
8.47 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
4.40 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
8.75 KB,
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.
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 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
•