1032853 - TPS infinitely loops through the first phase of a test

Status: RESOLVED FIXED

(Testing Graveyard :: TPS, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[Henrik Skupin [:whimboo][⌚️UTC+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.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Always create a new FirefoxRunner v1

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Always create a new FirefoxRunner v1.1

Sorry, messed up the last export of the patch.

Comment 4

[Andrew Halberstadt [:ahal]]

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!

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/af4b196d4a6a

I will wait with the backport to beta until the next Aurora build.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9d89ab77e097