Create unified XPConnect argument conversion testing framework

RESOLVED FIXED in mozilla9

Status

()

Core
XPConnect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla9
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 9 obsolete attachments)

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
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.
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.
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!
Attachment #561060 - Flags: review?(khuey)
Created attachment 561061 [details] [diff] [review]
part 2 - Move the C++ implementation of the test component into its own subdirectory. v2
Attachment #561061 - Flags: review?(khuey)
Created attachment 561062 [details] [diff] [review]
part 3 - Introduce js-implemented test component.
Attachment #561062 - Flags: review?(khuey)
Created attachment 561063 [details] [diff] [review]
part 4 - Fix up test_readwriteattributes.js to use new component structure. v1
Attachment #561063 - Flags: review?(khuey)
Created attachment 561064 [details] [diff] [review]
part 5 - Make the read/write attribute tests deliberately check each property, in order. v1
Attachment #561064 - Flags: review?(khuey)
Created attachment 561065 [details] [diff] [review]
part 6 - Generalize test_readwriteattributes.js into test_attributes.js. v1
Attachment #561065 - Flags: review?(khuey)
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.
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)
Created attachment 561663 [details] [diff] [review]
part 1 - Remove TestXPC.cpp, which currently printfs that it's failing various things and then segfaults. v1
Attachment #561663 - Flags: review?(khuey)
Created attachment 561665 [details] [diff] [review]
part 2 - Start building a small subset of the xpconnect test component again, kill the rest. v2
Attachment #561665 - Flags: review?(khuey)
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.
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
Created attachment 561667 [details] [diff] [review]
part 4 - Move the C++ implementation of the test component into its own subdirectory. v2
Attachment #561667 - Flags: review?(khuey)
Created attachment 561668 [details] [diff] [review]
part 5 - Introduce js-implemented test component. v2
Attachment #561668 - Flags: review?(khuey)
Created attachment 561669 [details] [diff] [review]
part 6 - Add an xpcshell test that exercises both the native and js components. v1
Attachment #561669 - Flags: review?(khuey)
Created attachment 561670 [details] [diff] [review]
part 7 - Generalize test_readwriteattributes.js into test_attributes.js. v1
Attachment #561670 - Flags: review?(khuey)
Created attachment 561671 [details] [diff] [review]
part 8 - Test parameter passing. v2
Attachment #561671 - Flags: review?(khuey)
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.
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.
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. ;-)
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+
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.
Attachment #562101 - Flags: review?(khuey)
Attachment #562101 - Flags: review?(khuey) → review+
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 :-)

Updated

6 years ago
Target Milestone: mozilla9 → ---
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
toolkit-makefiles.sh adjustments for the js/src/xpconnect/tests/components/ makefile changes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/85ca334bad1d
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/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.