935652 - Add support for the new metro talos suites

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Jordan Lund (:jlund)]

we are adding a bunch of new talos suites for win8 metro.

As metro runs on the same platform as the non metro equivalent, we can either continue adding duplicate suites to win8 (currently what we do) or add a new platform row to give metro its own identity. The latter is starting to make sense as we now have 4 talos suites and 1 unittest suite while others like: mochitests, reftests, etc are planned to be added in the future.

The proposed new platform will be:
Windows 8 Metro: M(mc) T(md, mo, ms, mtp)

Comment 1

[Ryan VanderMeulen [:RyanVM]]

As mentioned on IRC, if the plan is to add more test suites "some time next year", I'd prefer we just punt on this until it becomes a bigger need.

Comment 2

[Jordan Lund (:jlund)]

K sounds good, reverting to original plan of appending to existing win8 line. just refreshing my JS regex patterns and I'll submit patch this afternoon

Comment 3

[Jordan Lund (:jlund)]

Attachment: bug_935652_tbpl_061113.diff <- adds tbpl support for new metro talos suites

Notice that I am changing the regex for tp talos suites. This is because, currently, that regex is picking up 'tp5o-metro'. I added the '5o' as I don't see how it can be anything else anymore besides 'tp5o' (I think tp5n was dropped?). At least that's what I can tell from:
http://hg.mozilla.org/build/buildbot-configs/file/a9112074f912/mozilla-tests/config.py#l223
and
http://hg.mozilla.org/mozilla-central/file/3254963dccbb/testing/talos/talos.json

Do you know if we need to be less explicit with just 'tp' and not 'tp5o'?

How does the rest look?

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 828280 [details] [diff] [review]
bug_935652_tbpl_061113.diff <- adds tbpl support for new metro talos suites

Review of attachment 828280 [details] [diff] [review]:
-----------------------------------------------------------------

Seems to work OK. I'm not nearly as much of a Regex master as Ed, so I'll let him go to work on that in greater depth.

::: js/Config.js
@@ +427,5 @@
>      "Talos roboprovider" : "rpr",
>      "Talos ts" : "ts",
>      "Talos tspaint" : "tsp",
>      "Talos xperf" : "x",
>      "Jetpack SDK Test" : "JP",

Weren't we going to use d-m, s-m, tp-m, and o-m?

Comment 5

[Jordan Lund (:jlund)]

Cool, thx for feedback.

> Weren't we going to use d-m, s-m, tp-m, and o-m?

Happy to. I did that because when I looked in Config.js, I saw that Metro Browser Chrome had 'mc' so I was sticking with the: 'm{suite initial}' template

I'll fire off another patch if you'd prefer '{suite initial}-m'. If we do go with that, should I change metro browser chrome in the patch to 'bc-m' or leave it as is?

:)

Comment 6

[Ed Morley [:emorley]]

Comment on attachment 828280 [details] [diff] [review]
bug_935652_tbpl_061113.diff <- adds tbpl support for new metro talos suites

Review of attachment 828280 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Jordan Lund (:jlund) from comment #5)
> Happy to. I did that because when I looked in Config.js, I saw that Metro
> Browser Chrome had 'mc' so I was sticking with the: 'm{suite initial}'
> template

Aiui the suite 'metro-browser-chrome' is actually an entirely different set of tests, rather than mochitest-browser-chrome run in metro mode.

I don't mind too much either way what we do - let's just pick something and we can always tweak later :-)

f- for the regexp tweak. 

Thank you for writing the patch! :-)

::: js/Data.js
@@ +611,2 @@
>          /talos remote-tp4m_nochrome$/i.test(name) ? "Talos tp nochrome" :
> +        /talos (?:remote-)?tp5o$/i.test(name) ? "Talos tp" :

esr17 and b2g18 are still running the older versions of the 'tp' suite ('tpn'), so this will no longer catch them. Please can you leave this existing tp entry without change, and instead add the metro line above it, so for the metro variants we catch it there first.

Comment 7

[Jim Mathies [:jimm]]

(In reply to Jordan Lund (:jlund) from comment #5)
> Cool, thx for feedback.
> 
> > Weren't we going to use d-m, s-m, tp-m, and o-m?
> 
> Happy to. I did that because when I looked in Config.js, I saw that Metro
> Browser Chrome had 'mc' so I was sticking with the: 'm{suite initial}'
> template
> 
> I'll fire off another patch if you'd prefer '{suite initial}-m'. If we do go
> with that, should I change metro browser chrome in the patch to 'bc-m' or
> leave it as is?
> 
> :)

I'd like to keep the 'mc' moniker for metro-chrome unless someone has a really good reason not too. :) the -m is fine for talos imho.

Comment 8

[Ed Morley [:emorley]]

(In reply to Jim Mathies [:jimm] from comment #7)
> I'd like to keep the 'mc' moniker for metro-chrome unless someone has a
> really good reason not too. :) the -m is fine for talos imho.

wfm - let's go with this :-)

Comment 9

[Jordan Lund (:jlund)]

Attachment: bug_935652_tbpl_071113.diff

Here's a patch to reflect feedback given. :)

thanks,
jlund

Comment 10

[Ed Morley [:emorley]]

Comment on attachment 828779 [details] [diff] [review]
bug_935652_tbpl_071113.diff

Review of attachment 828779 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great - thank you :-)

Comment 11

[Ed Morley [:emorley]]

Added author/commit message and landed :-)
https://hg.mozilla.org/webtools/tbpl/rev/b41f39160d0d

To avoid needing to add the author info, you can get mercurial to do so automatically, see:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

You can see this on tbpl-dev within 15mins; needinfo me for a production push if you need it urgently, otherwise I'm sure we'll do one by start of next week :-)

Comment 12

[Ed Morley [:emorley]]

Ended up pushing to prod now anyway :-)

Comment 13

[Jordan Lund (:jlund)]

(In reply to Ed Morley [:edmorley UTC+1] from comment #12)
> Ended up pushing to prod now anyway :-)

thank you :)

duly noted about author/commit message.