1491436 - Create regression tests for preferred application selection in preferences/options

Status: RESOLVED FIXED

(Firefox :: Settings UI, enhancement, P1)

Description

[:Gijs (he/him)]

The applications settings are undermaintained - they've kind of always been there and we've fixed things in them when we've broken them, but I would go so far as to say that very few people understand all of the code that governs that list, the editable items, and how that all works.

We broke this when doing some routine refactoring of enumerators because the code is highly magical. We didn't notice because there is no (or at least not sufficient) test coverage. That should change.

Comment 1

[:Gijs (he/him)]

I'll try biting this bullet and down-prio'ing if it proves too messy.

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1491436 - remove use of feeds from application selection test, and check the dropdown actually works, r?paolo

Comment 3

[:Gijs (he/him)]

This try is green at time of writing, though macOS is still pending:

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

Comment 4

[:Gijs (he/him)]

I did run into some issues with `alwaysAsk` not being set on the handler info returned by the external protocol service (or the handler service). I can reproduce that when testing manually on my "normal" beta profile, though even with the value being "wrong" (set to 2, ie useDefault), the alwaysAsk behavior does happen when I try to open mailto links.

Comment 5

[:Paolo Amadini]

Comment on attachment 9011132 [details]
Bug 1491436 - remove use of feeds from application selection test, and check the dropdown actually works, r?paolo

:Paolo Amadini has approved the revision.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/834bfb49df9a
remove use of feeds from application selection test, and check the dropdown actually works, r=paolo

Comment 7

[Noemi Erli[:noemi_erli]]

Backed out changeset 834bfb49df9a (Bug 1491436) for failing in browser_applications_selection.js CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed,busted,exception&classifiedState=unclassified&revision=834bfb49df9a0950866ffcfc464604f2490ca0cb&selectedJob=201671271

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=201671271&repo=autoland&lineNumber=1151

Backout: https://hg.mozilla.org/integration/autoland/rev/f41fce9645d1360b46f38c9109b48f55f47828b0

Comment 8

[:Gijs (he/him)]

(In reply to Noemi Erli[:noemi_erli] from comment #7)
> Backed out changeset 834bfb49df9a (Bug 1491436) for failing in
> browser_applications_selection.js CLOSED TREE
> 
> Push with failures:
> https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed,
> busted,
> exception&classifiedState=unclassified&revision=834bfb49df9a0950866ffcfc46460
> 4f2490ca0cb&selectedJob=201671271
> 
> Failure log:
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=201671271&repo=autoland&lineNumber=1151
> 
> Backout:
> https://hg.mozilla.org/integration/autoland/rev/
> f41fce9645d1360b46f38c9109b48f55f47828b0

Turns out there's a subtle bug in the test:

  // select one of our test cases:
  list.selectedItem = list.querySelector("menuitem[label='Handler 1']");
  let {preferredAction, alwaysAskBeforeHandling} = HandlerServiceTestUtils.getHandlerInfo(itemType);
  Assert.notEqual(preferredAction, Ci.nsIHandlerInfo.alwaysAsk,
      "Should have selected something other than 'always ask' (" + itemType + ")");

The querySelector call returns null. This makes the selection a no-op, or at least, the result depends on the OS / previous option selected. Unfortunately, all my trypushes excluded win7 (win32) and therefore missed that on that OS only, mailto defaults are (apparently) different, and as a result, the test behaves differently. I would have noticed if I'd used the new helper I made in the end, because it uses `element.click()`, so passing it a null element breaks the test.

I changed the test as follows:

-  list.selectedItem = list.querySelector("menuitem[label='Handler 1']");
+  let handlerItem = list.querySelector("menuitem[label*='Handler 1']");
+  await selectItemInPopup(handlerItem);

and that seems to work locally. I pushed to try again: 

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

Comment 9

[:Gijs (he/him)]

(In reply to :Gijs (out Thu 27 - Sun 30 / 9;  he/him) from comment #8)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=94078f2276ea84691d5730ea232448dd15224ea8

try is greener than our lawn in spring, so relanding. :-)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23841605d322
remove use of feeds from application selection test, and check the dropdown actually works, r=paolo

Comment 11

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/23841605d322