Closed Bug 485736 Opened 15 years ago Closed 15 years ago

Add (TUnit) 'xpcshell-tests' |make| target, using |runxpcshelltests.py| new '--manifest' option

Categories

(Testing :: XPCShell Harness, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: sgautherie, Assigned: sgautherie)

References

Details

(Keywords: fixed1.9.1, Whiteboard: [fixed1.9.1b99])

Attachments

(10 files, 6 obsolete files)

3.66 KB, patch
ted
: review+
Details | Diff | Splinter Review
976 bytes, patch
Callek
: review+
Details | Diff | Splinter Review
2.57 KB, patch
Details | Diff | Splinter Review
4.31 KB, patch
bhearsum
: review+
Details | Diff | Splinter Review
1.39 KB, patch
kairo
: review+
Details | Diff | Splinter Review
4.68 KB, patch
ted
: review+
Details | Diff | Splinter Review
8.74 KB, patch
ted
: review+
Details | Diff | Splinter Review
4.42 KB, patch
ted
: review+
Details | Diff | Splinter Review
4.42 KB, patch
standard8
: review+
Details | Diff | Splinter Review
1.21 KB, patch
coop
: review+
Details | Diff | Splinter Review
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(-*).
Blocks: 451474
Depends on: 421611
No longer depends on: 485672
Target Milestone: --- → mozilla1.9.2a1
Attached patch (Av1) Add 'xpcstest' target (obsolete) — Splinter Review
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>.)
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #369976 - Flags: review?(ted.mielczarek)
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.
Summary: Switch to new '--manifest' to run TUnit tests: 'make xpctests' → Switch to new '--manifest' to run TUnit tests: 'make xpcshell-tests'
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>.
Attached patch (Bv1-CC) Add target (obsolete) — Splinter Review
Attachment #370069 - Flags: review?(bugzilla)
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.
Attachment #369976 - Flags: review?(ted.mielczarek) → review-
Av1, with comment 8 suggestion(s).
Attachment #369976 - Attachment is obsolete: true
Attachment #370459 - Flags: review?(ted.mielczarek)
Bv1-CC, with comment 8 suggestion(s).
Attachment #370069 - Attachment is obsolete: true
Attachment #370460 - Flags: review?(bugzilla)
Attachment #370069 - Flags: review?(bugzilla)
Flags: in-testsuite-
Summary: Switch to new '--manifest' to run TUnit tests: 'make xpcshell-tests' → Add (TUnit) 'xpcshell-tests' |make| target, using |runxpcshelltests.py| new '--manifest' option
Attachment #370459 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 370459 [details] [diff] [review]
(Av2-MC) Add target
[Checkin: Comment 11 & 12]


http://hg.mozilla.org/mozilla-central/rev/2fae81427a55
Attachment #370459 - Attachment description: (Av2-MC) Add target → (Av2-MC) Add target [Checkin: Comment 11]
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
Attachment #370459 - Attachment description: (Av2-MC) Add target [Checkin: Comment 11] → (Av2-MC) Add target [Checkin: Comment 11 & 12]
Whiteboard: [Leave open till fully fixed] [fixed1.9.1b4: Av2-MC]
Attached patch (Cv1-FF) Call the new target (obsolete) — Splinter Review
(untested, but seems quite obvious)
Attachment #370550 - Flags: review?(ted.mielczarek)
Attachment #370550 - Flags: review?(bhearsum)
Comment on attachment 370460 [details] [diff] [review]
(Bv1a-CC) Add target
[Checkin: Comment 15]

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


http://hg.mozilla.org/comm-central/rev/fe72cee16752
Attachment #370460 - Attachment description: (Bv1a-CC) Add target → (Bv1a-CC) Add target [Checkin: Comment 15]
Attachment #370550 - Flags: review?(ted.mielczarek) → review+
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.
Attachment #370550 - Flags: review?(bhearsum) → review+
Cv1-FF updated to apply on top of bug 479225 patches.
Attachment #370550 - Attachment is obsolete: true
(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 :-/
Attachment #371193 - Flags: review?(ted.mielczarek)
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
Attachment #371132 - Attachment description: (Cv1a-FF) Call the new target → (Cv1a-FF) Call the new target [Checkin: Comment 25]
Also reduces timeout to 5 mn from 30 mn.
Attachment #371615 - Flags: review?(bugzilla)
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.
Attachment #371615 - Flags: review?(bugzilla)
Blocks: 487377
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.
Attachment #371615 - Attachment description: (Gv1-TB) Call the new target → (Gv1-TB) Call the new target [Moved to bug 487377]
Attachment #371615 - Attachment is obsolete: true
Attachment #371614 - Flags: review?(kairo) → review+
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
Attachment #371610 - Flags: review?(bhearsum) → review+
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
Attachment #371614 - Attachment description: (Fv1-SM) Call the new target → (Fv1-SM) Call the new target [Checkin: Comment 33]
Both targets are running fine on FF :-)
I'll check this in after SM, TB and tryserver are ready for it too.
Attachment #371694 - Flags: review?(ted.mielczarek)
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).
Attachment #371694 - Attachment is obsolete: true
Attachment #372407 - Flags: review?(ted.mielczarek)
Attachment #371694 - Flags: review?(ted.mielczarek)
Dv1, with extended |TEST_PATH| support.

NB: Patch applies cleanly on top of bug 384339 patch Dv1.
Attachment #372416 - Flags: review?(ted.mielczarek)
Attachment #371193 - Attachment is obsolete: true
Attachment #371193 - Flags: review?(ted.mielczarek)
(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.
Attachment #371610 - Attachment description: (Ev1) Call the new target on tryserver → [checked in] (Ev1) Call the new target on tryserver
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| :-|
Attachment #371610 - Attachment description: [checked in] (Ev1) Call the new target on tryserver → (Ev1) Call the new target on tryserver [Checkin: Comment 40]
(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.
Blocks: 488510
(In reply to comment #43)
> That could be a possibility.

I filed bug 488510.
No longer blocks: 485728
Attachment #372407 - Flags: review?(ted.mielczarek) → review+
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.
Blocks: 490255
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
Attachment #372407 - Attachment description: (Hv1a) Stop XPCSHELL_TESTS execution by 'check' target → (Hv1a) Stop XPCSHELL_TESTS execution by 'check' target [Checkin: Comment 46]
Follow-up to Hv1a.
Attachment #374715 - Flags: review?(ted.mielczarek)
Attachment #372416 - Flags: review?(ted.mielczarek) → review+
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)
Attachment #372416 - Attachment description: (Dv1a) Add |EXTRA_TEST_ARGS| support, enhance |--test| feature → (Dv1a) Add |EXTRA_TEST_ARGS| support, enhance |--test| feature [Checkin: Comment 49]
Whiteboard: [Leave open till fully fixed] [fixed1.9.1b4: Av2-MC] → [needs 1.9.1 landing: Hv1a depends on bug 483100] [Leave open till fully fixed] [fixed1.9.1b4: Av2-MC]
Depends on: 483100
Whiteboard: [needs 1.9.1 landing: Hv1a depends on bug 483100] [Leave open till fully fixed] [fixed1.9.1b4: Av2-MC] → [needs 1.9.1 landing: Hv1a (+ Dv1a) depends on bug 480069] [Leave open till fully fixed] [fixed1.9.1b4: Av2-MC]
Attachment #374715 - Flags: review?(ted.mielczarek) → review+
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).
Attachment #374715 - Attachment description: (Iv1-MC) Update '.PHONY' target too → (Iv1-MC) Update '.PHONY' target too [Checkin: See comment 51]
Whiteboard: [needs 1.9.1 landing: Hv1a (+ Dv1a) depends on bug 480069] [Leave open till fully fixed] [fixed1.9.1b4: Av2-MC] → [needs 1.9.1 landing: Hv1a (+ Dv1a + Iv1a-MC) depends on bug 480069] [Leave open till fully fixed] [fixed1.9.1b4: Av2-MC]
Attachment #374716 - Flags: review?(bugzilla) → review+
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
Attachment #374716 - Attachment description: (Kv1-CC) Stop XPCSHELL_TESTS execution by 'check' target, Update '.PHONY' target too → (Kv1-CC) Stop XPCSHELL_TESTS execution by 'check' target, Update '.PHONY' target too [Checkin: Comment 52]
(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...
Blocks: 491376
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
}
Attachment #372407 - Attachment description: (Hv1a) Stop XPCSHELL_TESTS execution by 'check' target [Checkin: Comment 46] → (Hv1a) Stop XPCSHELL_TESTS execution by 'check' target [Checkin: Comment 46 & 56]
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
Attachment #372416 - Attachment description: (Dv1a) Add |EXTRA_TEST_ARGS| support, enhance |--test| feature [Checkin: Comment 49] → (Dv1a) Add |EXTRA_TEST_ARGS| support, enhance |--test| feature [Checkin: Comment 49 & 57]
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
Attachment #374715 - Attachment description: (Iv1-MC) Update '.PHONY' target too [Checkin: See comment 51] → (Iv1-MC) Update '.PHONY' target too [Checkin: See comment 51 & 58]
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.)
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing: Hv1a (+ Dv1a + Iv1a-MC) depends on bug 480069] [Leave open till fully fixed] [fixed1.9.1b4: Av2-MC] → [Leave open till fully fixed] [fixed1.9.1b5]
Just leave it be, it's close enough.
Blocks: 491784
Whiteboard: [Leave open till fully fixed] [fixed1.9.1b5] → [Leave open till fully fixed] [fixed1.9.1b99]
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".
Keywords: dev-doc-needed
Please ignore last comment - it's only in one place, and I can fix that.
Keywords: dev-doc-needed
Attachment #375562 - Flags: review?(bhearsum)
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.
Attachment #375562 - Flags: review?(ccooper)
Attachment #375562 - Flags: review?(ccooper) → review+
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
Attachment #375562 - Attachment description: (Jv1) unittest.py: remove default 'check' value now → (Jv1) unittest.py: remove default 'check' value now [Checkin: comment 65]
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [Leave open till fully fixed] [fixed1.9.1b99] → [fixed1.9.1b99]
You need to log in before you can comment on or make changes to this bug.