1701950 - Replace addTestEngines and installTestEngine in search tests with SearchTestUtils.promiseNewSearchEngine

Status: RESOLVED FIXED

(Firefox :: Search, task)

Description

[Mark Banner (:standard8)]

In head_search.js we currently have addTestEngines and installTestEngine.

These can now be replaced by the newer SearchTestUtils.promiseNewSearchEngine().

Here are links to where these are currently used in tests: addTestEngines, installTestEngine.

The addObserver mechanism in addTestEngines is no longer required - promiseNewSearchEngine has the same effect overall, just simpler parameters.

You can run the tests after an initial build with ./mach xpcshell-test toolkit/components/search --tag=searchmain. In this case the --tag avoids running some long duration configuration tests which aren't going to be affected by these changes.

Comment 1

[Loek]

I've had a look at addTestEngines and promiseNewSearchEngine and the first assert in one of the test files in which addTestEngines should be replaced.

I noticed that the asserts in the test file use what addTestEngines returns from the addObserver mechanism, so if I replaceaddTestEngines with promiseNewSearchEngine that assert doesn't work anymore. Adding .name to both variables works, however that won't compare all properties of the object. Is using .name sufficient, or is there a wrapper to compare them properly?

Comment 2

[Mark Banner (:standard8)]

(In reply to Loek from comment #1)

I noticed that the asserts in the test file use what addTestEngines returns from the addObserver mechanism, so if I replaceaddTestEngines with promiseNewSearchEngine that assert doesn't work anymore. Adding .name to both variables works, however that won't compare all properties of the object. Is using .name sufficient, or is there a wrapper to compare them properly?

I didn't put it in my original message, but I assume you're using something like this as a replacement:

  let engine1 = await SearchTestUtils.promiseNewSearchEngine(gDataUrl + "engine.xml", null);
  let engine2 = await SearchTestUtils.promiseNewSearchEngine(gDataUrl + "engine2.xml", null);

If so, then in this case, I think we want to go with using these for the following lines:

  Assert.equal((await promise).wrappedJSObject, engine1);
  Assert.equal(search.defaultEngine.wrappedJSObject, engine1);

Sometimes we do just check the name, but I think for this case we should be checking the object.

The wrappedJSObject property is a way of getting the JavaScript object that's behind an XPCOM interface. In the call to promiseNewSearchEngine we get that JavaScript object returned directly, but the observers return the XPCOM interface object.

Comment 3

[Loek]

Attachment: Bug 1701950 - Replace addTestEngines with promiseNewSearchEngine in search xpcshell tests. r=standard8

Comment 4

[Loek]

Attachment: Bug 1701950 - Replace installTestEngine with promiseNewSearchEngine in search xpcshell tests. r=standard8

Depends on D110315

Comment 5

[Loek]

Thanks, .wrappedJSObject was what I was looking for!

I made the changes, tested locally to make sure they worked and submitted them. I don't have TryServer commit access, so can you handle it from here or should I still do something else?

Comment 6

[Mark Banner (:standard8)]

(In reply to Loek from comment #5)

I made the changes, tested locally to make sure they worked and submitted them. I don't have TryServer commit access, so can you handle it from here or should I still do something else?

Great. For this bug, we don't really need a try server - the only items we're changing are specific tests, and I also know that there isn't anything that's particularly platform dependent on these. Hence running them locally is fine.

Comment 7

[Loek]

Attachment: Bug 1701950 - Remove unused addTestEngines and installTestEngine. Move useHttpServer to setup function. r=standard8

Depends on D110316

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9f90f337c240
Replace addTestEngines with promiseNewSearchEngine in search xpcshell tests. r=Standard8
https://hg.mozilla.org/integration/autoland/rev/22cf7d2865ba
Replace installTestEngine with promiseNewSearchEngine in search xpcshell tests. r=Standard8

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/9f90f337c240
https://hg.mozilla.org/mozilla-central/rev/22cf7d2865ba