Closed Bug 1687932 Opened 3 years ago Closed 3 years ago

Replace addEngineWithDetails in xpcshell-tests

Categories

(Firefox :: Search, task, P3)

Product:
Firefox
Component:
Search
Type:
task
Points:
5

Tracking

()

Status:
RESOLVED FIXED
Milestone:
88 Branch
Iteration:
88.2 - Mar 8 - Mar 21
Tracking Flags:
Tracking Status
firefox88 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Whiteboard: [lang=js])

Attachments

(4 files)

Bug 1687932 - Improve test_webextensions_migrate_to.js to avoid potentially triggering installation routines that we do not want for the test. r?daleharvey!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1687932 - Simplify use of SearchTestUtils.installSearchExtension for xpcshell-tests and clean up the search tests. r?daleharvey!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1687932 - Remove use of nsISearchEngine.addEngineWithDetails from search xpcshell-tests. r?daleharvey!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1687932 - Remove use of nsISearchEngine.addEngineWithDetails from other xpcshell-tests. r?harry!
48 bytes, text/x-phabricator-request
Details | Review

I'm willing to mentor this bug, but if it is your first time here then I suggest you do a good-first-bug first - find them via Codetribute.

We're working on removing addEngineWithDetails (bug 1649186). As part of that, we want to replace the calls in tests with a newer test specific function.

For this bug, we'll concentrate on the instances in toolkit/components/search/tests/xpcshell/

Note: the tests beginning with test_addEngineWith can be ignored since they are testing the actual function, and hence will be removed later.

Here's an example of the new code that uses SearchTestUtils.installSearchExtension: https://searchfox.org/mozilla-central/rev/1ebc9745be02eebf7a694f5c527a44a045b8c97a/toolkit/components/search/tests/xpcshell/test_notifications.js#103-106,112

SearchTestUtils.installSearchExtension has various options that are described here: https://searchfox.org/mozilla-central/rev/1ebc9745be02eebf7a694f5c527a44a045b8c97a/toolkit/components/search/tests/SearchTestUtils.jsm#223-247

The extension must be unloaded at the end of the test. If the test covers several add_task functions, then it can be unloaded in a separate add_task at the end, e.g.

add_task(async function cleanup() {
  extension.unload();
});

Hi Mark,
I want to solve this bug.

Thanks
Abhishek

HI Abhishek, sure please go ahead.

Assignee: nobody → mick.sharma02

Thanks Mark

Hi mark,
Can you explain it. I open both the files.

For this bug, we'll concentrate on the instances in toolkit/components/search/tests/xpcshell/

The instances listed here in test_* files are the ones you need to fix. You can skip the test_addEngineWith* files as they'll be removed later.

You need to replace the calls to addEngineWithDetails with similar calls to those which I highlighted in the test_notifications.js file, i.e. SearchTestUtils.installSearchExtension, await extension.awaitStartup(); and await extension.unload(); at the end of the test. For the unload you need to take into account this:

The extension must be unloaded at the end of the test. If the test covers several add_task functions, then it can be unloaded in a separate add_task at the end, e.g.

add_task(async function cleanup() {
  extension.unload();
});

I forgot to say, once built, you can run the tests with:

./mach xpcshell-test path/to/test

You shouldn't need to re-build when changing the test files.

Also check ./mach eslint toolkit/components/search passes before you submit patches.

Please also skip the toolkit/components/search/tests/xpcshell/test_migrateWebExtensionEngine.js test. I've just noticed that test needs a different mechanism as it is not currently testing the right thing. Since I'm handling that in another bug, there's no point in you changing it here as well.

ok

Abhishek

Hi Abhishek, are you still working on this?

Flags: needinfo?(mick.sharma02)

Yes I am active on this bug. Today I submit my patch.

Abhishek

Flags: needinfo?(mick.sharma02)

Hi Abhishek, thank you for confirming. Just to let you know I'm probably going to need this bug fixed sometime in the next 2 to 4 weeks. We've some changes coming and it'd be easier to have this landed to be able to make those. I don't want to put pressure on you though, we can find alternate bugs if you need.

Unfortunately I've not heard back from Abhishek, and I need to progress this bug, so I'm going to take it on.

Abhishek: if you want more bugs like this in future let me know and I'll be quite happy to find you something.

Assignee: mick.sharma02 → standard8
Iteration: --- → 88.2 - Mar 8 - Mar 21
Points: --- → 5
Priority: -- → P3

Also remove addEngineWithDetails from test_TelemetryEnvironment_search.js to help resolve issues following the cleaning up SearchTestUtils.jsm.

Depends on D108459

Updated try push: https://treeherder.mozilla.org/jobs?repo=try&revision=1466a913f87d8014792ce323a2ae4a7d8aac1c15

Mentor: standard8
Summary: Replace addEngineWithDetails in toolkit/components/search/tests/xpcshell → Replace addEngineWithDetails in xpcshell-tests
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/33a8c6623c60
Improve test_webextensions_migrate_to.js to avoid potentially triggering installation routines that we do not want for the test. r=daleharvey
https://hg.mozilla.org/integration/autoland/rev/cfa47554d8c5
Simplify use of SearchTestUtils.installSearchExtension for xpcshell-tests and clean up the search tests. r=daleharvey
https://hg.mozilla.org/integration/autoland/rev/8f5707df7bad
Remove use of nsISearchEngine.addEngineWithDetails from search xpcshell-tests. r=daleharvey
https://hg.mozilla.org/integration/autoland/rev/838ed2321e07
Remove use of nsISearchEngine.addEngineWithDetails from other xpcshell-tests. r=harry
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: