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'.
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.
Created attachment 8449279 [details] [diff] [review] Always create a new FirefoxRunner v1
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
Created attachment 8449294 [details] [diff] [review] Always create a new FirefoxRunner v1.1 Sorry, messed up the last export of the patch.
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+
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. https://hg.mozilla.org/mozilla-central/rev/424600750c75 We also need backports here for aurora and beta, given that tps on those branches also makes use of mozrunner 6.0 now.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox31: --- → affected
status-firefox32: unaffected → affected
status-firefox33: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
https://hg.mozilla.org/releases/mozilla-aurora/rev/af4b196d4a6a I will wait with the backport to beta until the next Aurora build.
status-firefox32: affected → fixed
Depends on: 1033271
status-firefox31: affected → fixed
You need to log in before you can comment on or make changes to this bug.