Closed Bug 445611 Opened 11 years ago Closed 11 years ago

try server should also be able to run unit tests

Categories

(Release Engineering :: General, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: lsblakk)

References

Details

Attachments

(5 files, 8 obsolete files)

11.47 KB, patch
bhearsum
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
18.44 KB, patch
bhearsum
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
21.36 KB, patch
bhearsum
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
1.23 KB, patch
catlee
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
2.88 KB, patch
catlee
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
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 .
Component: Release Engineering → Try Server
Priority: -- → P3
Product: mozilla.org → Webtools
QA Contact: release → try-server
Target Milestone: --- → Future
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.
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.
Moving this into the right component so it at least gets triaged.
Component: Try Server → Release Engineering
Priority: P3 → --
Product: Webtools → mozilla.org
QA Contact: try-server → release
Target Milestone: Future → ---
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.
Component: Release Engineering → Release Engineering: Future
Priority: -- → P3
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.
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.
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).
Attached patch patch rev 1 (obsolete) — Splinter Review
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.
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Attachment #346249 - Flags: review?(bhearsum)
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..
Tests are enabled by default, and the default tryserver mozconfigs don't disable them.
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 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.
Attachment #346249 - Flags: review?(bhearsum) → review-
(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.
Ping me on IRC and I'll give the details.
Attached patch patch rev 2 (obsolete) — Splinter Review
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.
Attachment #346249 - Attachment is obsolete: true
Attachment #347109 - Flags: review?(bhearsum)
(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?
(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.
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.
Maybe, right now I'm interested in the fastest path to getting unit tests on the try server.
And you think setting up new build slaves is going to be the fastest?
(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.
(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.
Attached patch patch rev 3 (obsolete) — Splinter Review
Corrects a small syntax error.
Attachment #347109 - Attachment is obsolete: true
Attachment #347258 - Flags: review?(bhearsum)
Attachment #347109 - Flags: review?(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).
Attachment #347258 - Flags: review?(bhearsum) → review-
Attached patch patch rev 4 (obsolete) — Splinter Review
Attachment #347258 - Attachment is obsolete: true
Attachment #347622 - Flags: review?(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.
Attachment #347622 - Flags: review?(bhearsum) → review+
Depends on: 464256
--enable-logrefcnt will bias performance numbers.  (It'll make all reference counting seem slower.)
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
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.
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?
(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.
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.
Assignee: dtownsend → nobody
Component: Release Engineering: Future → Release Engineering
Priority: P3 → P2
Summary: should have try server builders that run unit tests → try server should also be able to run unit tests
Assignee: nobody → lukasblakk
Linux has been tested and has run on staging successfully.
Attachment #347622 - Attachment is obsolete: true
Attachment #363375 - Flags: review?(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.
Attachment #363375 - Flags: review?(bhearsum) → review-
(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 ?
(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?
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.
(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?
(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.
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.
Attachment #363375 - Attachment is obsolete: true
Attachment #364145 - Flags: review?(bhearsum)
Attachment #364145 - Flags: review?(bhearsum) → review-
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.
Hope this one applies cleanly.
Attachment #364145 - Attachment is obsolete: true
Attachment #364319 - Flags: review?(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.
Attachment #364319 - Flags: review?(bhearsum) → review+
Comment on attachment 364319 [details] [diff] [review]
Add a new linux unittest builder to try server

changeset:   963:9f48da91bbc2
Attachment #364319 - Flags: checked‑in+
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.
Attachment #366729 - Flags: review?(bhearsum)
Here's the adjustments to the unittest steps and factory as per https://bugzilla.mozilla.org/show_bug.cgi?id=479225
Attachment #366730 - Flags: review?(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.
Attachment #366730 - Flags: review?(bhearsum) → review-
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.
Attachment #366729 - Flags: review?(bhearsum) → review-
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.
Attachment #366730 - Attachment is obsolete: true
Attachment #367147 - Flags: review?(bhearsum)
Removed hard tabs.
Attachment #366729 - Attachment is obsolete: true
Attachment #367153 - Flags: review?(bhearsum)
Attachment #367147 - Flags: review?(bhearsum) → review+
Attachment #367153 - Flags: review?(bhearsum) → review+
Comment on attachment 367147 [details] [diff] [review]
Revised buildbotcustom patch for steps.py and factory.py

changeset:   222:54c692efb3cd
Attachment #367147 - Flags: checked‑in+
Comment on attachment 367153 [details] [diff] [review]
Revised all platform try unittests

changeset:   1012:5b6b347cc281
Attachment #367153 - Flags: checked‑in+
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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
}
It's not doing anything different than anything else. Is runreftest.py exiting with an error here?
(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
}
(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).
(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
Lukas, ping for fix?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Ben, is Lukas gone? Can you fix the regression? Thanks.
(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?
(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?
Sorry this took so long.  Hope that we can close this bug now.
Attachment #379582 - Flags: review?(bhearsum)
Attachment #379582 - Flags: review?(bhearsum) → review?(catlee)
Attachment #379582 - Flags: review?(catlee) → review+
Attachment #379582 - Flags: checked‑in?
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 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.
Attachment #379582 - Flags: review+ → review?
(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 :-/
Status: REOPENED → ASSIGNED
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.
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.
Attachment #382808 - Flags: review?(catlee)
Attachment #382808 - Flags: review?(catlee) → review+
Attachment #379582 - Flags: review? → review+
Attachment #382808 - Flags: checked‑in?
Attachment #379582 - Flags: checked‑in? → checked‑in+
Attachment #382808 - Flags: checked‑in? → checked‑in+
Checked in patches are working well, all three platforms have NO_FAIL_ON_TEST_ERRORS set now.  Closing.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.