Closed
Bug 684327
Opened 13 years ago
Closed 13 years ago
Create unified XPConnect argument conversion testing framework
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(10 files, 9 obsolete files)
(deleted),
text/plain
|
Details | |
(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'll want this before landing bug 683802.
We currently have 3 testing interfaces in xpconnect/tests/idl (xpctest_in.idl, xpctest_out.idl, and xpctest_inout.idl), and C++ implementations of these in xpconnect/tests/components.
There are a few things that need to happen here:
1 - Verify that these c++ implementations are actually being tested (I may be dense, but I can't find anywhere where we actually call these test methods from JS.)
2 - Add jsval (and any other missing types) to the tests
3 - Create JS implementations of these interfaces
4 - Test both implementation with a JS harness, and the JS implementation with a C++ harness.
Assignee | ||
Comment 1•13 years ago
|
||
This is ready to go. Flagging khuey for review, since a lot of this stuff is build-oriented and deals with xpidl.
The patches are here: https://github.com/bholley/mozilla-central/commits/xpc_test
The list of patches to be reviewed is in the attached file.
Kyle - I can easily push them to bugzilla if you prefer, just let me know.
Attachment #561050 -
Flags: review?(khuey)
(In reply to Bobby Holley (:bholley) from comment #1)
> Kyle - I can easily push them to bugzilla if you prefer, just let me know.
Please do. I'm not going to review patches outside of Bugzilla.
Assignee | ||
Comment 3•13 years ago
|
||
Thanks to Mook for the initial patch!
Attachment #561060 -
Flags: review?(khuey)
Assignee | ||
Comment 4•13 years ago
|
||
Attachment #561061 -
Flags: review?(khuey)
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #561062 -
Flags: review?(khuey)
Assignee | ||
Comment 6•13 years ago
|
||
Attachment #561063 -
Flags: review?(khuey)
Assignee | ||
Comment 7•13 years ago
|
||
Attachment #561064 -
Flags: review?(khuey)
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #561065 -
Flags: review?(khuey)
Assignee | ||
Comment 9•13 years ago
|
||
This serves as test coverage for bug 683802, which the WebAPI team needs landed before the branch.
Attachment #561066 -
Flags: review?(khuey)
Assignee | ||
Updated•13 years ago
|
Attachment #561050 -
Flags: review?(khuey)
Assignee | ||
Comment 10•13 years ago
|
||
So I just debugged some try failures, and found this epic method implementation:
http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/tests/components/xpctest_attributes.cpp#186
Given the quality I'm seeing here with this test component I'm reviving, I think it might be better just to kill it off entirely. The only file from this crap I use going forward is xpctest_attributes.cpp (other files are added later).
I'll do this tomorrow. Kyle, in the mean time go ahead and skip reviewing the changes to xpctest_foo.cpp (where foo != attributes) to make them build again. I'll introduce a patch to kill them off in the morning.
Assignee | ||
Updated•13 years ago
|
Attachment #561060 -
Attachment is obsolete: true
Attachment #561060 -
Flags: review?(khuey)
Assignee | ||
Updated•13 years ago
|
Attachment #561061 -
Attachment is obsolete: true
Attachment #561061 -
Flags: review?(khuey)
Assignee | ||
Updated•13 years ago
|
Attachment #561062 -
Attachment is obsolete: true
Attachment #561062 -
Flags: review?(khuey)
Assignee | ||
Updated•13 years ago
|
Attachment #561063 -
Attachment is obsolete: true
Attachment #561063 -
Flags: review?(khuey)
Assignee | ||
Updated•13 years ago
|
Attachment #561064 -
Attachment is obsolete: true
Attachment #561064 -
Flags: review?(khuey)
Assignee | ||
Updated•13 years ago
|
Attachment #561065 -
Attachment is obsolete: true
Attachment #561065 -
Flags: review?(khuey)
Assignee | ||
Updated•13 years ago
|
Attachment #561066 -
Attachment is obsolete: true
Attachment #561066 -
Flags: review?(khuey)
Assignee | ||
Comment 11•13 years ago
|
||
Attachment #561663 -
Flags: review?(khuey)
Assignee | ||
Comment 12•13 years ago
|
||
Attachment #561665 -
Flags: review?(khuey)
Assignee | ||
Comment 13•13 years ago
|
||
This patch is best reviewed while listening to http://www.youtube.com/watch?v=MK6TXMsvgQg
NB - The code here would make a great "find the bugs in this code" interview screen question.
Attachment #561666 -
Flags: review?(khuey)
Assignee | ||
Updated•13 years ago
|
Attachment #561666 -
Attachment description: part 2 - Misc fixes to xpctest_attributes. v1 → part 3 - Misc fixes to xpctest_attributes. v1
Assignee | ||
Comment 14•13 years ago
|
||
Attachment #561667 -
Flags: review?(khuey)
Assignee | ||
Comment 15•13 years ago
|
||
Attachment #561668 -
Flags: review?(khuey)
Assignee | ||
Comment 16•13 years ago
|
||
Attachment #561669 -
Flags: review?(khuey)
Assignee | ||
Comment 17•13 years ago
|
||
Attachment #561670 -
Flags: review?(khuey)
Assignee | ||
Comment 18•13 years ago
|
||
Attachment #561671 -
Flags: review?(khuey)
Assignee | ||
Comment 19•13 years ago
|
||
Fix some trivial bitrot in the TestXPC.cpp removal.
Attachment #561663 -
Attachment is obsolete: true
Attachment #561663 -
Flags: review?(khuey)
Comment on attachment 561672 [details] [diff] [review]
part 1 - Remove TestXPC.cpp, which currently printfs that it's failing various things and then segfaults. v2
Woo negative diffs.
Attachment #561672 -
Flags: review+
Comment on attachment 561665 [details] [diff] [review]
part 2 - Start building a small subset of the xpconnect test component again, kill the rest. v2
Review of attachment 561665 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/xpconnect/tests/components/Makefile.in
@@ +70,5 @@
> +
> +
> +DEFINES += -DLIBRARY_FILENAME="$(SHARED_LIBRARY)"
> +
> +unittestlocation = js/src/xpconnect/tests/unit
Per IRC convo this should be js/src/xpconnect/tests/components.
::: js/src/xpconnect/tests/idl/Makefile.in
@@ +45,5 @@
>
> +MODULE = xpctest
> +
> +# Work around bug 688356 :-(
> +libs:: export
This should not be necessary. Bug 688356 only affects you if you're making directly in that directory, and that shouldn't be happening here (outside of people hacking the test code, of course).
Attachment #561665 -
Flags: review?(khuey) → review+
Comment on attachment 561666 [details] [diff] [review]
part 3 - Misc fixes to xpctest_attributes. v1
Review of attachment 561666 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/xpconnect/tests/components/xpctest_attributes.cpp
@@ +176,5 @@
> return NS_OK;
> }
> NS_IMETHODIMP xpcTestObjectReadWrite :: SetBooleanProperty(PRBool aBooleanProperty) {
> + NS_ENSURE_TRUE(aBooleanProperty == PR_TRUE || aBooleanProperty == PR_FALSE,
> + NS_ERROR_INVALID_ARG);
weird indentation
::: js/src/xpconnect/tests/idl/xpctest_attributes.idl
@@ +55,3 @@
> };
>
> +[scriptable, uuid(492609a7-2582-436b-b0ef-92e29bb9e143)]
this is unnecessary, but it doesn't hurt anything
Attachment #561666 -
Flags: review?(khuey) → review+
Comment on attachment 561667 [details] [diff] [review]
part 4 - Move the C++ implementation of the test component into its own subdirectory. v2
Review of attachment 561667 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/xpconnect/tests/components/Makefile.in
@@ +78,3 @@
>
> libs:: $(MANIFEST_FILE)
> $(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) $(ACDEFINES) $(XULPPFLAGS) $< > $(testxpcobjdir)/$(unittestlocation)/$(<F)
Installing the two pieces to different places seems a little silly, but if it makes future patches easier I won't object.
::: js/src/xpconnect/tests/components/native/xpctest_native.manifest
@@ +1,2 @@
> +#filter substitution
> +binary-component components/native/@LIBRARY_FILENAME@
hmm, if you drop the manifest in the right place with respect to the binary you shouldn't need the components/native/ bit here, no?
Attachment #561667 -
Flags: review?(khuey) → review+
Comment on attachment 561668 [details] [diff] [review]
part 5 - Introduce js-implemented test component. v2
Review of attachment 561668 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/xpconnect/tests/components/js/Makefile.in
@@ +1,1 @@
> +DEPTH = ../../../../../..
Needs a license header.
@@ +12,5 @@
> +NO_DIST_INSTALL = 1
> +
> +JS_FILES = \
> + xpctest_attributes.js \
> + $(NULL)
No tabs please. We have a change to do this right here.
::: js/src/xpconnect/tests/components/js/xpctest_attributes.js
@@ +1,1 @@
> +Components.utils.import("resource:///modules/XPCOMUtils.jsm");
Needs a license header.
Attachment #561668 -
Flags: review?(khuey) → review+
Comment on attachment 561669 [details] [diff] [review]
part 6 - Add an xpcshell test that exercises both the native and js components. v1
Review of attachment 561669 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/xpconnect/tests/unit/test_readwriteattributes.js
@@ +4,5 @@
> +function run_test() {
> +
> + // Load the component manifests.
> + Components.manager.autoRegister(do_get_file('xpctest_native.manifest'));
> + Components.manager.autoRegister(do_get_file('xpctest_js.manifest'));
I suppose putting the manifests separate from the components does help here.
@@ +16,5 @@
> +function test_component(contractid) {
> +
> + // Instantiate the object.
> + var o = Components.classes[contractid].createInstance()
> + .QueryInterface(gInterface);
Components.classes[contractid].createInstance(gInterface)?
Attachment #561669 -
Flags: review?(khuey) → review+
Comment on attachment 561670 [details] [diff] [review]
part 7 - Generalize test_readwriteattributes.js into test_attributes.js. v1
Review of attachment 561670 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/xpconnect/tests/unit/test_readwriteattributes.js
@@ +18,4 @@
>
> // Instantiate the object.
> + var o = Cc[contractid].createInstance()
> + .QueryInterface(Ci["nsIXPCTestObjectReadWrite"]);
Same comment as last time.
@@ +62,5 @@
> +function test_component_readonly(contractid) {
> +
> + // Instantiate the object.
> + var o = Cc[contractid].createInstance()
> + .QueryInterface(Ci["nsIXPCTestObjectReadOnly"]);
And again.
@@ +72,5 @@
> + do_check_eq(2147483647, o.longReadOnly);
> + do_check_true(5.25 < o.floatReadOnly && 5.75 > o.floatReadOnly);
> + do_check_eq("X", o.charReadOnly);
> +
> + // TODO - Once bug 686979 is fixed, make sure we throw a TypeError when
Wow, that's pretty nasty :-/.
xpcshell has todo stuff now, you should use that here.
Attachment #561670 -
Flags: review?(khuey) → review+
Comment on attachment 561671 [details] [diff] [review]
part 8 - Test parameter passing. v2
Review of attachment 561671 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/xpconnect/tests/components/js/xpctest_params.js
@@ +1,1 @@
> +Components.utils.import("resource:///modules/XPCOMUtils.jsm");
Needs a license header.
@@ +2,5 @@
> +
> +function TestParams() {
> +}
> +
> +/* For once I'm happy that JS is weakly typed. */
That makes one of us.
::: js/src/xpconnect/tests/components/native/Makefile.in
@@ +58,1 @@
> $(NULL)
I should have said this earlier, but kill the tabs here too please.
@@ +61,5 @@
>
> MANIFEST_FILE = xpctest_native.manifest
>
> EXTRA_DSO_LDOPTS += \
> $(XPCOM_GLUE_LDOPTS) \
And down here.
::: js/src/xpconnect/tests/components/native/xpctest_params.cpp
@@ +34,5 @@
> +#define STRING_METHOD_IMPL { \
> + _retval.Assign(b); \
> + b.Assign(a); \
> + return NS_OK; \
> +}
You could macro away a lot more of this if you wanted.
e.g.
#define IMPL_GENERIC(_name, _type) \
NS_IMETHODIMP nsXPCTestParams::Test##_name(_type a, _type *b NS_INOUTPARAM, _type *_retval NS_OUTPARAM) \
{ \
*_retval = *b; \
*b = a; \
return NS_OK; \
}
@@ +106,5 @@
> +/* string testString (in string a, inout string b); */
> +NS_IMETHODIMP nsXPCTestParams::TestString(const char * a, char * *b NS_INOUTPARAM, char * *_retval NS_OUTPARAM)
> +{
> + nsCAutoString aprime(a);
> + nsCAutoString bprime(*b);
nsDependentCString?
@@ +122,5 @@
> +/* wstring testWstring (in wstring a, inout wstring b); */
> +NS_IMETHODIMP nsXPCTestParams::TestWstring(const PRUnichar * a, PRUnichar * *b NS_INOUTPARAM, PRUnichar * *_retval NS_OUTPARAM)
> +{
> + nsAutoString aprime(a);
> + nsAutoString bprime(*b);
nsDependentString?
@@ +162,5 @@
> + return NS_OK;
> +}
> +
> +nsresult
> +xpctest::ConstructXPCTestParams(nsISupports *aOuter, REFNSIID aIID, void **aResult)
Isn't there a macro to implement these for you?
::: js/src/xpconnect/tests/idl/Makefile.in
@@ +54,1 @@
> $(NULL)
and more tabs. die tabs die.
::: js/src/xpconnect/tests/idl/xpctest_params.idl
@@ +1,1 @@
> +/**
Needs a license header.
::: js/src/xpconnect/tests/unit/test_params.js
@@ +15,5 @@
> +function test_component(contractid) {
> +
> + // Instantiate the object.
> + var o = Cc[contractid].createInstance()
> + .QueryInterface(Ci["nsIXPCTestParams"]);
same comments as earlier
@@ +68,5 @@
> + doTestWorkaround("testAUTF8String", "We deliver 〠!");
> + doTestWorkaround("testACString", "Just a regular C string.");
> +
> + // TODO: Uncomment when bug 604196 is fixed.
> + // doTest("testJsval", {aprop: 12, bprop: "str"}, 4.22);
todo ..
Attachment #561671 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 28•13 years ago
|
||
> xpcshell has todo stuff now, you should use that here.
It turns out that WebIDL says it shouldn't throw, but bent and jonas disagree, so they're filing a WebIDL bug (see bug 688017). While that's sorted out I'm just going to remove that comment.
Assignee | ||
Comment 29•13 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #27)
> You could macro away a lot more of this if you wanted.
I just used the template generated by xpidl, so I didn't bother.
Assignee | ||
Comment 30•13 years ago
|
||
> > + // TODO: Uncomment when bug 604196 is fixed.
> > + // doTest("testJsval", {aprop: 12, bprop: "str"}, 4.22);
>
> todo ..
This will be fixed by bug 683802, which will land at the same time.
Assignee | ||
Comment 31•13 years ago
|
||
I've addressed all of khuey's review comments. The only one that might warrant re-review is my use of the factory macros (at kyle's suggestion), which led to some re-organization in the above patch.
Kyle, feel free to give it as much or as little attention as you like, as long as it's quick. ;-)
Attachment #561666 -
Attachment is obsolete: true
Attachment #561871 -
Flags: review?(khuey)
Comment on attachment 561871 [details] [diff] [review]
part 3 - Misc fixes to xpctest_attributes. v2
Review of attachment 561871 [details] [diff] [review]:
-----------------------------------------------------------------
Looks sane enough.
Attachment #561871 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 33•13 years ago
|
||
This was causing the tests to fail on tinderbox due to the way they're packaged. Flagging khuey for review.
Attachment #562101 -
Flags: review?(khuey)
Attachment #562101 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 34•13 years ago
|
||
Landed on mozilla-inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/9eb6dc0ea6b2 (and ancestors)
This passed try: https://tbpl.mozilla.org/?tree=Try&rev=82561a8c8594
The version of these patches that was pushed is available on github: https://github.com/bholley/mozilla-central/commits/xpcwn_refactor
Flags: in-testsuite+
Target Milestone: --- → mozilla9
Comment 35•13 years ago
|
||
Backed out of inbound since bholley's 23-changeset push caused xpcshell orange, even after an in-place bustage fix (!!). Was just easier to back the whole push out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/91b82bc49470
Comment 34 try run didn't complete on Windows, which was the platform that failed. Please make sure that all platform pass try before landing a whopping 23 changesets (which when combined with a non-consecutive maemo bustage fix, proved a hassle to backout). Thanks :-)
Updated•13 years ago
|
Target Milestone: mozilla9 → ---
Comment 36•13 years ago
|
||
Relanded on inbound :-)
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=8c30ddd8b511
https://hg.mozilla.org/integration/mozilla-inbound/rev/f841024a4288
https://hg.mozilla.org/integration/mozilla-inbound/rev/aef192bef235
https://hg.mozilla.org/integration/mozilla-inbound/rev/96405a264d9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/de1c69fb961e
https://hg.mozilla.org/integration/mozilla-inbound/rev/7934b2288e1c
https://hg.mozilla.org/integration/mozilla-inbound/rev/614b6c0627b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca6bb27d3527
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d4cbdd72c03
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fe54a83eb50
Target Milestone: --- → mozilla9
Comment 37•13 years ago
|
||
toolkit-makefiles.sh adjustments for the js/src/xpconnect/tests/components/ makefile changes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/85ca334bad1d
Comment 38•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f841024a4288
https://hg.mozilla.org/mozilla-central/rev/aef192bef235
https://hg.mozilla.org/mozilla-central/rev/96405a264d9d
https://hg.mozilla.org/mozilla-central/rev/de1c69fb961e
https://hg.mozilla.org/mozilla-central/rev/7934b2288e1c
https://hg.mozilla.org/mozilla-central/rev/614b6c0627b9
https://hg.mozilla.org/mozilla-central/rev/ca6bb27d3527
https://hg.mozilla.org/mozilla-central/rev/3d4cbdd72c03
https://hg.mozilla.org/mozilla-central/rev/2fe54a83eb50
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 39•13 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•