Closed Bug 937637 Opened 10 years ago Closed 10 years ago

Please schedule cpp unittests on all trunk trees and make them ride the trains

Categories

(Release Engineering :: General, defect)

Product:
Release Engineering
Component:
General
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: dminor, Assigned: dminor)

References

Details

Attachments

(2 files, 5 obsolete files)

address review comments
13.34 KB, patch
rail
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
set gecko version for esr24
351 bytes, patch
catlee
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review 
CPP unittests are running green again on Cedar. Please schedule them for desktop and android 4.0 on all trunk trees, and then make them ride the trains after that.
I can't test this locally, but based on reading the code, I *think* this is the proper way to do things. If this looks good, I'll use a similar approach for desktop.
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Attachment #8341123 - Flags: review?(jgriffin)
Attachment #8341123 - Flags: feedback?(bugspam.Callek)
Comment on attachment 8341123 [details] [diff] [review]
Part 1 - Schedule cpp unittests for android 4

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

r+ with this fixed

::: mozilla-tests/mobile_config.py
@@ +1134,5 @@
>                          BRANCHES[branch]['platforms'][platform][slave_plat][type].remove(suite)
>  
> +# schedule cpp unittests for pandas, rides trains
> +# https://bugzilla.mozilla.org/show_bug.cgi?id=937637
> +for branch, name in items_before(BRANCHES, 'gecko_version', 28):

items_before returns a name, branch tuple, not a branch, name tuple
Attachment #8341123 - Flags: review?(jgriffin) → review+
(In reply to Dan Minor [:dminor] from comment #3)
> Thanks, pushed to
> https://hg.mozilla.org/build/buildbot-configs/rev/3a693a723841

Generally you need a releng review before you can push live. I have not yet done so.

Secondarily you also have CI failure on your push so please back it out:

[14:41:52]	releng-ci	Project buildbot-configs_tests build #1370:FAILURE in 2 min 37 sec: http://10.134.48.37:8080/job/buildbot-configs_tests/1370/
[14:41:53]	releng-ci	Dan Minor <dminor@mozilla.com>: Bug 937637 - Part 1 Schedule cpp unittests on android 4.0; r=jgriffin

I'll officially review soon
as an example of the type of error this had:

INFO  - FAILED-MASTER bm19-tests1-tegra-universal, log: '', dir: ''
INFO  - creating "bm81-tests_scheduler" master
INFO  - created  "bm81-tests_scheduler" master, running checkconfig
INFO  - starting to print log file '/run/shm/buildbot/bm81-tests_scheduler-3ZSaRX-checkconfig.log'
INFO  - Traceback (most recent call last):
INFO  -   File "/var/lib/jenkins/jobs/buildbot-configs_tests/workspace/.pyenv/local/lib/python2.7/site-packages/buildbot-0.8.2_hg_21e19427a084_production_0.8-py2.7.egg/buildbot/scripts/runner.py", line 1042, in doCheckConfig
INFO  -     ConfigLoader(configFileName=configFileName)
INFO  -   File "/var/lib/jenkins/jobs/buildbot-configs_tests/workspace/.pyenv/local/lib/python2.7/site-packages/buildbot-0.8.2_hg_21e19427a084_production_0.8-py2.7.egg/buildbot/scripts/checkconfig.py", line 31, in __init__
INFO  -     self.loadConfig(configFile, check_synchronously_only=True)
INFO  -   File "/var/lib/jenkins/jobs/buildbot-configs_tests/workspace/.pyenv/local/lib/python2.7/site-packages/buildbot-0.8.2_hg_21e19427a084_production_0.8-py2.7.egg/buildbot/master.py", line 652, in loadConfig
INFO  -     exec f in localDict
INFO  -   File "/run/shm/buildbot/bm81-tests_scheduler-3ZSaRX/master.cfg", line 10, in <module>
INFO  -     import mobile_config
INFO  -   File "/run/shm/buildbot/bm81-tests_scheduler-3ZSaRX/mobile_config.py", line 1139, in <module>
INFO  -     for platform in BRANCHES[branch]['platforms']:
INFO  - TypeError: unhashable type: 'dict'
INFO  - finished printing log file '/run/shm/buildbot/bm81-tests_scheduler-3ZSaRX-checkconfig.log'
ERROR - TEST-FAIL bm81-tests_scheduler failed to run checkconfig
Comment on attachment 8341123 [details] [diff] [review]
Part 1 - Schedule cpp unittests for android 4

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

::: mozilla-tests/mobile_config.py
@@ +1139,1 @@
>      for platform in BRANCHES[branch]['platforms']:

looks like this needs to become:
   for platform in branch['platforms']

or -
   for platform in BRANCHES[name]['platforms']
I've removed the explicit fedora/fedora64 check and added cppunit to the list of ubuntu tests, which seems the proper way of doing things.

Currently cpp unittests are not being scheduled for linux on Try while the jit-tests are, and the difference seems to be this check. I think what is going on is that since the tests are not in the list of ubuntu tests, they are being classified as fedora tests, and then we are skipping running them on fedora. It looks like there is logic to not apply this to cedar, which is why they work there.
Attachment #8341233 - Flags: review?(jgriffin)
Comment on attachment 8341233 [details] [diff] [review]
Part 2 - Schedule cpp unittests on desktop

Need to fix issues with part 1.
Attachment #8341233 - Flags: review?(jgriffin)
Callek, sorry I did not wait for the second review the first time around, I wasn't aware of the process.

Fixed, combined version of the earlier patches.
Attachment #8341123 - Attachment is obsolete: true
Attachment #8341233 - Attachment is obsolete: true
Attachment #8341123 - Flags: feedback?(bugspam.Callek)
Attachment #8341245 - Flags: review?(jgriffin)
Attachment #8341245 - Flags: review?(bugspam.Callek)
Comment on attachment 8341245 [details] [diff] [review]
Enable cpp unittests on trunk trees.

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

::: mozilla-tests/config.py
@@ +1838,3 @@
>              continue
>          for slave_platform in PLATFORMS[platform]['slave_platforms']:
>              if slave_platform not in BRANCHES[branch]['platforms'][platform]:

All instances of BRANCHES[branch] inside this loop should be replaced just branch.  (They could also be replaced by BRANCHES[name], but that's not consistent with the rest of the file)

::: mozilla-tests/mobile_config.py
@@ +1143,5 @@
>              continue
>          if platform.endswith('-debug'):
>              continue  # no slave_platform for debug
>          for slave_plat in PLATFORMS[platform]['slave_platforms']:
>              if not slave_plat in BRANCHES[branch]['platforms'][platform]:

Same as config.py; s/BRANCHES[branch]/branch/
Attachment #8341245 - Flags: review?(jgriffin) → review-
Well, that was embarassing.
Attachment #8341245 - Attachment is obsolete: true
Attachment #8341245 - Flags: review?(bugspam.Callek)
Attachment #8342351 - Flags: review?(jgriffin)
Attachment #8342351 - Flags: review?(bugspam.Callek)
Comment on attachment 8342351 [details] [diff] [review]
Enable cpp unittests on trunk trees.

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

lgtm
Attachment #8342351 - Flags: review?(jgriffin) → review+
Attached patch turn cpp unittests on by default (obsolete) — — Splinter Review 
Now that we're running these by default on trunk we need to add them to the default set of things, and turn them _off_ in the loops. This patch should do that do. Based on my before and after runs of builder_list.py, this doesn't remove any existing builders, and adds the cpp builders to try, mozilla-central, and the project branches.
Attachment #8342351 - Attachment is obsolete: true
Attachment #8342351 - Flags: review?(bugspam.Callek)
Attachment #8343938 - Flags: review?(rail)
Comment on attachment 8343938 [details] [diff] [review]
turn cpp unittests on by default

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

I have a question and a comment.

::: mozilla-tests/config.py
@@ +1868,5 @@
>          if slave_platform in BRANCHES['try']['platforms'][platform]:
>              if BRANCHES['try']['platforms'][platform][slave_platform]['opt_unittest_suites']:
>                  BRANCHES['try']['platforms'][platform][slave_platform]['opt_unittest_suites'] += JITTEST[:]
>              else:
> +                BRANCHES['try']['platforms'][platform][slave_platform]['opt_unittest_suites'] += JITTEST[:]

Aren't the both branches of if/else the same?

::: mozilla-tests/mobile_config.py
@@ +1212,2 @@
>                  continue
> +            for type in branch['platforms'][platform][slave_plat]:

A nit: can you rename "type" to something else -- "type" is a builtin function.
(In reply to Rail Aliiev [:rail] from comment #15)
> Comment on attachment 8343938 [details] [diff] [review]
> turn cpp unittests on by default
> 
> Review of attachment 8343938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have a question and a comment.
> 
> ::: mozilla-tests/config.py
> @@ +1868,5 @@
> >          if slave_platform in BRANCHES['try']['platforms'][platform]:
> >              if BRANCHES['try']['platforms'][platform][slave_platform]['opt_unittest_suites']:
> >                  BRANCHES['try']['platforms'][platform][slave_platform]['opt_unittest_suites'] += JITTEST[:]
> >              else:
> > +                BRANCHES['try']['platforms'][platform][slave_platform]['opt_unittest_suites'] += JITTEST[:]
> 
> Aren't the both branches of if/else the same?

Good point. I forgot to adjust this tidying that I did.

> ::: mozilla-tests/mobile_config.py
> @@ +1212,2 @@
> >                  continue
> > +            for type in branch['platforms'][platform][slave_plat]:
> 
> A nit: can you rename "type" to something else -- "type" is a builtin
> function.

Sure...
Attachment #8343966 - Flags: review?(rail) → review+
Attachment #8343938 - Attachment is obsolete: true
Attachment #8343938 - Flags: review?(rail)
Comment on attachment 8343966 [details] [diff] [review]
address review comments

This will go into production around 12:30/1pm eastern.
Attachment #8343966 - Flags: checked-in+
This is running on esr24 for some reason...need to fix that.
Turns out we never did this. Doing this removes the following builders:
-Rev5 MacOSX Mountain Lion 10.8 mozilla-esr24 opt test cppunit ScriptFactory
-Rev5 MacOSX Mountain Lion 10.8 mozilla-esr24 debug test cppunit ScriptFactory
-Rev4 MacOSX Snow Leopard 10.6 mozilla-esr24 opt test cppunit ScriptFactory
-Rev4 MacOSX Snow Leopard 10.6 mozilla-esr24 debug test cppunit ScriptFactory
-WINNT 6.2 mozilla-esr24 opt test cppunit ScriptFactory
-WINNT 6.2 mozilla-esr24 pgo test cppunit ScriptFactory
-WINNT 6.2 mozilla-esr24 debug test cppunit ScriptFactory
-Windows 7 32-bit mozilla-esr24 opt test cppunit ScriptFactory
-Windows 7 32-bit mozilla-esr24 pgo test cppunit ScriptFactory
-Windows XP 32-bit mozilla-esr24 opt test cppunit ScriptFactory
-Windows XP 32-bit mozilla-esr24 pgo test cppunit ScriptFactory
-Ubuntu VM 12.04 x64 mozilla-esr24 opt test cppunit ScriptFactory
-Ubuntu VM 12.04 x64 mozilla-esr24 pgo test cppunit ScriptFactory
-Ubuntu VM 12.04 x64 mozilla-esr24 debug test cppunit ScriptFactory
-Ubuntu VM 12.04 mozilla-esr24 opt test cppunit ScriptFactory
-Ubuntu VM 12.04 mozilla-esr24 pgo test cppunit ScriptFactory
-Ubuntu VM 12.04 mozilla-esr24 debug test cppunit ScriptFactory
-Rev5 MacOSX Mountain Lion 10.8 release-mozilla-esr24 opt test cppunit ScriptFactory
-Rev5 MacOSX Mountain Lion 10.8 release-mozilla-esr24 debug test cppunit ScriptFactory
-Rev4 MacOSX Snow Leopard 10.6 release-mozilla-esr24 opt test cppunit ScriptFactory
-Rev4 MacOSX Snow Leopard 10.6 release-mozilla-esr24 debug test cppunit ScriptFactory
-WINNT 6.2 release-mozilla-esr24 opt test cppunit ScriptFactory
-WINNT 6.2 release-mozilla-esr24 pgo test cppunit ScriptFactory
-WINNT 6.2 release-mozilla-esr24 debug test cppunit ScriptFactory
-Windows 7 32-bit release-mozilla-esr24 opt test cppunit ScriptFactory
-Windows 7 32-bit release-mozilla-esr24 pgo test cppunit ScriptFactory
-Windows XP 32-bit release-mozilla-esr24 opt test cppunit ScriptFactory
-Windows XP 32-bit release-mozilla-esr24 pgo test cppunit ScriptFactory
-Ubuntu VM 12.04 x64 release-mozilla-esr24 opt test cppunit ScriptFactory
-Ubuntu VM 12.04 x64 release-mozilla-esr24 pgo test cppunit ScriptFactory
-Ubuntu VM 12.04 x64 release-mozilla-esr24 debug test cppunit ScriptFactory
-Ubuntu VM 12.04 release-mozilla-esr24 opt test cppunit ScriptFactory
-Ubuntu VM 12.04 release-mozilla-esr24 pgo test cppunit ScriptFactory
-Ubuntu VM 12.04 release-mozilla-esr24 debug test cppunit ScriptFactory
Attachment #8345867 - Flags: review?(catlee)
Attachment #8345867 - Flags: review?(catlee) → review+
Comment on attachment 8345867 [details] [diff] [review]
set gecko version for esr24

Landed on default. Planning to reconfig tomorrow morning.
Attachment #8345867 - Flags: checked-in+
In production...I think we're all done here now.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.