Closed Bug 1061982 Opened 10 years ago Closed 10 years ago

Allow subsuites to be conditional

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla35

People

(Reporter: jgriffin, Assigned: jgriffin)

References

Details

Attachments

(1 file, 1 obsolete file)

In bug 994920, we'd like to create a 'media' subsuite for mochitests, but only on B2G. Currently, this isn't possible. I'd like to extend the current subsuite mechanism to be optionally conditional. I propose doing this by extending the --subsuite syntax a bit, to: --subsuite suite_name[,condition] where condition would be evaluated by the manifestparser according to the same rules that are used by the manifests. So, for bug 994920, we could specify: --subsuite media,toolkit=='gonk' Another approach would be to split the condition into a separate argument, e.g., --subsuite media --subsuite-if toolkit=='gonk'
What would the manifest annotations look like in this case? Would it be easier to just have something like: moz.build ================================================= if CONFIG['MOZ_B2G']: MOCHITEST_MANIFESTS += ['mochitest-b2g.ini'] else: MOCHITEST_MANIFESTS += ['mochitest.ini'] ================================================= mochitest-b2g.ini ================================================= [default] subsuite = media [include mochitest.ini] =================================================
Hmm, maybe we should apply the condition in the manifest rather than the command-line: mochitest.ini ============= [test_something.html] subsuite = media,toolkit=='gonk' ...which would mean that test belongs to the 'media' subsuite iff toolkit=='gonk'.
With tests.
Attachment #8483841 - Flags: review?(jmaher)
Assignee: nobody → jgriffin
Comment on attachment 8483841 [details] [diff] [review] manifestparser changes to support conditional subsuites, Review of attachment 8483841 [details] [diff] [review]: ----------------------------------------------------------------- I like the tests, but I would like to see us be a bit more robust with our parsing ::: testing/mozbase/manifestparser/manifestparser/manifestparser.py @@ +1100,5 @@ > + for test in tests: > + subsuite = test.get('subsuite') > + if ',' in subsuite: > + subsuite, condition = subsuite.split(',') > + matched = parse(condition, **values) my concern here is if we have >1 condition specified. The syntax gets confusing, here are some examples: * subsuite=webgl,os!=android,platform!=b2g # bug 157 * subsuite=webgl,(os=='linux'&&debug) # bug 31415926 1) do comments work in this condition? 2) it could easily end up where test authors add larger conditions 3) do we support a full clause of && || () for a condition? 4) the , can lead people to not use && || and just add conditions Most of this can be solved simply- I suggest adding a check for further commas and then adding some documentation for this feature. ::: testing/mozbase/manifestparser/tests/test_testmanifest.py @@ +58,5 @@ > + # 5 tests total > + self.assertEquals(len(manifest.active_tests(exists=False, **info)), 5) > + > + # only 2 test for subsuite bar when foo==bar > + self.assertEquals(len(manifest.active_tests(exists=False, nit: after each False, there is a trailing whitespace
Attachment #8483841 - Flags: review?(jmaher) → review-
Can't we just filter the manifests with a "subsuite" key? e.g: [DEFAULT] skip-if = subsuite != "media"
(In reply to Andrew Halberstadt [:ahal] from comment #5) > Can't we just filter the manifests with a "subsuite" key? > > e.g: > > [DEFAULT] > skip-if = subsuite != "media" Not really. The desired effect here is to have the subsuite setting for the media subsuite completely ignored if we're not running B2G, but have it processed in the normal way if we are. The existing subsuite mechanism already filters out subsuite tests if you're not targeting that subsuite...i.e., it already implicitly does what you're suggesting above.
Addressed review comments. The syntax is exactly the same as for skip-if, so multiple conditions are chained via || or && and not commas. I added a check for superfluous commas and bail with an error if present, and now strip comments if present.
Attachment #8484333 - Flags: review?(jmaher)
Attachment #8483841 - Attachment is obsolete: true
Comment on attachment 8484333 [details] [diff] [review] manifestparser changes to support conditional subsuites, Review of attachment 8484333 [details] [diff] [review]: ----------------------------------------------------------------- I like this, just would like to see additional tests- not a blocker. ::: testing/mozbase/manifestparser/tests/subsuite.ini @@ +6,5 @@ > + > +[test3] > +subsuite=baz > + > +[test4] can we add a failure case here with: subsuite=bza,foo="bar",bar="false"
Attachment #8484333 - Flags: review?(jmaher) → review+
Updated with two new tests, one for the error case and one for chained conditions. pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=386f95109c25
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 1056045
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: