445611 - try server should also be able to run unit tests

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It seems like it's almost every day that somebody says "gosh, I wish the try servers ran unit tests".  Yet I couldn't find a bug on it, so I'm filing one.

In a tiny bit more detail:  it would be good to have unit test machines for each platform so that when patches are submitted to the try server according to http://wiki.mozilla.org/Build:TryServer there would be builds on
http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry that run and report the same test results as the machines with "unit test" in their names on http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox .

Comment 1

[(not currently active) Ted Mielczarek]

This should probably be in mozilla.org : Release Engineering, since it's more about getting the machines setup to run unit tests than it is changing try server code.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hm, I could've sworn a bug existed from a long time ago, but I can't find it now either.

This is pretty key for development, because very few developers have access to all three main platforms (and even less so things like x86-64).  Given our now very extensive testing framework, there are often cross-platform test issues that arise from all sorts of patches.  Having try server unit test coverage would reduce the number of patches bouncing out of the tree due to test failures, and would prevent the tree from needing to be held closed due to test failures.  It also gives people a much better checkpoint as they're using the try server to check compilation and performance, so that issues are caught early.

So, if there's anything that the dev team can do to help this along faster, please let me know; I'd like to see this happen.

Comment 3

[(not currently active) Ted Mielczarek]

Moving this into the right component so it at least gets triaged.

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

It will also make it easier for contributors without commit access to get quicker service, because they'll be able to indicate to potential checkers-in that the risk of landing is lower.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Any progress/ETA on this?  The lack of try server unit test functionality has chewed up significant time for myself and others this week.  My offer from comment #2 still stands:

> So, if there's anything that the dev team can do to help this along faster,
> please let me know; I'd like to see this happen.

Comment 6

[(not currently active) Ted Mielczarek]

Given that this has been triaged into "Release Engineering: Future" I can guarantee that nobody is planning to work on it at the moment. The RelEng team has their hands full at the moment, so until they get some more hands on deck, it's probably not happening. I think lsblakk should be back on part-time soon, so that might help some.

Literally, unless you want them to prioritize this above doing *releases* right now, they don't have the staff to do it.

Comment 7

[[e-mail] (:bhearsum)]

What Ted said is right. Right now we have a few things taking absolute priority:
Releases
Mercurial Release Automation
Mercurial l10n infrastructure (which we barely have a start on).

Comment 8

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Copied the buildbot steps across so I think this is right, though I don't think I have anyway to test it unless there are simple steps for setting up my own tryserver?

My only concern is I am not passing an env to the steps for OSX and Linux, however the rest of tryserver doesn't for those platforms so I suspect it is ok.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

That won't work, no?  The current builds don't build with --enable-testing, and I don't know that we'd want to block on tests to actually having the build binaries available..

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Tests are enabled by default, and the default tryserver mozconfigs don't disable them.

Comment 11

[(not currently active) Ted Mielczarek]

Yeah, build-wise they should be fine. The things I'd be worried about are: are the screen resolutions/color depths set correctly? Do the Linux slaves have Xvfb running and DISPLAY set accordingly? Also, there's only one Mac slave, so this is going to make that a scarce resource.

Comment 12

[[e-mail] (:bhearsum)]

Comment on attachment 346249 [details] [diff] [review]
patch rev 1

As Vlad points out we should probably run tests after packaging and uploading -- assuming packaging doesn't strip any important pieces out.

And as Ted points out the slaves need some setup done on them - I'm certain unittests won't work without some work there. We're planning on adding more build slaves in the next week or so, that would help mitigate the scarce resource issue.

Currently, running unittests in the same builder will delay Talos from starting. There's a patch that still needs try talos deployment that will poll FTP instead of Tinderbox, which will work around this. But if unittests happen before that patch is deployed Talos results will take significantly longer.

I don't know if the steps as called will correctly run the unit tests. When someone has time, we need to test this out in the staging environment. Dave, I can give you access to that if you'd like to give it a shot. Personally, I'm bogged down with Beta 2 work right now.

To sum up, here's what needs to be tested/solved:
* make sure tests still work post packaging/uploading
* make sure the tests run _at all_

After those things are done we will have to make time for setting up machines with the proper display settings and such. And ftr, I can't promise a timeline on such things.

Comment 13

[Dave Townsend [:mossop]]

(In reply to comment #12)
> I don't know if the steps as called will correctly run the unit tests. When
> someone has time, we need to test this out in the staging environment. Dave, I
> can give you access to that if you'd like to give it a shot. Personally, I'm
> bogged down with Beta 2 work right now.

Give me access and I can see what I can do.

Comment 14

[[e-mail] (:bhearsum)]

Ping me on IRC and I'll give the details.

Comment 15

[Dave Townsend [:mossop]]

Attachment: patch rev 2

This is a slightly different approach to the last patch. Instead of adding the tests into the existing builders I've added new unit test builders for each platform, which perform the base build steps and then run through the test suite. This is necessary to allow us to use a separate mozconfig for the build to enable tests.

I have tested this in the staging environment and with the windows and linux slaves available managed to get runs that ran and passed all tests. However the runs were randomly failing on a couple of tests, mostly the same tests that randomly fail on tinderbox so I take this as a good sign.

There were a few tweaks necessary to the staging slaves I used, but from what I see they were just the ones documented on setting up unit test slaves. Basically what we should do ideally is have the existing try server slaves for the build and upload builders and then some unit test slaves for running the tests. Obviously the patch will need a few tweaks to put new slaves into the unit test builders.

Comment 16

[(not currently active) Ted Mielczarek]

(In reply to comment #15)
> This is a slightly different approach to the last patch. Instead of adding the
> tests into the existing builders I've added new unit test builders for each
> platform, which perform the base build steps and then run through the test
> suite. This is necessary to allow us to use a separate mozconfig for the build
> to enable tests.

What's the benefit here? Why wouldn't we want the normal builders to have tests enabled?

Comment 17

[Dave Townsend [:mossop]]

(In reply to comment #16)
> (In reply to comment #15)
> > This is a slightly different approach to the last patch. Instead of adding the
> > tests into the existing builders I've added new unit test builders for each
> > platform, which perform the base build steps and then run through the test
> > suite. This is necessary to allow us to use a separate mozconfig for the build
> > to enable tests.
> 
> What's the benefit here? Why wouldn't we want the normal builders to have tests
> enabled?

Well I figure it makes it easier if the slaves are either normal build slaves or unit test slaves. It also means the builds uploaded are built using the same config as on tinderbox and so mac builds for example won't include all the test cruft.

Comment 18

[(not currently active) Ted Mielczarek]

Note that in bug 463455 I am proposing we combine the build and unit test machines on the normal tinderbox, so this might be a good trial for that.

Comment 19

[Dave Townsend [:mossop]]

Maybe, right now I'm interested in the fastest path to getting unit tests on the try server.

Comment 20

[(not currently active) Ted Mielczarek]

And you think setting up new build slaves is going to be the fastest?

Comment 21

[Dave Townsend [:mossop]]

(In reply to comment #20)
> And you think setting up new build slaves is going to be the fastest?

Well your way has things that still need to be fixed. I don't know the timescale of those. This way we just have to clone some build slaves and do a small amount of config to them. I don't know how long that takes either. However I believe there are some slaves potentially available already.

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #16)
> What's the benefit here? Why wouldn't we want the normal builders to have tests
> enabled?

Because that would mean that someone who wants perf test results would have to wait for the unit tests to finish running before the builds were uploaded for the talos slaves to test.

We have 3 slaves that are mostly set up (images have been cloned), one for each of the top platforms.. that should be enough to get us started.

Comment 23

[Dave Townsend [:mossop]]

Attachment: patch rev 3

Corrects a small syntax error.

Comment 24

[[e-mail] (:bhearsum)]

Comment on attachment 347258 [details] [diff] [review]
patch rev 3

>Index: tools/buildbot-configs/tryserver/master.cfg
>===================================================================
>RCS file: /cvsroot/mozilla/tools/buildbot-configs/tryserver/master.cfg,v
>retrieving revision 1.19
>diff -u -8 -p -r1.19 master.cfg
>--- tools/buildbot-configs/tryserver/master.cfg	6 Oct 2008 13:06:28 -0000	1.19
>+++ tools/buildbot-configs/tryserver/master.cfg	10 Nov 2008 09:22:37 -0000
>@@ -12,16 +12,17 @@ import buildbotcustom.changes.hgpoller
> import buildbotcustom.tryserver.env
> import buildbotcustom.tryserver.steps
> reload(buildbotcustom.changes.hgpoller)
> reload(buildbotcustom.tryserver.env)
> reload(buildbotcustom.tryserver.steps)
> 
> from buildbotcustom.changes.hgpoller import HgPoller
> from buildbotcustom.tryserver.env import MozillaEnvironments
>+from buildbotcustom.unittest.steps import *

Please don't import *, future additions to that file could duplicated variable names resulting in the imported one getting overridden.

> c['schedulers'].append(Scheduler(name="Sendchange hg scheduler",
> c['schedulers'].append(Scheduler(name="Sendchange hg push scheduler",

There's still some CVS trunk patches that come through - is there a reason we can't run unittests on them, too?


Doing two clobber builds per platform per patch is a big waste IMHO. As mentioned in comment#12 there's code to let try talos poll FTP instead of Tinderbox - which will eliminate the "talos doesn't start until a build is finished" issue. Please put the unittests in the existing builders (at the very end, of course).

Comment 25

[Dave Townsend [:mossop]]

Attachment: patch rev 4

Comment 26

[[e-mail] (:bhearsum)]

Comment on attachment 347622 [details] [diff] [review]
patch rev 4

What does --enable-logrefcnt do? I know we have it on for unittests but I wonder if it will affect performance numbers, though. Try talos isn't in a great place right now, anyways, so not going to block on it.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

--enable-logrefcnt will bias performance numbers.  (It'll make all reference counting seem slower.)

Comment 28

[Aki Sasaki (not active)]

i've attached a mac slave to the staging try master and re-enabled tests in the mozconfig.  it's consistently dying with

/builds/buildbot/sendchange-slave/sendchange-mac-hg/mozilla/build/macosx/universal/unify: compareZipArchives: members differ:
  content/autoconf.js

Comment 29

[(not currently active) Ted Mielczarek]

Yeah, reftest converts autoconf.mk to autoconf.js and packages it, so as it stands it can't possibly work in a universal build. Something like bug 420084 could fix that.

Comment 30

[Dave Townsend [:mossop]]

So so far, having regular builders do unit tests means that talos results are potentially no longer in line with what we might see in reality and we can't do universal OSX builds. Should we still proceed with this patch as is?

Comment 31

[Dave Townsend [:mossop]]

(In reply to comment #30)
> So so far, having regular builders do unit tests means that talos results are
> potentially no longer in line with what we might see in reality and we can't do
> universal OSX builds. Should we still proceed with this patch as is?

We probably need to answer this question, as far as I can surmise the slaves are ready so we could go with this as is now.

Comment 32

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Now that all slaves in the main pool can do either builds and/or unittests, we'd like to roll those same new slaves here in the try-server pool-of-try-slaves. This is important because: 

1) the slaves in the pool-of-try-slaves should be identical to the slaves in the main pool-of-slaves, to avoid differences in behavior.
2) with these new slaves, and some changes to try-server master, we get unittests on try for all o.s, and without hitting the talos and mac universal build problems being described below.
3) this model scales better then having dedicated build-only slaves and unittest-only slaves; it avoids bottlenecking on one subset of machines during peak loads.

Lukas did a lot of this work for the main pool-of-slaves, so after talking with mossop in irc, I'm reassigning this bug to Lukas, so she can do the same again here in the pool-of-try-slaves.

Comment 33

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Add unittest steps to Linux tryserver builds

Linux has been tested and has run on staging successfully.

Comment 34

[[e-mail] (:bhearsum)]

Comment on attachment 363375 [details] [diff] [review]
Add unittest steps to Linux tryserver builds

Unfortunately, we can't --enable-logrefcnt on these builders because it will mess up the performance. You're going to have to add separate unittest builders. Don't worry about adding them for CVS, just for Mercurial ones.

On the plus side, this means you can use the UnittestBuildFactory. You might need to set brand_name to '*.app' or something, because they'll need to work on 1.9.1/m-c/whatever.

You can make copies of the unittest mozconfig files in hg.m.o/build/buildbot-configs into the tryserver/ directory.

Sorry, I didn't realize this requirement until I was reviewing this patch. I hope it doesn't cause too much pain for you.

Comment 35

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to comment #34)
> Unfortunately, we can't --enable-logrefcnt on these builders because it will
> mess up the performance. You're going to have to add separate unittest
> builders. Don't worry about adding them for CVS, just for Mercurial ones.

Argh, we'll have the same problem on the main pool right, right ? Will impact bug 383136. logrefcnt is for catching leaks in tests ?

Comment 36

[(not currently active) Ted Mielczarek]

See comment 27, and bug 421611 comment 19, which references comment 27 here.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #35)
> Argh, we'll have the same problem on the main pool right, right ? Will impact
> bug 383136. logrefcnt is for catching leaks in tests ?

I think what we should do in the main pool is switch to running unit tests in two configurations, each of which differs from the current configuration, but in opposite directions:

 (1) run unit tests on the builds we ship.  (This means no leak logging.)

 (2) run unit tests on debug builds.  (This could be done by using debug builders or by doing build+test on the same machine as today.)  We (really really really) need this for assertion testing anyway, and we can also use this configuration for doing the leak testing that we need.

Admittedly, this does have the disadvantage of meaning that our leak testing configuration would become more different from what we ship, but I'm not sure that difference makes it worth having three configurations.


I'm not sure how that affects what we do here.  Having the try server be able to run just one of these configurations would be a huge step up from where we are now.  Would it be so horrible if the try server unit tests started off as just configuration (1) above?

Comment 38

[[e-mail] (:bhearsum)]

I think the try server configuration needs to remain consistent with the main pool. If we run unittests here without --enable-logrefcnt we'll end up with patches pushed to a central repo that leak, and people upset that they weren't caught on try server.

Any changes we make in production because of unittests-on-packaged-builds or anything else should be made here at the same time IMHO.

Comment 39

[Shawn Wilsher :sdwilsh]

(In reply to comment #38)
> I think the try server configuration needs to remain consistent with the main
> pool. If we run unittests here without --enable-logrefcnt we'll end up with
> patches pushed to a central repo that leak, and people upset that they weren't
> caught on try server.
For what it's worth, having unit testing, even without refcount leak testing, would be a huge step forward.  Could we possibly get unit tests without refcount leak testing first, and fix that issue in a follow-up?

Comment 40

[[e-mail] (:bhearsum)]

(In reply to comment #39)
> (In reply to comment #38)
> > I think the try server configuration needs to remain consistent with the main
> > pool. If we run unittests here without --enable-logrefcnt we'll end up with
> > patches pushed to a central repo that leak, and people upset that they weren't
> > caught on try server.
> For what it's worth, having unit testing, even without refcount leak testing,
> would be a huge step forward.  Could we possibly get unit tests without
> refcount leak testing first, and fix that issue in a follow-up?

I think it's better to fix this right off the bat. Historically, it takes us awhile to fix follow-up issues when we have a "good enough" solution. I don't think one way or the other is any appreciable difference in amount of work, anyways.

Comment 41

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Add a new linux unittest builder to try server

So now we're using a separate builder, this has been tested on staging and has passed tests comparable to the current unittest results.

Windows and Mac coming soon.

Comment 42

[[e-mail] (:bhearsum)]

Comment on attachment 364145 [details] [diff] [review]
Add a new linux unittest builder to try server

This is mostly fine. A few specifics:
* You need to import/reload buildbotcustom.unittest, just like the other buildbotcustom modules
* This patch doesn't apply cleanly to the latest buildbot-configs repo, please fix that.
* Where's the mozconfig?

Almost there. We can land this tomorrow as soon as you fix the above.

Comment 43

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Add a new linux unittest builder to try server

Hope this one applies cleanly.

Comment 44

[[e-mail] (:bhearsum)]

Comment on attachment 364319 [details] [diff] [review]
Add a new linux unittest builder to try server

Looks good! I'll land this in a few minutes.

Comment 45

[[e-mail] (:bhearsum)]

Comment on attachment 364319 [details] [diff] [review]
Add a new linux unittest builder to try server

changeset:   963:9f48da91bbc2

Comment 46

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Add a unittest builder for each platform on tryserver

This has to go in conjunction with the make reftest/crashtest/mochitest patch to unittest/steps.py and therefore is also connected to process/factory.py - a second patch will follow.

Tested on try-staging and passed both mac and windows - the windows slave had been set to the wrong colour depth, once that was fixed the build ran no problem.

Comment 47

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Updated unittest/steps.py and process/factory.py

Here's the adjustments to the unittest steps and factory as per https://bugzilla.mozilla.org/show_bug.cgi?id=479225

Comment 48

[[e-mail] (:bhearsum)]

Comment on attachment 366730 [details] [diff] [review]
Updated unittest/steps.py and process/factory.py

>diff --git a/unittest/steps.py b/unittest/steps.py

>-    def __init__(self, **kwargs):
>-        self.description = [self.name + " test"]
>+    def __init__(self, test_name, **kwargs):
>+        self.name = test_name
>+        self.command = ["make", test_name]
>+        self.description = [test_name + " test"]
>         self.descriptionDone = [self.description[0] + " complete"]
> 	self.super_class = ShellCommandReportTimeout
> 	ShellCommandReportTimeout.__init__(self, **kwargs)
>    
>     def createSummary(self, log):
>         # Counts.
>         successfulCount = -1
>         unexpectedCount = -1
>         knownProblemsCount = -1
>         # Regular expression for result summary details.
>         infoRe = re.compile(r"REFTEST INFO \| (Successful|Unexpected|Known problems): (\d+) \(")
>+        command = ["make", self.name]

You already have self.command, why are you creating another variable?

>+	def createSummary(self, log):
>+	    # Support browser-chrome result summary format which differs from MozillaMochitest's.
>+		if (self.name == 'mochitest-browser-chrome'):

Style nit: don't use parentheses here, Python doesn't require them.

>+		else:

Does this logic work for a11y, too? Either way, you should probably be explicit here rather than using a catch-all 'else'.

>+				passCount = 0
>+				failCount = 0
>+				todoCount = 0
>+				leaked = False
>+				command = ["make", self.name]

Same thing here about command.

>+	def evaluateCommand(self, cmd):
>+		# Support browser-chrome result summary format which differs from MozillaMochitest's.
>+		if (self.name == 'mochitest-browser-chrome'):

Parentheses again.

>+			superResult = self.super_class.evaluateCommand(self, cmd)
>+			if SUCCESS != superResult:
>+				return WARNINGS
>+			if re.search('TEST-UNEXPECTED-', cmd.logs['stdio'].getText()):
>+				return WARNINGS
>+			if re.search('FAIL Exited', cmd.logs['stdio'].getText()):
>+				return WARNINGS
>+			return SUCCESS
>+		else:

Here too, wrt being explicit.

>+				superResult = self.super_class.evaluateCommand(self, cmd)
>+				if SUCCESS != superResult:
>+					return WARNINGS
>+				if re.search('TEST-UNEXPECTED-', cmd.logs['stdio'].getText()):
>+					return WARNINGS
>+				if re.search('FAIL Exited', cmd.logs['stdio'].getText()):
>+					return WARNINGS
>+				if not re.search('TEST-PASS', cmd.logs['stdio'].getText()):
>+					return WARNINGS
>+				return SUCCESS
>+				

Overall style nit: Use spaces, not hard tabs for whitespace, and make it consistent. Eg, the section above is indented inconsistently.

I'm not particularly happy that we've blocked try server unittests on this patch landing. We have much fewer opportunities to land on production-master than try. With that said, it shouldn't be a huge deal to get this in soon after the nits are fixed.

r- for the whitespace/indent problems, and other nits listed above. Functionality-wise this patch seems great.

Comment 49

[[e-mail] (:bhearsum)]

Comment on attachment 366729 [details] [diff] [review]
Add a unittest builder for each platform on tryserver

Same thing in this patch w.r.t. tabs - use spaces, not hard tabs.

I'm not going to make you fix it as part of this bug, but please file a follow-up on getting stacks for try server unittest crashes.

r=bhearsum with the tabs fixed.

Comment 50

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Revised buildbotcustom patch for steps.py and factory.py

Took out the hard tabs (sorry, bad text editor), the parentheses, and the extra command variables.

Also, for the if/else - only browser-chrome has different output formatting so yes, that is how it should look I believe.

Comment 51

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Revised all platform try unittests

Removed hard tabs.

Comment 52

[[e-mail] (:bhearsum)]

Comment on attachment 367147 [details] [diff] [review]
Revised buildbotcustom patch for steps.py and factory.py

changeset:   222:54c692efb3cd

Comment 53

[[e-mail] (:bhearsum)]

Comment on attachment 367153 [details] [diff] [review]
Revised all platform try unittests

changeset:   1012:5b6b347cc281

Comment 54

[Lukas Blakk [:lsblakk] use ?needinfo]

small bustage fix needed - to set up the step with test_name = test_name instead of name=test_name in the master.cfg for tryserver.

There is now a unittest builder per platform on try production/staging.

Comment 55

[Serge Gautherie (:sgautherie)]

MacOSX (at least) misses or not honors |NO_FAIL_ON_TEST_ERRORS=1| (on reftests):
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1237288349.1237295430.8833.gz
OS X 10.5.2 mozilla-central unit test on 2009/03/17 04:12:29

REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/mozilla-central-macosx-unittest/build/layout/base/crashtests/99776-1.html | timed out waiting for onload to fire
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/mozilla-central-macosx-unittest/build/layout/base/crashtests/99776-1.html | timed out waiting for onload to fire
make: *** [crashtest] Error 1
Unknown Error: command finished with exit code: 2
}

Comment 56

[(not currently active) Ted Mielczarek]

It's not doing anything different than anything else. Is runreftest.py exiting with an error here?

Comment 57

[Serge Gautherie (:sgautherie)]

(In reply to comment #56)
> Is runreftest.py exiting with an error here?

No, as there's no "TEST-UNEXPECTED-FAIL" complain from 'automation.py':
{
REFTEST INFO | Total canvas count = 0
INFO | (automation.py) | Application ran for: 0:01:49.431190

== BloatView: ALL (cumulative) LEAK STATISTICS
}

Comment 58

[Serge Gautherie (:sgautherie)]

(In reply to comment #55)
> MacOSX (at least) misses or not honors |NO_FAIL_ON_TEST_ERRORS=1| (on
> reftests):

(This example is crashtests actually ... but should not matter.)


(In reply to comment #56)
> It's not doing anything different than anything else.

Actually it's Linux which is different from MacOSX and Windows:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1237297020.1237304926.25185.gz
Linux mozilla-central unit test on 2009/03/17 06:37:00

REFTEST INFO | runreftest.py | Running tests: end.
program finished with exit code 0
TinderboxPrint: reftest<br/><em class="testfail">2777/2/133</em>
}

(MacOSX and) Windows:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1237288349.1237295733.9291.gz&fulltext=1
WINNT 5.2 mozilla-central unit test on 2009/03/17 04:12:29

REFTEST INFO | runreftest.py | Running tests: end.
crashtest passed
program finished with exit code 0
elapsedTime=108.437000
TinderboxPrint: crashtest<br/>1081/0/8
}
Notice "crashtest passed" (and "elapsedTime=108.437000" too).

Comment 59

[Serge Gautherie (:sgautherie)]

(In reply to comment #56)
> It's not doing anything different than anything else.

Actually it is.

(In reply to comment #58)
> Actually it's Linux which is different from MacOSX and Windows:

Tryserver has a shared config.
But production was patched for Linux only :-/

http://mxr.mozilla.org/build/search?string=NO_FAIL_ON_TEST_ERRORS&case=on&find=%2Fbuildbotcustom%2F.*env.py%24

Comment 60

[Serge Gautherie (:sgautherie)]

Lukas, ping for fix?

Comment 61

[Serge Gautherie (:sgautherie)]

Ben, is Lukas gone? Can you fix the regression? Thanks.

Comment 62

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to comment #61)
> Ben, is Lukas gone? Can you fix the regression? Thanks.

Lukas will be back Monday (18th).

(In reply to comment #59)
> (In reply to comment #56)
> > It's not doing anything different than anything else.
> Actually it is.
Can you clarify whats different? I've tried reading the twists/turns of the comments here, and am unclear what exactly is the problem you want fixed?

Comment 63

[Serge Gautherie (:sgautherie)]

(In reply to comment #62)

> Can you clarify whats different?

MacOSX and Windows production are missing |NO_FAIL_ON_TEST_ERRORS=1|!

> I've tried reading the twists/turns of the
> comments here, and am unclear what exactly is the problem you want fixed?

I wonder what is unclear in comment 59 details?

Comment 64

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Adding NO_FAIL_ON_TEST_ERRORS to Mac and win32

Sorry this took so long.  Hope that we can close this bug now.

Comment 65

[Chris AtLee [:catlee]]

Comment on attachment 379582 [details] [diff] [review]
Adding NO_FAIL_ON_TEST_ERRORS to Mac and win32

Lukas, are these environments actually used anywhere by Try?

Comment 66

[Chris AtLee [:catlee]]

Comment on attachment 379582 [details] [diff] [review]
Adding NO_FAIL_ON_TEST_ERRORS to Mac and win32

Un-approving until we can figure where these environments are used, and if Try uses them.

Comment 67

[Serge Gautherie (:sgautherie)]

(In reply to comment #59)
> http://mxr.mozilla.org/build/search?string=NO_FAIL_ON_TEST_ERRORS&case=on&find=%2Fbuildbotcustom%2F.*env.py%24

(In reply to comment #65)
> Lukas, are these environments actually used anywhere by Try?


See
http://mxr.mozilla.org/build/search?string=NO_FAIL_ON_TEST_ERRORS&case=on
and take |make reftest| as an example:

{
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1243644877.1243652571.6449.gz&fulltext=1
Linux try hg unit test on 2009/05/29 17:54:37

  DISPLAY=:2
  NO_FAIL_ON_TEST_ERRORS=1
}
as expected (afaict), from
http://mxr.mozilla.org/build/source/buildbotcustom/env.py#72

{
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1243644877.1243653302.7567.gz&fulltext=1
OS X 10.5.2 try hg unit test on 2009/05/29 17:54:37

("nothing")
}
unexpected, as it does not even have
http://mxr.mozilla.org/build/source/buildbotcustom/env.py#79

{
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1243644877.1243652074.5795.gz&fulltext=1
WINNT 5.2 try hg unit test on 2009/05/29 17:54:37

  MOZ_OBJDIR=obj-firefox
  NO_FAIL_ON_TEST_ERRORS=1
}
explained by
http://mxr.mozilla.org/build/source/buildbot-configs/tryserver/config.py#18
but unexpected, as it does not have
http://mxr.mozilla.org/build/source/buildbotcustom/env.py#86

I admit I'm rather "confused" by this (different on each platform) situation :-/

Comment 68

[Lukas Blakk [:lsblakk] use ?needinfo]

So at the moment - Linux unittest uses env.py:
http://hg.mozilla.org/build/buildbot-configs/file/5cee911de535/tryserver/factories.py#l432

Mac OSX doesn't use any env :
http://hg.mozilla.org/build/buildbot-configs/file/5cee911de535/tryserver/factories.py#l494

And win32 uses:
http://hg.mozilla.org/build/buildbot-configs/file/5cee911de535/tryserver/factories.py#l605

which loads from config.py instead of env.py

Currently looking into best approach for unifying this.

Comment 69

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Corrects the env settings for Try Server Unittests in factories.py

So with this patch to factories.py and the patch from attachment 379582 [details] [diff] [review] everything lines up properly.  Try Server unittests on all platforms now look for their environment variables in buildbotcustom/env.py where NO_FAIL_ON_ERRORS is set.

Comment 70

[Lukas Blakk [:lsblakk] use ?needinfo]

Checked in patches are working well, all three platforms have NO_FAIL_ON_TEST_ERRORS set now.  Closing.