Closed Bug 1581243 Opened 1 year ago Closed 5 months ago

Various extension related test failures on 2019-09-14

Categories

(Thunderbird :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 71.0

People

(Reporter: jorgk-bmo, Assigned: darktrojan)

References

(Regression)

Details

Attachments

(4 files, 1 obsolete file)

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 - ["addon2@tests.mozilla.org","addon3@tests.mozilla.org"] deepEqual ["addon1@tests.mozilla.org","addon2@tests.mozilla.org","addon3@tests.mozilla.org"]

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

Could be relate to bug 1570715.

Flags: needinfo?(geoff)

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

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.

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.

Assignee: nobody → jorgk
Assignee: jorgk → nobody

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

Assignee: nobody → jorgk
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5b0b472c4a7a
set pref extensions.webextensions.warnings-as-errors in a few tests. rs=bustage-fix

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

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/dbe658106fe7
Follow-up: fix linting issues. rs=white-space-only DONTBUILD

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

Regressed by: 1570715

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

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.

Attached patch 1581243-schema-warnings-1.diff (obsolete) — Splinter Review

Silences the errors in the recommended way.

Assignee: jorgk → geoff
Status: NEW → ASSIGNED
Flags: needinfo?(geoff)
Attachment #9092939 - Flags: review?(mkmelin+mozilla)
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.

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

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?

Flags: needinfo?(rob)

(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 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).
Attachment #9092939 - Flags: review?(mkmelin+mozilla) → review+

(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.

Flags: needinfo?(rob)
Pushed by geoff@darktrojan.net:
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
See Also: → 1581496
See Also: → 1581498
Attachment #9092939 - Attachment is obsolete: true
Attachment #9092984 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f44f040f0192
Follow-up: Improve bustage fix for WebExtensions schema warnings-as-errors (bug 1570715); r=mkmelin

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.

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.

Pushed by mozilla@jorgk.com:
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

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

Flags: needinfo?(geoff)

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 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.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d3543c7f1207
Revert changeset 8ddd3b92ac4f but place ExtensionTestUtils.failOnSchemaWarnings(true) later. r=me DONTBUILD

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.

Flags: needinfo?(geoff)
See Also: → 1581760
You need to log in before you can comment on or make changes to this bug.