Last Comment Bug 684327 - Create unified XPConnect argument conversion testing framework
: Create unified XPConnect argument conversion testing framework
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla9
Assigned To: Bobby Holley (PTO through June 13)
:
Mentors:
Depends on: 689288
Blocks: 652571 683802
  Show dependency treegraph
 
Reported: 2011-09-02 12:59 PDT by Bobby Holley (PTO through June 13)
Modified: 2011-09-26 15:55 PDT (History)
5 users (show)
bobbyholley: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patches to review (547 bytes, text/plain)
2011-09-19 15:49 PDT, Bobby Holley (PTO through June 13)
no flags Details
part 1 - Start building xpconnect test component again, and revive read/write attributes test. v1 (18.94 KB, patch)
2011-09-19 16:07 PDT, Bobby Holley (PTO through June 13)
no flags Details | Diff | Review
part 2 - Move the C++ implementation of the test component into its own subdirectory. v2 (12.68 KB, patch)
2011-09-19 16:07 PDT, Bobby Holley (PTO through June 13)
no flags Details | Diff | Review
part 3 - Introduce js-implemented test component. (3.02 KB, patch)
2011-09-19 16:08 PDT, Bobby Holley (PTO through June 13)
no flags Details | Diff | Review
part 4 - Fix up test_readwriteattributes.js to use new component structure. v1 (6.42 KB, patch)
2011-09-19 16:09 PDT, Bobby Holley (PTO through June 13)
no flags Details | Diff | Review
part 5 - Make the read/write attribute tests deliberately check each property, in order. v1 (3.96 KB, patch)
2011-09-19 16:10 PDT, Bobby Holley (PTO through June 13)
no flags Details | Diff | Review
part 6 - Generalize test_readwriteattributes.js into test_attributes.js. v1 (7.01 KB, patch)
2011-09-19 16:10 PDT, Bobby Holley (PTO through June 13)
no flags Details | Diff | Review
part 7 - Test parameter passing. v1 (20.14 KB, patch)
2011-09-19 16:12 PDT, Bobby Holley (PTO through June 13)
no flags Details | Diff | Review
part 1 - Remove TestXPC.cpp, which currently printfs that it's failing various things and then segfaults. v1 (28.64 KB, patch)
2011-09-21 23:12 PDT, Bobby Holley (PTO through June 13)
no flags Details | Diff | Review
part 2 - Start building a small subset of the xpconnect test component again, kill the rest. v2 (173.20 KB, patch)
2011-09-21 23:26 PDT, Bobby Holley (PTO through June 13)
khuey: review+
Details | Diff | Review
part 3 - Misc fixes to xpctest_attributes. v1 (9.42 KB, patch)
2011-09-21 23:27 PDT, Bobby Holley (PTO through June 13)
khuey: review+
Details | Diff | Review
part 4 - Move the C++ implementation of the test component into its own subdirectory. v2 (5.12 KB, patch)
2011-09-21 23:28 PDT, Bobby Holley (PTO through June 13)
khuey: review+
Details | Diff | Review
part 5 - Introduce js-implemented test component. v2 (3.07 KB, patch)
2011-09-21 23:28 PDT, Bobby Holley (PTO through June 13)
khuey: review+
Details | Diff | Review
part 6 - Add an xpcshell test that exercises both the native and js components. v1 (2.92 KB, patch)
2011-09-21 23:29 PDT, Bobby Holley (PTO through June 13)
khuey: review+
Details | Diff | Review
part 7 - Generalize test_readwriteattributes.js into test_attributes.js. v1 (5.37 KB, patch)
2011-09-21 23:29 PDT, Bobby Holley (PTO through June 13)
khuey: review+
Details | Diff | Review
part 8 - Test parameter passing. v2 (17.27 KB, patch)
2011-09-21 23:30 PDT, Bobby Holley (PTO through June 13)
khuey: review+
Details | Diff | Review
part 1 - Remove TestXPC.cpp, which currently printfs that it's failing various things and then segfaults. v2 (28.63 KB, patch)
2011-09-21 23:40 PDT, Bobby Holley (PTO through June 13)
khuey: review+
Details | Diff | Review
part 3 - Misc fixes to xpctest_attributes. v2 (13.93 KB, patch)
2011-09-22 13:47 PDT, Bobby Holley (PTO through June 13)
khuey: review+
Details | Diff | Review
part 9 - Package the typelib along with the components so that it's accessible from the test harness. v1 (1.88 KB, patch)
2011-09-23 10:44 PDT, Bobby Holley (PTO through June 13)
khuey: review+
Details | Diff | Review

Description Bobby Holley (PTO through June 13) 2011-09-02 12:59:57 PDT
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.
Comment 1 Bobby Holley (PTO through June 13) 2011-09-19 15:49:45 PDT
Created attachment 561050 [details]
patches to review

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.
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-09-19 15:51:03 PDT
(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.
Comment 3 Bobby Holley (PTO through June 13) 2011-09-19 16:07:19 PDT
Created attachment 561060 [details] [diff] [review]
part 1 - Start building xpconnect test component again, and revive read/write attributes test. v1

Thanks to Mook for the initial patch!
Comment 4 Bobby Holley (PTO through June 13) 2011-09-19 16:07:59 PDT
Created attachment 561061 [details] [diff] [review]
part 2 - Move the C++ implementation of the test component into its own subdirectory. v2
Comment 5 Bobby Holley (PTO through June 13) 2011-09-19 16:08:16 PDT
Created attachment 561062 [details] [diff] [review]
part 3 - Introduce js-implemented test component.
Comment 6 Bobby Holley (PTO through June 13) 2011-09-19 16:09:44 PDT
Created attachment 561063 [details] [diff] [review]
part 4 - Fix up test_readwriteattributes.js to use new component structure. v1
Comment 7 Bobby Holley (PTO through June 13) 2011-09-19 16:10:07 PDT
Created attachment 561064 [details] [diff] [review]
part 5 - Make the read/write attribute tests deliberately check each property, in order. v1
Comment 8 Bobby Holley (PTO through June 13) 2011-09-19 16:10:31 PDT
Created attachment 561065 [details] [diff] [review]
part 6 - Generalize test_readwriteattributes.js into test_attributes.js. v1
Comment 9 Bobby Holley (PTO through June 13) 2011-09-19 16:12:03 PDT
Created attachment 561066 [details] [diff] [review]
part 7 - Test parameter passing. v1

This serves as test coverage for bug 683802, which the WebAPI team needs landed before the branch.
Comment 10 Bobby Holley (PTO through June 13) 2011-09-20 18:15:14 PDT
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.
Comment 11 Bobby Holley (PTO through June 13) 2011-09-21 23:12:28 PDT
Created attachment 561663 [details] [diff] [review]
part 1 - Remove TestXPC.cpp, which currently printfs that it's failing various things and then segfaults. v1
Comment 12 Bobby Holley (PTO through June 13) 2011-09-21 23:26:50 PDT
Created attachment 561665 [details] [diff] [review]
part 2 - Start building a small subset of the xpconnect test component again, kill the rest. v2
Comment 13 Bobby Holley (PTO through June 13) 2011-09-21 23:27:56 PDT
Created attachment 561666 [details] [diff] [review]
part 3 - Misc fixes to xpctest_attributes. v1

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.
Comment 14 Bobby Holley (PTO through June 13) 2011-09-21 23:28:29 PDT
Created attachment 561667 [details] [diff] [review]
part 4 - Move the C++ implementation of the test component into its own subdirectory. v2
Comment 15 Bobby Holley (PTO through June 13) 2011-09-21 23:28:52 PDT
Created attachment 561668 [details] [diff] [review]
part 5 - Introduce js-implemented test component. v2
Comment 16 Bobby Holley (PTO through June 13) 2011-09-21 23:29:10 PDT
Created attachment 561669 [details] [diff] [review]
part 6 - Add an xpcshell test that exercises both the native and js components. v1
Comment 17 Bobby Holley (PTO through June 13) 2011-09-21 23:29:39 PDT
Created attachment 561670 [details] [diff] [review]
part 7 - Generalize test_readwriteattributes.js into test_attributes.js. v1
Comment 18 Bobby Holley (PTO through June 13) 2011-09-21 23:30:07 PDT
Created attachment 561671 [details] [diff] [review]
part 8 - Test parameter passing. v2
Comment 19 Bobby Holley (PTO through June 13) 2011-09-21 23:40:31 PDT
Created attachment 561672 [details] [diff] [review]
part 1 - Remove TestXPC.cpp, which currently printfs that it's failing various things and then segfaults. v2

Fix some trivial bitrot in the TestXPC.cpp removal.
Comment 20 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-09-22 10:10:53 PDT
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.
Comment 21 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-09-22 10:19:19 PDT
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).
Comment 22 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-09-22 10:26:45 PDT
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
Comment 23 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-09-22 10:28:46 PDT
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?
Comment 24 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-09-22 10:30:16 PDT
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.
Comment 25 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-09-22 10:32:04 PDT
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)?
Comment 26 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-09-22 10:34:40 PDT
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.
Comment 27 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-09-22 10:43:14 PDT
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 ..
Comment 28 Bobby Holley (PTO through June 13) 2011-09-22 11:55:00 PDT
> 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.
Comment 29 Bobby Holley (PTO through June 13) 2011-09-22 12:07:04 PDT
(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.
Comment 30 Bobby Holley (PTO through June 13) 2011-09-22 12:19:18 PDT
> > +  // 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.
Comment 31 Bobby Holley (PTO through June 13) 2011-09-22 13:47:01 PDT
Created attachment 561871 [details] [diff] [review]
part 3 - Misc fixes to xpctest_attributes. v2

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. ;-)
Comment 32 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-09-22 13:53:37 PDT
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.
Comment 33 Bobby Holley (PTO through June 13) 2011-09-23 10:44:54 PDT
Created attachment 562101 [details] [diff] [review]
part 9 - Package the typelib along with the components so that it's accessible from the test harness. v1

This was causing the tests to fail on tinderbox due to the way they're packaged. Flagging khuey for review.
Comment 34 Bobby Holley (PTO through June 13) 2011-09-23 15:00:50 PDT
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
Comment 35 Ed Morley [:emorley] 2011-09-23 19:50:06 PDT
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 :-)
Comment 37 Ed Morley [:emorley] 2011-09-25 08:21:07 PDT
toolkit-makefiles.sh adjustments for the js/src/xpconnect/tests/components/ makefile changes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/85ca334bad1d
Comment 39 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-26 07:41:42 PDT
https://hg.mozilla.org/mozilla-central/rev/85ca334bad1d

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