Closed
Bug 1061982
Opened 10 years ago
Closed 10 years ago
Allow subsuites to be conditional
Categories
(Testing :: Mochitest, defect)
Testing
Mochitest
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla35
People
(Reporter: jgriffin, Assigned: jgriffin)
References
Details
Attachments
(1 file, 1 obsolete file)
5.19 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
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'
Comment 1•10 years ago
|
||
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]
=================================================
Assignee | ||
Comment 2•10 years ago
|
||
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'.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jgriffin
Comment 4•10 years ago
|
||
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-
Comment 5•10 years ago
|
||
Can't we just filter the manifests with a "subsuite" key?
e.g:
[DEFAULT]
skip-if = subsuite != "media"
Assignee | ||
Comment 6•10 years ago
|
||
(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.
Assignee | ||
Comment 7•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8483841 -
Attachment is obsolete: true
Comment 8•10 years ago
|
||
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+
Assignee | ||
Comment 9•10 years ago
|
||
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
Assignee | ||
Comment 10•10 years ago
|
||
Target Milestone: --- → mozilla35
Comment 11•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•