Closed Bug 684327 Opened 13 years ago Closed 13 years ago

Create unified XPConnect argument conversion testing framework

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(10 files, 9 obsolete files)

547 bytes, text/plain
Details
173.20 KB, patch
khuey
: review+
Details | Diff | Splinter Review
5.12 KB, patch
khuey
: review+
Details | Diff | Splinter Review
3.07 KB, patch
khuey
: review+
Details | Diff | Splinter Review
2.92 KB, patch
khuey
: review+
Details | Diff | Splinter Review
5.37 KB, patch
khuey
: review+
Details | Diff | Splinter Review
17.27 KB, patch
khuey
: review+
Details | Diff | Splinter Review
28.63 KB, patch
khuey
: review+
Details | Diff | Splinter Review
13.93 KB, patch
khuey
: review+
Details | Diff | Splinter Review
1.88 KB, 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.
Blocks: 652571
Attached file 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.
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.
Thanks to Mook for the initial patch!
Attachment #561060 - Flags: review?(khuey)
Attachment #561062 - Flags: review?(khuey)
This serves as test coverage for bug 683802, which the WebAPI team needs landed before the branch.
Attachment #561066 - Flags: review?(khuey)
Attachment #561050 - Flags: review?(khuey)
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.
Attachment #561060 - Attachment is obsolete: true
Attachment #561060 - Flags: review?(khuey)
Attachment #561061 - Attachment is obsolete: true
Attachment #561061 - Flags: review?(khuey)
Attachment #561062 - Attachment is obsolete: true
Attachment #561062 - Flags: review?(khuey)
Attachment #561063 - Attachment is obsolete: true
Attachment #561063 - Flags: review?(khuey)
Attachment #561064 - Attachment is obsolete: true
Attachment #561064 - Flags: review?(khuey)
Attachment #561065 - Attachment is obsolete: true
Attachment #561065 - Flags: review?(khuey)
Attachment #561066 - Attachment is obsolete: true
Attachment #561066 - Flags: review?(khuey)
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)
Attachment #561666 - Attachment description: part 2 - Misc fixes to xpctest_attributes. v1 → part 3 - Misc fixes to xpctest_attributes. v1
Attachment #561671 - Flags: review?(khuey)
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+
> 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.
(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.
> > +  // 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.
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+
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)
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
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 :-)
Target Milestone: mozilla9 → ---
toolkit-makefiles.sh adjustments for the js/src/xpconnect/tests/components/ makefile changes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/85ca334bad1d
Depends on: 689288
Depends on: 689350
No longer depends on: 689350
You need to log in before you can comment on or make changes to this bug.