TPS infinitely loops through the first phase of a test

RESOLVED FIXED in Firefox 31


4 years ago
4 years ago


(Reporter: whimboo, Assigned: whimboo)



Dependency tree / graph

Firefox Tracking Flags

(firefox31 fixed, firefox32 fixed, firefox33 fixed)



(1 attachment, 1 obsolete attachment)



4 years ago
I have seen today that we loop through the same phase of a test again and again. The runner correctly passes the phase number as argument to mozrunner, but in the cmd handler of the extension we get always '1'.

Comment 1

4 years ago
Just another regression from bug 997244. The changes for cmdargs have been moved from its own line into the FirefoxRunner constructor. Given that for each run we update the args, those will never end-up being used, because the code is never executed again. Given that the cmdargs member is not a public interface we should stop updating it.

So we have two options here:

1. Create the runner instance each time we have to launch Firefox, so the command will get the proper arguments

2. Change TPS to not use cmdargs for it's phase and test values, but pass them over via preferences.

For me option 2 is an enhancement which takes a bit amount of work to finish, whereby option 1 enables us to run TPS tests again today. I don't think that this option even hurts us, instead it makes us more stable for possible future mozrunner changes. From the beginning it was called out that you should better create a new mozrunner instance, as modifying its class members.
Blocks: 997244
Keywords: regression

Comment 2

4 years ago
Created attachment 8449279 [details] [diff] [review]
Always create a new FirefoxRunner v1
Attachment #8449279 - Flags: review?(ahalberstadt)


4 years ago
status-firefox32: --- → unaffected
status-firefox33: --- → affected
Summary: TPS infinitely loops through the same phase of a test → TPS infinitely loops through the first phase of a test

Comment 3

4 years ago
Created attachment 8449294 [details] [diff] [review]
Always create a new FirefoxRunner v1.1

Sorry, messed up the last export of the patch.
Attachment #8449279 - Attachment is obsolete: true
Attachment #8449279 - Flags: review?(ahalberstadt)
Attachment #8449294 - Flags: review?(ahalberstadt)
Comment on attachment 8449294 [details] [diff] [review]
Always create a new FirefoxRunner v1.1

Review of attachment 8449294 [details] [diff] [review]:

Lgtm! Note: I wouldn't be opposed to changing mozrunner back to supporting this use case.. This patch does seem simplest though. Thanks for catching this!
Attachment #8449294 - Flags: review?(ahalberstadt) → review+

Comment 5

4 years ago
I think the current behavior is fine from Mozrunner. As long as we don't have a public property for the arguments we should munging with them.

We also need backports here for aurora and beta, given that tps on those branches also makes use of mozrunner 6.0 now.
Last Resolved: 4 years ago
status-firefox31: --- → affected
status-firefox32: unaffected → affected
status-firefox33: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla33

Comment 6

4 years ago

I will wait with the backport to beta until the next Aurora build.
status-firefox32: affected → fixed
Depends on: 1033271

Comment 7

4 years ago
status-firefox31: affected → fixed
You need to log in before you can comment on or make changes to this bug.