Closed Bug 693341 Opened 13 years ago Closed 13 years ago

Add XPConnect test coverage for dependent params

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(9 files)

We currently have none, which is bad. Dependent params, especial arrays, have lots of special things about them that need to be handled.
Attachment #568307 - Flags: review?(khuey)
Attachment #568308 - Flags: review?(khuey)
Attached patch part 4 - Test sized strings. v1 (deleted) — Splinter Review
Attachment #568309 - Flags: review?(khuey)
Attachment #568310 - Flags: review?(khuey)
Attachment #568311 - Flags: review?(khuey)
Attached patch part 7 - Test iid_is(). v1 (deleted) — Splinter Review
Attachment #568312 - Flags: review?(khuey)
Attached 7 patches. Flagging khuey for 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+
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.
Attachment #569191 - Flags: review?(khuey)
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+
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: