Closed Bug 1032787 Opened 10 years ago Closed 10 years ago

TPSFirefoxRunner should not reset environment variables

Categories

(Testing Graveyard :: TPS, defect)

defect
Not set
major

Tracking

(firefox31 fixed, firefox32 fixed, firefox33 fixed)

RESOLVED FIXED
mozilla33
Tracking Status
firefox31 --- fixed
firefox32 --- fixed
firefox33 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Keywords: regression)

Attachments

(1 file)

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
Actually this is a regression between the released Mozrunner 5.35 and 5.36. Whatever has landed there, mangles with the environment variables.
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.
Severity: normal → major
OS: Linux → All
Hardware: x86_64 → All
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 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+
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.
Target Milestone: --- → mozilla33
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.
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
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)
Summary: FirefoxRunner should not reset environment variables → TPSFirefoxRunner should not reset environment variables
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.
Makes sense, sorry for removing that.
Flags: needinfo?(ahalberstadt)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-aurora/rev/5c2c09f3da52

I will wait with the backport to beta until the next Aurora build
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: