1581243 - Various extension related test failures on 2019-09-14

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

Mochitest/bct:
TEST-UNEXPECTED-FAIL | comm/mail/components/extensions/test/browser/browser_ext_commands_onCommand.js | Uncaught exception - startup failed
TEST-UNEXPECTED-FAIL | comm/mail/components/extensions/test/browser/browser_ext_commands_onCommand.js | A promise chain failed to handle a rejection: Warning processing commands.valid-command-with-unrecognized-property-name.unrecognized_property: An unexpected property was found in the WebExtension manifest. - stack: logWarning@resource://gre/modules/Schemas.jsm:1224:17
TEST-UNEXPECTED-FAIL | comm/mail/components/extensions/test/browser/browser_ext_commands_onCommand.js | Found an unexpected mail window at the end of test run -
TEST-UNEXPECTED-FAIL | comm/mail/components/extensions/test/browser/browser_ext_quickFilter.js | Test timed out -
TEST-UNEXPECTED-FAIL | comm/mail/components/extensions/test/browser/browser_ext_quickFilter.js | Extension left running at test shutdown -
TEST-UNEXPECTED-FAIL | comm/mail/components/extensions/test/browser/browser_ext_quickFilter.js | A promise chain failed to handle a rejection: Warning processing starred: This property is deprecated - stack: background@moz-extension://95752a7c-6bec-d845-9fa0-c2283a342b06/%7Bb486312e-749c-1e4a-b719-291d397f090a%7D.js:17:22

Xpcshell:
TEST-UNEXPECTED-FAIL | comm/mail/components/extensions/test/xpcshell/test_ext_cloudFile.js | xpcshell return code: 0
TEST-UNEXPECTED-TIMEOUT | comm/mail/components/extensions/test/xpcshell/test_ext_alias.js | Test timed out
TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_sideloads.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_sideloads.js | test_sideloading - [test_sideloading : 61] Got the correct sideload add-ons - ["[e-mail]","[e-mail]"] deepEqual ["[e-mail]","[e-mail]","[e-mail]"]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6a36994d2e14869283ea43331eeeaedac5&tochange=598d441e4ebaa93ab098d266035a396057

Could be relate to bug 1570715.

Comment 1

[Jorg K (CEST = GMT+2)]

Running
mach xpcshell-test comm/mail/components/extensions/test/xpcshell/test_ext_cloudFile.js
locally I see:
0:02.40 INFO "CONSOLE_MESSAGE: (info) 1568445220074 addons.webextension.cloudfile@xpcshell WARN Loading extension 'cloudfile@xpcshell': Reading manifest: Warning processing cloud_file.settings_url: This property is deprecated"
0:02.40 INFO "CONSOLE_MESSAGE: (info) Treating warning as error because the preference extensions.webextensions.warnings-as-errors is set to true"

Maybe setting that pref will help:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=64b4791e5bfabfd98af7022896ef0a2984ccc824

Comment 2

[Jorg K (CEST = GMT+2)]

That didn't help. I also saw:
0:02.38 pid:10812 Extension error: Warning processing cloud_file.settings_url: This property is deprecated resource://gre/modules/Schemas.jsm:1224 :: logWarning@resource://gre/modules/Schemas.jsm:1224:17

Removing that seems to help.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1581243.patch - fixes test_ext_cloudFile.js

This seems to be a small part of what needs to be done.

mach xpcshell-test comm/mail/components/extensions/test/xpcshell/test_ext_alias.js
gives:
JavaScript Error: "Error: proxy.register has been deprecated and will be removed in Firefox 71."
And that's used in the file. Looks like the time has come.

That leaves toolkit's test_sideloads.js and our browser_ext_commands_onCommand.js and browser_ext_quickFilter.js to investigate.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1581243-pref.patch

OK setting the pref in the failing tests makes those pass. That will get us through the weekend ;-)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5b0b472c4a7a
set pref extensions.webextensions.warnings-as-errors in a few tests. rs=bustage-fix

Comment 6

[Jorg K (CEST = GMT+2)]

Oops, messed up linting. Damn, the line is one character too long.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dbe658106fe7
Follow-up: fix linting issues. rs=white-space-only DONTBUILD

Comment 8

[Jorg K (CEST = GMT+2)]

Note that on today's Daily, the X4 has turned green again since bug 1570715 was backed out. But it will reland ...

Comment 9

[Rob Wu [:robwu]]

I suggest to fix the issue using the dedicated ExtensionTestUtils.failOnSchemaWarnings helper, as shown in this commit: https://hg.mozilla.org/mozilla-central/rev/e7364a10a663

That helper ensures that the pref is correctly returned to its original value if somehow the test fails.
In xpcshell tests, setting the test directly would not matter that much, because each test runs in its own process.
Browser tests, however, share the same environment. So if you set Services.prefs.setBoolPref("extensions.webextensions.warnings-as-errors", false); at the top of the test, as done in https://hg.mozilla.org/comm-central/rev/5b0b472c4a7a, then the pref does not only apply to that file, but also all other tests that run after it (which results in hard-to-debug test failures).

The feature and preference was added to ensure that tests fail (so someone takes a look) when deprecated APIs or misspelled manifest properties are used.
To take maximum advantage of this feature for code health, the warnings should only be suppressed if absolutely necessary, preferably limited to the lines where the exception is needed.

When deprecated/unknown properties are knowingly used in manifest.json, and you want to suppress the warning, use the following fix:

// Add the first and the last line around extension.startup() in test code.
ExtensionTestUtils.failOnSchemaWarnings(false);
await extension.startup();
ExtensionTestUtils.failOnSchemaWarnings(false);

If you want to use deprecated APIs in the test files, call ExtensionTestUtils.failOnSchemaWarnings(false); before startup (or at least before the test uses the deprecated API), and ExtensionTestUtils.failOnSchemaWarnings(true); as soon as the test stops using the deprecated API (e.g. at test completion).

For more examples of how this pref / test helper should be used, see https://hg.mozilla.org/mozilla-central/rev/e7364a10a663

Comment 10

[Jorg K (CEST = GMT+2)]

Thanks for the comment, Rob. I just did some "emergency surgery" to get the tests green again. We'll fix it properly soon.

We also have to investigate why toolkit/mozapps/extensions/test/xpcshell/test_sideloads.js fails in our environment.

Comment 11

[Geoff Lankow (:darktrojan)]

Attachment: 1581243-schema-warnings-1.diff

Silences the errors in the recommended way.

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9092939 [details] [diff] [review]
1581243-schema-warnings-1.diff

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

::: mail/components/extensions/test/xpcshell/test_ext_cloudFile.js
@@ +268,5 @@
>      let account = cloudFileAccounts.getAccount(id);
>      account.deleteFile(uploads.cloudFile1.id);
>    });
>  
> +  // Deprecated property "settings_url".

Shouldn't we remove it then?
https://searchfox.org/comm-central/rev/45e763ae702e00168ffc2e8287315c2d1e5efffd/mail/components/extensions/schemas/cloudFile.json#36

Same question for the other ones.

Comment 13

[Jorg K (CEST = GMT+2)]

Thanks. And what about the failures in toolkit/mozapps/extensions/test/xpcshell/test_sideloads.js? Move that to another bug?

Comment 14

[Geoff Lankow (:darktrojan)]

Truth be told I forgot about it, but it's failing because it creates an extension with the "history" permission, which is one we don't have. Rob, do you think we could change that to "tabs", or something else?

Comment 15

[Geoff Lankow (:darktrojan)]

(In reply to Magnus Melin [:mkmelin] from comment #12)

Comment on attachment 9092939 [details] [diff] [review]
1581243-schema-warnings-1.diff

Review of attachment 9092939 [details] [diff] [review]:

::: mail/components/extensions/test/xpcshell/test_ext_cloudFile.js
@@ +268,5 @@

 let account = cloudFileAccounts.getAccount(id);
 account.deleteFile(uploads.cloudFile1.id);

});

• // Deprecated property "settings_url".

Shouldn't we remove it then?
https://searchfox.org/comm-central/rev/
45e763ae702e00168ffc2e8287315c2d1e5efffd/mail/components/extensions/schemas/
cloudFile.json#36

Same question for the other ones.

Well yes, but we're here to fix a failing test, not to decide if we should continue to support old properties.

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 9092939 [details] [diff] [review]
1581243-schema-warnings-1.diff

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

Alright, fix the test. But for each case, please add a comment about it and file a bug to fix the problem (i.e. remove old properties).

Comment 17

[Rob Wu [:robwu]]

(In reply to Geoff Lankow (:darktrojan) from comment #14)

Truth be told I forgot about it, but it's failing because it creates an extension with the "history" permission, which is one we don't have. Rob, do you think we could change that to "tabs", or something else?

The test itself does not use the "history" API, so I guess that any other permission with a warning would work equally well. This includes the "tabs" permission.

Comment 18

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1581243 - Change extension permissions in test_sideloads.js so that it does not fail in Thunderbird; r?robwu

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dfd083fa9a4d
Change extension permissions in test_sideloads.js so that it does not fail in Thunderbird; r=robwu

Comment 20

[Geoff Lankow (:darktrojan)]

Attachment: 1581243-schema-warnings-2.diff

Comment 21

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/dfd083fa9a4d

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f44f040f0192
Follow-up: Improve bustage fix for WebExtensions schema warnings-as-errors (bug 1570715); r=mkmelin

Comment 23

[Jorg K (CEST = GMT+2)]

Sigh, that shouldn't have been landed before bug 1570715 since that supplied the ExtensionTestUtils.failOnSchemaWarnings() function. So the tests where we stuck this in are now failing :-( - Anyway, that bug is on autoland, so it should be OK soon.

Comment 24

[Jorg K (CEST = GMT+2)]

Bug 1570715 has landed now. mail/components/extensions/test/xpcshell/test_ext_alias.js is still failing :-( - I guess ExtensionTestUtils.failOnSchemaWarnings(false); doesn't deal with the proxy.register deprecation, so we'd need to return to my crude way of fixing the error.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8ddd3b92ac4f
Follow-up: Reinstate setting extensions.webextensions.warnings-as-errors in test_ext_alias.js. rs=bustage-fix DONTBUILD

Comment 26

[Jorg K (CEST = GMT+2)]

You're going to file a bug for the proxy stuff?

Comment 27

[Rob Wu [:robwu]]

The helper function does correctly deal with deprecations; you need to move the second call after awaitFinish, so that the exception is effective while the test's background script is executing.

The helper sets the pref, and emsures that its value is restored at the end of the test. It's not possible for setting the pref directly to be more effective than using the helper.

Comment 28

[Jorg K (CEST = GMT+2)]

Comment on attachment 9092984 [details] [diff] [review]
1581243-schema-warnings-2.diff

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

::: mail/components/extensions/test/xpcshell/test_ext_alias.js
@@ +136,5 @@
>    });
>  
> +  // Deprecated proxy functions and events. This test will fail once
> +  // bug 1545811 is fixed, and the proxy parts should be removed then.
> +  // ExtensionTestUtils.failOnSchemaWarnings(false);

Hmm, this was added as a comment :-( - No wonder it didn't work.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d3543c7f1207
Revert changeset 8ddd3b92ac4f but place ExtensionTestUtils.failOnSchemaWarnings(true) later. r=me DONTBUILD

Comment 30

[Geoff Lankow (:darktrojan)]

Gah, dammit, I must've left that behind after checking something.

I'll file a bug and get rid of that stuff now because there's little point in keeping it.