1283282 - Certain combinations of mochitests break Trychooser

Status: RESOLVED FIXED

(Infrastructure & Operations Graveyard :: CIDuty, task)

Description

[Robin Steuber (they/them) [:bytesized]]

Steps to Reproduce:
 - Point browser to http://trychooser.pub.build.mozilla.org/
 - Build Types => Both
 - Platforms => All
 - Under Unit Test Suites, check "mochitests (all)"
 - Uncheck mochitest-jetpack

Expected Result:
 - Computed Command and Computed Syntax will display a command that will run all mochitests except for jetpack

Actual Result:
 - Computed Command and Computed Syntax display "(NO JOBS CHOSEN)"

Comment 1

[Andrei Obreja [:aobreja NOT AVAILABLE][:buildduty]]

It seems that the problem is with mochitest-browser-screenshots tests,any combination with these tests will display "(NO JOBS CHOSEN)".

Comment 2

[Andrei Obreja [:aobreja NOT AVAILABLE][:buildduty]]

Attachment: patch_bug1283282_tools.patch

Adding this patch from trychooser.js will solve the problem but this mean that we are not longer need  MOZSCREENSHOTS_SETS to have a value in order to work.
So I need a feedback if is wise to remove this sequence.

Comment 3

[Chris Cooper [:coop] (he/him)]

Comment on attachment 8767157 [details] [diff] [review]
patch_bug1283282_tools.patch

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

I don't know enough about this code. Punting the feedback request to Armen.

Comment 4

[Armen [:armenzg]]

Comment on attachment 8767157 [details] [diff] [review]
patch_bug1283282_tools.patch

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

I only approved that the change was not insane (since I don't know enough JavaScript)
Passing it to the author of the patch.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8767157 [details] [diff] [review]
patch_bug1283282_tools.patch

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

(In reply to Kirk Steuber [:bytesized] from comment #0)
> Steps to Reproduce:
>  - Point browser to http://trychooser.pub.build.mozilla.org/
>  - Build Types => Both
>  - Platforms => All
>  - Under Unit Test Suites, check "mochitests (all)"
>  - Uncheck mochitest-jetpack
> 
> Expected Result:
>  - Computed Command and Computed Syntax will display a command that will run
> all mochitests except for jetpack
> 
> Actual Result:
>  - Computed Command and Computed Syntax display "(NO JOBS CHOSEN)"

IMO this is expected if mochitest-browser-screenshots is checked. Simply uncheck mochitest-browser-screenshots to get the result you desire. If you want we could also add the attention class to the mochitest-browser-screenshots label so it's more obvious but I would prefer to leave the validation.

Comment 6

[Robin Steuber (they/them) [:bytesized]]

(In reply to Matthew N. [:MattN] (PTO Jun. 29 - Jul. 1) from comment #5)
> IMO this is expected if mochitest-browser-screenshots is checked. Simply
> uncheck mochitest-browser-screenshots to get the result you desire.

I don't know exactly what the mochitest-browser-screenshots test is. However, I was very surprised to see "NO JOBS CHOSEN", when there ARE jobs chosen. If the combination of tests I picked is invalid, perhaps the error message should say that instead.

Comment 7

[Andrei Obreja [:aobreja NOT AVAILABLE][:buildduty]]

Attachment: patch_bug1283282_tools_nodefault.patch

This patch can also be added so when selecting ALL test suites, mochitest-browser-screenshots will not be selected by default.

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8768687 [details] [diff] [review]
patch_bug1283282_tools_nodefault.patch

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

> This patch can also be added so when selecting ALL test suites, mochitest-browser-screenshots will not be selected by default.

That wouldn't reflect reality though so I don't see why that would be a good idea. I think a better solution is comment 5:
> If you want we could also add the attention class to the mochitest-browser-screenshots label so it's more obvious.

Comment 9

[Andrei Obreja [:aobreja NOT AVAILABLE][:buildduty]]

Attachment: bug1283282_tools_attention1.patch

This patch puts mochitest-browser-screenshots test into attention class.

Comment 10

[Andrei Obreja [:aobreja NOT AVAILABLE][:buildduty]]

Attachment: bug1283282_tools_attention2.patch

This patch puts the message for test mochitest-browser-screenshots into attension class.

Comment 11

[Andrei Obreja [:aobreja NOT AVAILABLE][:buildduty]]

Matthew please chose which patch should we use.

Comment 12

[Matthew N. [:MattN]]

Attachment: Bug 1283282 - Make it more clear that MOZSCREENSHOTS_SETS is required for screenshots.

Review commit: https://reviewboard.mozilla.org/r/65362/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65362/

Comment 13

[Matthew N. [:MattN]]

See my patch for what I had in mind. It only draws attention when necessary.

Comment 14

[Andrei Obreja [:aobreja NOT AVAILABLE][:buildduty]]

Comment on attachment 8772610 [details]
Bug 1283282 - Make it more clear that MOZSCREENSHOTS_SETS is required for screenshots.

Looks good to me.

Comment 15

[Matthew N. [:MattN]]

https://hg.mozilla.org/build/tools/rev/fb5a8d8f73a0