1123945 - mach mochitest-chrome --no-autorun won't allow running

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

If I use the command

mach mochitest-chrome browser/devtools/webide --no-autorun

then clicking the button to actually run fails with:

JavaScript error: chrome://mochikit/content/tests/SimpleTest/setup.js, line 253: TypeError: RunSet is undefined

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: 0002-Bug-1123945-Fix-no-autorun-harness-option.-r-jmaher.patch

Comment 2

[Joel Maher ( :jmaher ) (UTC -8) (PTO'ish until July 5)]

Comment on attachment 8552058 [details] [diff] [review]
0002-Bug-1123945-Fix-no-autorun-harness-option.-r-jmaher.patch

Review of attachment 8552058 [details] [diff] [review]:
-----------------------------------------------------------------

I like most of this, but I do want to learn more before r+.

::: testing/mochitest/tests/SimpleTest/setup.js
@@ +58,5 @@
>  }
>  
>  if (config.testRoot == "chrome" || config.testRoot == "a11y") {
> +  for (let p in params) {
> +    if (params[p] == [1]) {

I don't understand the [1] and [0], is there a specific javascript feature for this?

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8552058 [details] [diff] [review]
0002-Bug-1123945-Fix-no-autorun-harness-option.-r-jmaher.patch

Review of attachment 8552058 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/tests/SimpleTest/setup.js
@@ +58,5 @@
>  }
>  
>  if (config.testRoot == "chrome" || config.testRoot == "a11y") {
> +  for (let p in params) {
> +    if (params[p] == [1]) {

No, nothing that magical.

|parseQueryString| is called with useArrays: true, so it puts each query param's value in an array, I think to allow for multiple values with the same param name?

I'll confess I don't understand what's expected to happen here at all, so I was mostly just flailing to get this to work. :)

I looked through version history, but did not see any recent changes to explain why this broke...  so I tried to make what seemed like the smallest change possible.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

See comment 3.  Maybe a different approach is needed?  Not sure.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8) (PTO'ish until July 5)]

I see where you get it.  have you tested this patch with mochitest-plain?  on try server to see what android and b2g do?

Assuming this works for everything, I would like a small comment explaining why we are comparing to an array.  This wouldn't make sense just reading it a year from now, without some context in this bug.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=afb9d1fe78e0

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Seems some contexts don't load the file with "let" support.  For such a small change, I've just gone back to "var".

Let's see what happens.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=09555dbc59aa

Comment 8

[Joel Maher ( :jmaher ) (UTC -8) (PTO'ish until July 5)]

Comment on attachment 8552058 [details] [diff] [review]
0002-Bug-1123945-Fix-no-autorun-harness-option.-r-jmaher.patch

Review of attachment 8552058 [details] [diff] [review]:
-----------------------------------------------------------------

please add a comment to the code to indicate why you have the []

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Joel Maher (:jmaher) from comment #8)
> Comment on attachment 8552058 [details] [diff] [review]
> 0002-Bug-1123945-Fix-no-autorun-harness-option.-r-jmaher.patch
> 
> Review of attachment 8552058 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> please add a comment to the code to indicate why you have the []

Thanks! I've got a comment in the one[1] on Try now.  Was going to re-request review once I was sure it looked good, but this works too! :)

Looking okay, except for some kind of Mac build failure, waiting on a rebased Try run to confirm.

[1]: https://hg.mozilla.org/try/rev/9bda75f63db7

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

With the additional run[1], it looks like the Mac issues were fixed after rebase.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d796ff6e72ff

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://hg.mozilla.org/integration/fx-team/rev/8b6b57a1d6eb

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b6b57a1d6eb