Bug 485672 comment 4: { From Ted Mielczarek (:luser) 2009-03-28 12:51:21 PDT I think I'd like to change how the tests are invoked. Maybe add another target like "make xpcshell-tests" which just runs the tests using the manifest, and stop invoking them from check. }

Suggest not "xpctests", "xpc" being a very loaded prefix. xpcshell-tests is a good name.

Actually, what about 'xpcstest' ? As we have reftest, crashtest, mochitest(-*).

In addition, what do you think about sorting <all-test-dirs.list>, with the exception of having <test_testing_xpcshell_example/unit> first ? I could do it in python, though I don't know where this script should be called from ? (I'd prefer not to do it in <runxpcshelltests.py>.)

I'd rather not sort it. They should be in the order they're encountered during a build, which will mirror the current recursive "make check" behavior. Also, I do want it to be called xpcshell-tests (or at least xpcshell-test), since that's very clear. "mochitest" and "reftest" are the names of those test harnesses. "xpcstest" is not the name of anything.

I don't know what we'll do to preserve a sane behavior for running the tests from a single dir. Once we get this target in and working, I'd like to stop running the xpcshell tests as part of "make check", so we can make the unittest builders run xpcshell tests separately from "make check". I guess we can leave "check-one" and "check-interactive" alone, but that'd still involve breaking "make -C dir check" to run all the tests in that dir, which sucks. I guess we could make "make xpcshell-tests" work differently at the top level and in a dir with XPCSHELL_TESTS defined, such that the former runs all tets, and the latter runs all the tests in that dir.

Copied from irc discussion: *We agree. *I'll fix the target name to 'xpcshell-test' before checkin. *Patch Av1 is only the very first step: lots to do after this. *** (In reply to comment #4) > I'd rather not sort it. They should be in the order they're encountered during > a build, which will mirror the current recursive "make check" behavior. I'm not sure what the advantage is, once we "know" the manifest feature does work as intended ... as the tests themselves are not order dependent. [1] The advantage I see in the sorted+exception is: *Test the harness itself first, as reftest starts with 'reftest-sanity'. *Be in sync' with the disk order, as the mochitests are. [1] Yet, if you're saying the "build order" represent some kind of logical dependency which is good to keep, I'd probably would suggest to do the "test_testing_xpcshell_example first" part only ... which I could simply add to <runxpcshelltests.py>.

Comment on attachment 369976 [details] [diff] [review] (Av1) Add 'xpcstest' target Please move this target to testing/testsuite-targets.mk. Also, I want it named xpcshell-tests, but you knew that.

Av1, with comment 8 suggestion(s).

Bv1-CC, with comment 8 suggestion(s).

Comment on attachment 370459 [details] [diff] [review] (Av2-MC) Add target [Checkin: Comment 11 & 12] http://hg.mozilla.org/mozilla-central/rev/2fae81427a55

Comment on attachment 370459 [details] [diff] [review] (Av2-MC) Add target [Checkin: Comment 11 & 12] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/71b13ae9402b

(untested, but seems quite obvious)

Comment on attachment 370460 [details] [diff] [review] (Bv1a-CC) Add target [Checkin: Comment 15] simple enough, r=me; no need to wait on Mark.

Comment on attachment 370460 [details] [diff] [review] (Bv1a-CC) Add target [Checkin: Comment 15] http://hg.mozilla.org/comm-central/rev/fe72cee16752

Comment on attachment 370550 [details] [diff] [review] (Cv1-FF) Call the new target + self.addStep(unittest_steps.MozillaCheck, You're reusing the same step class for both versions of the step? I don't know if that's common practice here, but I'll let bhearsum comment. FWIW, it looks like this should do the right thing.

(In reply to comment #16) > (From update of attachment 370550 [details] [diff] [review]) > + self.addStep(unittest_steps.MozillaCheck, > > You're reusing the same step class for both versions of the step? Yes, even if we split the xpcshell part out of 'check', what it does/reports remains the same. > I don't know if that's common practice here, but I'll let bhearsum comment. We already use Reftest for 2 and Mochitest for 4, so... > FWIW, it looks like this should do the right thing. Yeah, I just wanted you to confirm what we discussed previously: that we will end up running the xpcshell tests twice for a (short) while. Ben will indeed check the code itself.

Comment on attachment 370550 [details] [diff] [review] (Cv1-FF) Call the new target Aren't we going to run xpcshell-tests twice by doing this?

Yes. The plan is to have a short transition period where we run them twice (they're pretty quick anyway), and then stop calling them from "make check" in the build system. This makes sense going forward to a point where we'll run xpcshell tests from packaged builds, but not the other random stuff in "make check".

Comment on attachment 370550 [details] [diff] [review] (Cv1-FF) Call the new target Looks fine to me.

Cv1-FF updated to apply on top of bug 479225 patches.

(In reply to comment #4) > They should be in the order they're encountered during > a build, which will mirror the current recursive "make check" behavior. Ftr, for whatever reason, this order is not fully the same for check and for xpcshell-tests targets :-/

Comment on attachment 371193 [details] [diff] [review] (Dv1) Add |EXTRA_TEST_ARGS| support, enhance |--test| feature NB: Patch context is bug 384339 patch Dv1.

Comment on attachment 371132 [details] [diff] [review] (Cv1a-FF) Call the new target [Checkin: Comment 25] Checked in: changeset: 247:7c763b51f7ac

Also reduces timeout to 5 mn from 30 mn.

Comment on attachment 371615 [details] [diff] [review] (Gv1-TB) Call the new target [Moved to bug 487377] Serge, please file this patch in a bug in Mozilla Messaging / Release Engineering so that we can track this change correctly.

Comment on attachment 371615 [details] [diff] [review] (Gv1-TB) Call the new target [Moved to bug 487377] (In reply to comment #29) > Serge, please file this patch in a bug in Mozilla Messaging / Release > Engineering so that we can track this change correctly. I filed bug 487377.

Comment on attachment 371614 [details] [diff] [review] (Fv1-SM) Call the new target [Checkin: Comment 33] looks safe for me as it mirrors what the buildbotcustom factory has as well :)

Comment on attachment 371610 [details] [diff] [review] (Ev1) Call the new target on tryserver [Checkin: Comment 40] Looks fine

Comment on attachment 371614 [details] [diff] [review] (Fv1-SM) Call the new target [Checkin: Comment 33] http://hg.mozilla.org/build/buildbot-configs/rev/2ed691cfdf9b

Both targets are running fine on FF :-) I'll check this in after SM, TB and tryserver are ready for it too.

Comment on attachment 371694 [details] [diff] [review] (Hv1) Stop XPCSHELL_TESTS execution by 'check' target One thing I don't like about this patch is that it removes the ability to do: "make -C some/directory check" to run all the tests in that directory. I'm fairly certain people use that functionality when they're hacking on a specific component. Perhaps we should just rename the "check" target here to "xpcshell-tests", so you could instead do: "make -C some/directory xpcshell-tests" to run all the tests in that directory? That way, executing that target at the top-level would run all tests, and executing it in a subdir would execute just that subdir, without recursing. Thoughts?

Hv1, with comment 36 suggestion(s).

Dv1, with extended |TEST_PATH| support. NB: Patch applies cleanly on top of bug 384339 patch Dv1.

(In reply to comment #35) > (From update of attachment 371694 [details] [diff] [review]) > One thing I don't like about this patch is that it removes the ability to do: > "make -C some/directory check" to run all the tests in that directory. Most comm-central folks use that method for mailnews. > Perhaps we should just rename the "check" target here to > "xpcshell-tests", so you could instead do: > "make -C some/directory xpcshell-tests" > to run all the tests in that directory? That way, executing that target at the > top-level would run all tests, and executing it in a subdir would execute just > that subdir, without recursing. Does that mean we'd have to remember to do a separate (additional) run for make check to pick up the compiled code tests?

Comment on attachment 371610 [details] [diff] [review] (Ev1) Call the new target on tryserver [Checkin: Comment 40] This tested fine. Going to land it shortly.

Comment on attachment 371610 [details] [diff] [review] (Ev1) Call the new target on tryserver [Checkin: Comment 40] changeset: 1090:24e70f1f5109

Comment on attachment 371610 [details] [diff] [review] (Ev1) Call the new target on tryserver [Checkin: Comment 40] tryserver master has been reconfig'ed for this. new builds should be running with this code.

(In reply to comment #38) > Does that mean we'd have to remember to do a separate (additional) run for make > check to pick up the compiled code tests? Yes. (Though I think the trend would be to rewrite cpp tests as xpcs ones,) If this is a problem, I think we could add a new target, like |TUnit:: check xpcshell-tests| :-|

(In reply to comment #42) > (Though I think the trend would be to rewrite cpp tests as xpcs ones,) I doubt that's possible. The general reason we have cpp test is because its not possible to do them via xpcshell ones - i.e. direct access to classes on interfaces that aren't exposed via xpcom. > If this is a problem, I think we could add a new target, like > |TUnit:: check xpcshell-tests| :-| That could be a possibility.

(In reply to comment #43) > That could be a possibility. I filed bug 488510.

Comment on attachment 372407 [details] [diff] [review] (Hv1a) Stop XPCSHELL_TESTS execution by 'check' target [Checkin: Comment 46 & 56] Please post about this on dev.planning or somewhere else when you land this, so people aren't blindsided.

Comment on attachment 372407 [details] [diff] [review] (Hv1a) Stop XPCSHELL_TESTS execution by 'check' target [Checkin: Comment 46 & 56] http://hg.mozilla.org/mozilla-central/rev/6b07db0a9661

Follow-up to Hv1a.

Comment on attachment 372416 [details] [diff] [review] (Dv1a) Add |EXTRA_TEST_ARGS| support, enhance |--test| feature [Checkin: Comment 49 & 57] http://hg.mozilla.org/mozilla-central/rev/5a8a199bd62a (with new context to apply on bare tree)

Comment on attachment 374715 [details] [diff] [review] (Iv1-MC) Update '.PHONY' target too [Checkin: See comment 51 & 58] - $(testxpcobjdir)/all-test-dirs.list \ - $(addprefix $(MODULE)/,$(XPCSHELL_TESTS)) + $(testxpcobjdir)/all-test-dirs.list \ + $(addprefix $(MODULE)/,$(XPCSHELL_TESTS)) What's the point of this indentation change? Just leave it out.

Comment on attachment 374715 [details] [diff] [review] (Iv1-MC) Update '.PHONY' target too [Checkin: See comment 51 & 58] http://hg.mozilla.org/mozilla-central/rev/8436c4cf084d Iv1-MC, with comment 50 suggestion(s).

Comment on attachment 374716 [details] [diff] [review] (Kv1-CC) Stop XPCSHELL_TESTS execution by 'check' target, Update '.PHONY' target too [Checkin: Comment 52] http://hg.mozilla.org/comm-central/rev/1df30a3cd989

(In reply to comment #53) > Created an attachment (id=375562) [details] > (Jv1) unittest.py: remove default 'check' value now Why?

(In reply to comment #54) > > (Jv1) unittest.py: remove default 'check' value now > > Why? Why not? It was temporary and is no longer needed...

Comment on attachment 372407 [details] [diff] [review] (Hv1a) Stop XPCSHELL_TESTS execution by 'check' target [Checkin: Comment 46 & 56] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/67b21e7d9da7 After fixing context for { patching file config/rules.mk Hunk #2 FAILED at 2246 1 out of 2 hunks FAILED patching file js/src/config/rules.mk Hunk #2 FAILED at 2246 1 out of 2 hunks FAILED }

Comment on attachment 372416 [details] [diff] [review] (Dv1a) Add |EXTRA_TEST_ARGS| support, enhance |--test| feature [Checkin: Comment 49 & 57] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/750f55f6a54c

Comment on attachment 374715 [details] [diff] [review] (Iv1-MC) Update '.PHONY' target too [Checkin: See comment 51 & 58] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a8279e457be5

Comment on attachment 374715 [details] [diff] [review] (Iv1-MC) Update '.PHONY' target too [Checkin: See comment 51 & 58] (In reply to comment #58) > http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a8279e457be5 After fixing context for { patching file config/rules.mk Hunk #2 FAILED at 2114 1 out of 2 hunks FAILED patching file js/src/config/rules.mk Hunk #2 FAILED at 2114 1 out of 2 hunks FAILED }

(In reply to comment #6) > (In reply to comment #4) > > I'd rather not sort it. They should be in the order they're encountered during > > a build, which will mirror the current recursive "make check" behavior. > > [1] Yet, if you're saying the "build order" represent some kind of logical > dependency which is good to keep, I'd probably would suggest to do the > "test_testing_xpcshell_example first" part only ... which I could simply add to > <runxpcshelltests.py>. Ted, what do you think (again)? (In reply to comment #22) > Ftr, for whatever reason, this order is not fully the same for check and for > xpcshell-tests targets :-/ Maybe because -j<n> was used to build!? (Not sure.)

Just leave it be, it's close enough.

https://developer.mozilla.org/en/Writing_xpcshell-based_unit_tests is quite inconsistent now - several places say "make check", and several others say "make xpcshell-tests".

Please ignore last comment - it's only in one place, and I can fix that.

Comment on attachment 375562 [details] [diff] [review] (Jv1) unittest.py: remove default 'check' value now [Checkin: comment 65] I don't think it's worth my time to review and land this. If someone else wants to, be my guest.

Comment on attachment 375562 [details] [diff] [review] (Jv1) unittest.py: remove default 'check' value now [Checkin: comment 65] http://hg.mozilla.org/build/buildbotcustom/rev/06892ba1bb62