Closed
Bug 1032787
Opened 10 years ago
Closed 10 years ago
TPSFirefoxRunner should not reset environment variables
Categories
(Testing Graveyard :: TPS, defect)
Testing Graveyard
TPS
Tracking
(firefox31 fixed, firefox32 fixed, firefox33 fixed)
RESOLVED
FIXED
mozilla33
People
(Reporter: whimboo, Assigned: whimboo)
References
Details
(Keywords: regression)
Attachments
(1 file)
1.27 KB,
patch
|
andrei
:
review+
|
Details | Diff | Splinter Review |
Currently the FirefoxRunner in TPS is resetting all the environment variables. That means it fails to start Firefox when it is launched through Jenkins. Reason here is the missing display environment variable. http://mxr.mozilla.org/mozilla-central/source/testing/tps/tps/firefoxrunner.py#82 To avoid that we should not pass in an empty dictionary, but simply leave this parameter alone. Mozrunner will handle it appropriately: http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozrunner/mozrunner/base/runner.py#37
Assignee | ||
Comment 1•10 years ago
|
||
Actually this is a regression between the released Mozrunner 5.35 and 5.36. Whatever has landed there, mangles with the environment variables.
Assignee | ||
Comment 2•10 years ago
|
||
The version bump to 5.36 happened on bug 989961. As given bug 981190 and bug 988382 should be included in this version. Here some other not referenced bugs which went into 5.36: bug 874476, bug 967647, and bug 969146. None of them give me actually a clue why it is failing now. Also having mozbase in mozilla-central makes it very hard to run regression tests. Not sure if it is worth my time to continue to check that. I think I will simply fix TPS, so we pass in all environment variables from the system.
Assignee | ||
Updated•10 years ago
|
Severity: normal → major
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Comment 3•10 years ago
|
||
Given that no one is around, and we have to get this fix in ASAP, I will have to request review from Andrei.
Attachment #8448755 -
Flags: review?(andrei.eftimie)
Comment 4•10 years ago
|
||
Comment on attachment 8448755 [details] [diff] [review] Environment fix v1 Review of attachment 8448755 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.
Attachment #8448755 -
Flags: review?(andrei.eftimie) → review+
Assignee | ||
Comment 5•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3a28d3bb4f55 I don't see such a failure for aurora so far. So lets check back tomorrow, if that branch and beta might also be affected.
status-firefox31:
--- → ?
status-firefox32:
--- → ?
status-firefox33:
--- → fixed
Target Milestone: --- → mozilla33
Assignee | ||
Comment 6•10 years ago
|
||
So interesting here is that the fix on this bug also got our old TPS system working again, which didn't run any test since June 20st. The last working changeset was: https://hg.mozilla.org/mozilla-central/rev/aaaa56174361 So something in the following changeset broke our TPS system: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=aaaa56174361&tochange=bdac18bd6c74 The only change which can really affect us here is Andrew's refactoring of mozrunner in bug 997244. So I would call this out as regressor.
Blocks: 997244
Keywords: regression
Assignee | ||
Comment 7•10 years ago
|
||
The referenced bug is indeed the regressor here! What I haven't seen before is that it changes the firefoxrunner module in a not appropriate way: https://hg.mozilla.org/mozilla-central/diff/f5d1163268ae/testing/tps/tps/firefoxrunner.py
Assignee | ||
Comment 8•10 years ago
|
||
So the problematic lines are: 1.17 - if env is not None: 1.18 - self.runner.env.update(env) With that change we do no longer pass in the system environment variables but only our extra ones. Reason is that FirefoxRunner is called with env=env, which are our env variables. Mozrunner itself checks if env is passed in, and only takes those variables. All system wide env variables are dropped. So I think the way I have handled it here with my patch fixes this bug. I can still see other failures, which all could be based off this specific mozrunner refactoring. It would have been great if tests had been executed, to proof that the refactoring didn't cause problems for others. Sadly I even didn't receive feedback from Edwin, who usually observes the test results. :( Andrew, can you please check my patch which already landed for completeness? Thanks.
Flags: needinfo?(ahalberstadt)
Assignee | ||
Updated•10 years ago
|
Summary: FirefoxRunner should not reset environment variables → TPSFirefoxRunner should not reset environment variables
Assignee | ||
Comment 9•10 years ago
|
||
Hm, given that TPS is installing mozrunner via pypi on the testing machines, the refactoring of Mozrunner and its version 6.0 also affects TPS on aurora and beta. So we should backport this patch. Maybe we should consider moving the package out of mozilla-central given that it contains the same code across all branches. I will wait for Andrew's feedback before I will backport the patch.
Assignee | ||
Updated•10 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/5c2c09f3da52 I will wait with the backport to beta until the next Aurora build
Depends on: 1033271
Assignee | ||
Comment 12•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/959746d4b268
Updated•6 years ago
|
Product: Testing → Testing Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•