diff options
Diffstat (limited to 'browser/components/urlbar/tests/quicksuggest/unit')
8 files changed, 277 insertions, 283 deletions
diff --git a/browser/components/urlbar/tests/quicksuggest/unit/head.js b/browser/components/urlbar/tests/quicksuggest/unit/head.js index c468e4526f..73bedf468e 100644 --- a/browser/components/urlbar/tests/quicksuggest/unit/head.js +++ b/browser/components/urlbar/tests/quicksuggest/unit/head.js @@ -20,18 +20,49 @@ add_setup(async function setUpQuickSuggestXpcshellTest() { UrlbarPrefs._testSkipTelemetryEnvironmentInit = true; }); +let gAddTasksWithRustSetup; + /** * Adds two tasks: One with the Rust backend disabled and one with it enabled. * The names of the task functions will be the name of the passed-in task * function appended with "_rustDisabled" and "_rustEnabled". If the passed-in - * task doesn't have a name, "anonymousTask" will be used. Call this with the - * usual `add_task()` arguments. + * task doesn't have a name, "anonymousTask" will be used. + * + * Call this with the usual `add_task()` arguments. Additionally, an object with + * the following properties can be specified as any argument: + * + * {boolean} skip_if_rust_enabled + * If true, a "_rustEnabled" task won't be added. Useful when Rust is enabled + * by default but the task doesn't make sense with Rust and you still want to + * test some behavior when Rust is disabled. + * + * @param {...any} args + * The usual `add_task()` arguments. */ function add_tasks_with_rust(...args) { + let skipIfRustEnabled = false; + let i = args.findIndex(a => a.skip_if_rust_enabled); + if (i >= 0) { + skipIfRustEnabled = true; + args.splice(i, 1); + } + let taskFnIndex = args.findIndex(a => typeof a == "function"); let taskFn = args[taskFnIndex]; for (let rustEnabled of [false, true]) { + let newTaskName = + (taskFn.name || "anonymousTask") + + (rustEnabled ? "_rustEnabled" : "_rustDisabled"); + + if (rustEnabled && skipIfRustEnabled) { + info( + "add_tasks_with_rust: Skipping due to skip_if_rust_enabled: " + + newTaskName + ); + continue; + } + let newTaskFn = async (...taskFnArgs) => { info("add_tasks_with_rust: Setting rustEnabled: " + rustEnabled); UrlbarPrefs.set("quicksuggest.rustEnabled", rustEnabled); @@ -42,6 +73,12 @@ function add_tasks_with_rust(...args) { await QuickSuggestTestUtils.forceSync(); info("add_tasks_with_rust: Done forcing sync"); + if (gAddTasksWithRustSetup) { + info("add_tasks_with_rust: Calling setup function"); + await gAddTasksWithRustSetup(); + info("add_tasks_with_rust: Done calling setup function"); + } + let rv; try { info( @@ -77,18 +114,36 @@ function add_tasks_with_rust(...args) { return rv; }; - Object.defineProperty(newTaskFn, "name", { - value: - (taskFn.name || "anonymousTask") + - (rustEnabled ? "_rustEnabled" : "_rustDisabled"), - }); - let addTaskArgs = [...args]; - addTaskArgs[taskFnIndex] = newTaskFn; + Object.defineProperty(newTaskFn, "name", { value: newTaskName }); + + let addTaskArgs = []; + for (let j = 0; j < args.length; j++) { + addTaskArgs[j] = + j == taskFnIndex + ? newTaskFn + : Cu.cloneInto(args[j], this, { cloneFunctions: true }); + } add_task(...addTaskArgs); } } /** + * Registers a setup function that `add_tasks_with_rust()` will await before + * calling each of your original tasks. Call this at most once in your test file + * (i.e., in `add_setup()`). This is useful when enabling/disabling Rust has + * side effects related to your particular test that need to be handled or + * awaited for each of your tasks. On the other hand, if only one or two of your + * tasks need special setup, do it directly in those tasks instead of using + * this. + * + * @param {Function} setupFn + * A function that will be awaited before your original tasks are called. + */ +function registerAddTasksWithRustSetup(setupFn) { + gAddTasksWithRustSetup = setupFn; +} + +/** * Returns an expected Wikipedia (non-sponsored) result that can be passed to * `check_results()` regardless of whether the Rust backend is enabled. * @@ -106,7 +161,7 @@ function makeWikipediaResult({ iconBlob = new Blob([new Uint8Array([])]), impressionUrl = "http://example.com/wikipedia-impression", clickUrl = "http://example.com/wikipedia-click", - blockId = 1, + blockId = 2, advertiser = "Wikipedia", iabCategory = "5 - Education", suggestedIndex = -1, @@ -362,7 +417,6 @@ function makeWeatherResult({ temperatureUnit, url: MerinoTestUtils.WEATHER_SUGGESTION.url, iconId: "6", - helpUrl: QuickSuggest.HELP_URL, requestId: MerinoTestUtils.server.response.body.request_id, source: "merino", provider: "accuweather", @@ -909,3 +963,40 @@ async function doRustProvidersTests({ searchString, tests }) { UrlbarPrefs.clear("quicksuggest.rustEnabled"); await QuickSuggestTestUtils.forceSync(); } + +/** + * Waits for `Weather` to start and finish a new fetch. Typically you call this + * before you know a new fetch will start, save but don't await the promise, do + * the thing that triggers the new fetch, and then await the promise to wait for + * the fetch to finish. + * + * If a fetch is currently ongoing, this will first wait for a new fetch to + * start, which might not be what you want. If you only want to wait for any + * ongoing fetch to finish, await `QuickSuggest.weather.fetchPromise` instead. + */ +async function waitForNewWeatherFetch() { + let { fetchPromise: oldFetchPromise } = QuickSuggest.weather; + + // Wait for a new fetch to start. + let newFetchPromise; + await TestUtils.waitForCondition(() => { + let { fetchPromise } = QuickSuggest.weather; + if ( + (oldFetchPromise && fetchPromise != oldFetchPromise) || + (!oldFetchPromise && fetchPromise) + ) { + newFetchPromise = fetchPromise; + return true; + } + return false; + }, "Waiting for a new weather fetch to start"); + + Assert.equal( + QuickSuggest.weather.fetchPromise, + newFetchPromise, + "Sanity check: fetchPromise hasn't changed since waitForCondition returned" + ); + + // Wait for the new fetch to finish. + await newFetchPromise; +} diff --git a/browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest.js b/browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest.js index e4c145aabb..59c0b26241 100644 --- a/browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest.js +++ b/browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest.js @@ -18,6 +18,8 @@ const HTTP_SEARCH_STRING = "http prefix"; const HTTPS_SEARCH_STRING = "https prefix"; const PREFIX_SUGGESTIONS_STRIPPED_URL = "example.com/prefix-test"; +const ONE_CHAR_SEARCH_STRINGS = ["x", "x ", " x", " x "]; + const { TIMESTAMP_TEMPLATE, TIMESTAMP_LENGTH } = QuickSuggest; const TIMESTAMP_SEARCH_STRING = "timestamp"; const TIMESTAMP_SUGGESTION_URL = `http://example.com/timestamp-${TIMESTAMP_TEMPLATE}`; @@ -69,6 +71,11 @@ const REMOTE_SETTINGS_RESULTS = [ iab_category: "22 - Shopping", icon: "1234", }, + QuickSuggestTestUtils.ampRemoteSettings({ + keywords: [...ONE_CHAR_SEARCH_STRINGS, "12", "a longer keyword"], + title: "Suggestion with 1-char keyword", + url: "http://example.com/1-char-keyword", + }), ]; function expectedNonSponsoredResult() { @@ -751,10 +758,10 @@ async function doDedupeAgainstURLTest({ } // Tests the remote settings latency histogram. -add_task( +add_tasks_with_rust( { // Not supported by the Rust backend. - skip_if: () => UrlbarPrefs.get("quickSuggestRustEnabled"), + skip_if_rust_enabled: true, }, async function latencyTelemetry() { UrlbarPrefs.set("suggest.quicksuggest.nonsponsored", true); @@ -791,10 +798,10 @@ add_task( // Tests setup and teardown of the remote settings client depending on whether // quick suggest is enabled. -add_task( +add_tasks_with_rust( { // Not supported by the Rust backend. - skip_if: () => UrlbarPrefs.get("quickSuggestRustEnabled"), + skip_if_rust_enabled: true, }, async function setupAndTeardown() { Assert.ok( @@ -885,7 +892,7 @@ add_task( "Remote settings backend is disabled after enabling the Rust backend" ); - UrlbarPrefs.clear("quicksuggest.rustEnabled"); + UrlbarPrefs.set("quicksuggest.rustEnabled", false); Assert.ok( QuickSuggest.jsBackend.rs, "Settings client is non-null after disabling the Rust backend" @@ -898,6 +905,7 @@ add_task( // Leave the prefs in the same state as when the task started. UrlbarPrefs.clear("suggest.quicksuggest.nonsponsored"); UrlbarPrefs.clear("suggest.quicksuggest.sponsored"); + UrlbarPrefs.clear("quicksuggest.rustEnabled"); UrlbarPrefs.set("quicksuggest.enabled", true); Assert.ok( !QuickSuggest.jsBackend.rs, @@ -1349,10 +1357,10 @@ add_tasks_with_rust(async function block_timestamp() { // Makes sure remote settings data is fetched using the correct `type` based on // the value of the `quickSuggestRemoteSettingsDataType` Nimbus variable. -add_task( +add_tasks_with_rust( { // Not supported by the Rust backend. - skip_if: () => UrlbarPrefs.get("quickSuggestRustEnabled"), + skip_if_rust_enabled: true, }, async function remoteSettingsDataType() { UrlbarPrefs.set("suggest.quicksuggest.sponsored", true); @@ -1509,7 +1517,7 @@ add_tasks_with_rust(async function tabToSearch() { // search heuristic makeSearchResult(context, { engineName: Services.search.defaultEngine.name, - engineIconUri: Services.search.defaultEngine.getIconURL(), + engineIconUri: await Services.search.defaultEngine.getIconURL(), heuristic: true, }), // tab to search @@ -1595,7 +1603,7 @@ add_tasks_with_rust(async function position() { // search heuristic makeSearchResult(context, { engineName: Services.search.defaultEngine.name, - engineIconUri: Services.search.defaultEngine.getIconURL(), + engineIconUri: await Services.search.defaultEngine.getIconURL(), heuristic: true, }), // best match whose backing suggestion has a `position` @@ -1659,3 +1667,35 @@ add_task(async function rustProviders() { ], }); }); + +// Tests the keyword/search-string-length threshold. Keywords/search strings +// must be at least two characters long to be matched. +add_tasks_with_rust(async function keywordLengthThreshold() { + UrlbarPrefs.set("suggest.quicksuggest.sponsored", true); + await QuickSuggestTestUtils.forceSync(); + + let tests = [ + ...ONE_CHAR_SEARCH_STRINGS.map(keyword => ({ keyword, expected: false })), + { keyword: "12", expected: true }, + { keyword: "a longer keyword", expected: true }, + ]; + + for (let { keyword, expected } of tests) { + await check_results({ + context: createContext(keyword, { + providers: [UrlbarProviderQuickSuggest.name], + isPrivate: false, + }), + matches: !expected + ? [] + : [ + makeAmpResult({ + keyword, + title: "Suggestion with 1-char keyword", + url: "http://example.com/1-char-keyword", + originalUrl: "http://example.com/1-char-keyword", + }), + ], + }); + } +}); diff --git a/browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest_addons.js b/browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest_addons.js index c17f3f1655..806685eff7 100644 --- a/browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest_addons.js +++ b/browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest_addons.js @@ -42,7 +42,7 @@ const REMOTE_SETTINGS_RESULTS = [ icon: "https://example.com/first-addon.svg", title: "First Addon", rating: "4.7", - keywords: ["first", "1st", "two words", "a b c"], + keywords: ["first", "1st", "two words", "aa b c"], description: "Description for the First Addon", number_of_ratings: 1256, score: 0.25, @@ -353,35 +353,35 @@ add_tasks_with_rust(async function remoteSettings() { }), }, { - input: "a", + input: "aa", expected: makeExpectedResult({ suggestion: REMOTE_SETTINGS_RESULTS[0].attachment[0], source: "remote-settings", }), }, { - input: "a ", + input: "aa ", expected: makeExpectedResult({ suggestion: REMOTE_SETTINGS_RESULTS[0].attachment[0], source: "remote-settings", }), }, { - input: "a b", + input: "aa b", expected: makeExpectedResult({ suggestion: REMOTE_SETTINGS_RESULTS[0].attachment[0], source: "remote-settings", }), }, { - input: "a b ", + input: "aa b ", expected: makeExpectedResult({ suggestion: REMOTE_SETTINGS_RESULTS[0].attachment[0], source: "remote-settings", }), }, { - input: "a b c", + input: "aa b c", expected: makeExpectedResult({ suggestion: REMOTE_SETTINGS_RESULTS[0].attachment[0], source: "remote-settings", diff --git a/browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest_pocket.js b/browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest_pocket.js index 29133a8579..79b4df2b77 100644 --- a/browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest_pocket.js +++ b/browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest_pocket.js @@ -214,9 +214,9 @@ add_tasks_with_rust(async function lowPrefixes() { // starting at "how to" instead of the first word. // // Note: The Rust implementation doesn't support this. -add_task( +add_tasks_with_rust( { - skip_if: () => UrlbarPrefs.get("quickSuggestRustEnabled"), + skip_if_rust_enabled: true, }, async function lowPrefixes_howTo() { // search string -> should match diff --git a/browser/components/urlbar/tests/quicksuggest/unit/test_rust_ingest.js b/browser/components/urlbar/tests/quicksuggest/unit/test_rust_ingest.js index e6ec61bcd4..f2e3f96c1f 100644 --- a/browser/components/urlbar/tests/quicksuggest/unit/test_rust_ingest.js +++ b/browser/components/urlbar/tests/quicksuggest/unit/test_rust_ingest.js @@ -78,11 +78,15 @@ add_task(async function firstRun() { await checkSuggestions(); - // Disable and re-enable the backend. No new ingestion should start - // immediately since this isn't the first time the backend has been enabled. + // Disable and re-enable the backend. Another ingest should start immediately + // since ingest is done every time the backend is re-enabled. UrlbarPrefs.set("quicksuggest.rustEnabled", false); UrlbarPrefs.set("quicksuggest.rustEnabled", true); - await assertNoNewIngestStarted(ingestPromise); + ({ ingestPromise } = await waitForIngestStart(ingestPromise)); + + info("Awaiting ingest promise"); + await ingestPromise; + info("Done awaiting ingest promise"); await checkSuggestions(); @@ -101,14 +105,18 @@ add_task(async function interval() { "Sanity check: Rust backend is initially disabled" ); - // Set a small interval and enable the backend. No new ingestion should start - // immediately since this isn't the first time the backend has been enabled. + // Set a small interval and enable the backend. A new ingest will immediately + // start. let intervalSecs = 1; UrlbarPrefs.set("quicksuggest.rustIngestIntervalSeconds", intervalSecs); UrlbarPrefs.set("quicksuggest.rustEnabled", true); - await assertNoNewIngestStarted(ingestPromise); + ({ ingestPromise } = await waitForIngestStart(ingestPromise)); - // Wait for a few ingests to happen. + info("Awaiting ingest promise"); + await ingestPromise; + info("Done awaiting ingest promise"); + + // Wait for a few ingests to happen due to the timer firing. for (let i = 0; i < 3; i++) { info("Preparing for ingest at index " + i); @@ -193,17 +201,6 @@ async function waitForIngestStart(oldIngestPromise) { return { ingestPromise: newIngestPromise }; } -async function assertNoNewIngestStarted(oldIngestPromise) { - for (let i = 0; i < 3; i++) { - await TestUtils.waitForTick(); - } - Assert.equal( - QuickSuggest.rustBackend.ingestPromise, - oldIngestPromise, - "No new ingest started" - ); -} - async function checkSuggestions(expected = [REMOTE_SETTINGS_SUGGESTION]) { let actual = await QuickSuggest.rustBackend.query("amp"); Assert.deepEqual( diff --git a/browser/components/urlbar/tests/quicksuggest/unit/test_weather.js b/browser/components/urlbar/tests/quicksuggest/unit/test_weather.js index 28801904a1..cd794f435b 100644 --- a/browser/components/urlbar/tests/quicksuggest/unit/test_weather.js +++ b/browser/components/urlbar/tests/quicksuggest/unit/test_weather.js @@ -34,6 +34,15 @@ add_setup(async () => { // `lastFetchTimeMs` is expected to be `fetchDelayAfterComingOnlineMs`, we can // be sure the value actually came from `fetchDelayAfterComingOnlineMs`. QuickSuggest.weather._test_fetchDelayAfterComingOnlineMs = 53; + + // When `add_tasks_with_rust()` disables the Rust backend and forces sync, the + // JS backend will sync `Weather` with remote settings. Since keywords are + // present in remote settings at that point (we added them above), `Weather` + // will then start fetching. The fetch may or may not be done before our test + // task starts. To make sure it's done, queue another fetch and await it. + registerAddTasksWithRustSetup(async () => { + await QuickSuggest.weather._test_fetch(); + }); }); // The feature should be properly uninitialized when it's disabled and then @@ -52,17 +61,15 @@ add_tasks_with_rust(async function disableAndEnable_suggestPref() { async function doBasicDisableAndEnableTest(pref) { // Sanity check initial state. - assertEnabled({ + await assertEnabled({ message: "Sanity check initial state", hasSuggestion: true, - pendingFetchCount: 0, }); // Disable the feature. It should be immediately uninitialized. UrlbarPrefs.set(pref, false); assertDisabled({ message: "After disabling", - pendingFetchCount: 0, }); // No suggestion should be returned for a search. @@ -83,19 +90,17 @@ async function doBasicDisableAndEnableTest(pref) { // Re-enable the feature. It should be immediately initialized and a fetch // should start. info("Re-enable the feature"); - let fetchPromise = QuickSuggest.weather.waitForFetches(); + let fetchPromise = waitForNewWeatherFetch(); UrlbarPrefs.set(pref, true); - assertEnabled({ + await assertEnabled({ message: "Immediately after re-enabling", hasSuggestion: false, - pendingFetchCount: 1, }); await fetchPromise; - assertEnabled({ + await assertEnabled({ message: "After awaiting fetch", hasSuggestion: true, - pendingFetchCount: 0, }); Assert.equal( @@ -126,16 +131,15 @@ async function doBasicDisableAndEnableTest(pref) { // This task is only appropriate for the JS backend, not Rust, since fetching is // always active with Rust. -add_task( +add_tasks_with_rust( { - skip_if: () => UrlbarPrefs.get("quickSuggestRustEnabled"), + skip_if_rust_enabled: true, }, async function keywordsNotDefined() { // Sanity check initial state. - assertEnabled({ + await assertEnabled({ message: "Sanity check initial state", hasSuggestion: true, - pendingFetchCount: 0, }); // Set RS data without any keywords. Fetching should immediately stop. @@ -147,7 +151,6 @@ add_task( ]); assertDisabled({ message: "After setting RS data without keywords", - pendingFetchCount: 0, }); // No suggestion should be returned for a search. @@ -162,24 +165,22 @@ add_task( // Set keywords. Fetching should immediately start. info("Setting keywords"); - let fetchPromise = QuickSuggest.weather.waitForFetches(); + let fetchPromise = waitForNewWeatherFetch(); await QuickSuggestTestUtils.setRemoteSettingsRecords([ { type: "weather", weather: MerinoTestUtils.WEATHER_RS_DATA, }, ]); - assertEnabled({ + await assertEnabled({ message: "Immediately after setting keywords", hasSuggestion: false, - pendingFetchCount: 1, }); await fetchPromise; - assertEnabled({ + await assertEnabled({ message: "After awaiting fetch", hasSuggestion: true, - pendingFetchCount: 0, }); Assert.equal( @@ -211,27 +212,24 @@ add_task( // it should be discarded since the feature is disabled. add_tasks_with_rust(async function disableAndEnable_immediate1() { // Sanity check initial state. - assertEnabled({ + await assertEnabled({ message: "Sanity check initial state", hasSuggestion: true, - pendingFetchCount: 0, }); // Disable the feature. It should be immediately uninitialized. UrlbarPrefs.set("weather.featureGate", false); assertDisabled({ message: "After disabling", - pendingFetchCount: 0, }); // Re-enable the feature. It should be immediately initialized and a fetch // should start. - let fetchPromise = QuickSuggest.weather.waitForFetches(); + let fetchPromise = waitForNewWeatherFetch(); UrlbarPrefs.set("weather.featureGate", true); - assertEnabled({ + await assertEnabled({ message: "Immediately after re-enabling", hasSuggestion: false, - pendingFetchCount: 1, }); // Disable it again. The fetch will remain ongoing since pending fetches @@ -239,7 +237,6 @@ add_tasks_with_rust(async function disableAndEnable_immediate1() { UrlbarPrefs.set("weather.featureGate", false); assertDisabled({ message: "After disabling again", - pendingFetchCount: 1, }); // Wait for the fetch to finish. @@ -249,21 +246,19 @@ add_tasks_with_rust(async function disableAndEnable_immediate1() { // uninitialized. assertDisabled({ message: "After awaiting fetch", - pendingFetchCount: 0, }); // Clean up by re-enabling the feature for the remaining tasks. - fetchPromise = QuickSuggest.weather.waitForFetches(); + fetchPromise = waitForNewWeatherFetch(); UrlbarPrefs.set("weather.featureGate", true); await fetchPromise; // Wait for keywords to be re-synced from remote settings. await QuickSuggestTestUtils.forceSync(); - assertEnabled({ + await assertEnabled({ message: "On cleanup", hasSuggestion: true, - pendingFetchCount: 0, }); }); @@ -279,26 +274,23 @@ add_tasks_with_rust(async function disableAndEnable_immediate1() { // from step 2 should be discarded. add_tasks_with_rust(async function disableAndEnable_immediate2() { // Sanity check initial state. - assertEnabled({ + await assertEnabled({ message: "Sanity check initial state", hasSuggestion: true, - pendingFetchCount: 0, }); // Disable the feature. It should be immediately uninitialized. UrlbarPrefs.set("weather.featureGate", false); assertDisabled({ message: "After disabling", - pendingFetchCount: 0, }); // Re-enable the feature. It should be immediately initialized and a fetch // should start. UrlbarPrefs.set("weather.featureGate", true); - assertEnabled({ + await assertEnabled({ message: "Immediately after re-enabling", hasSuggestion: false, - pendingFetchCount: 1, }); // Disable it again. The fetch will remain ongoing since pending fetches @@ -306,25 +298,21 @@ add_tasks_with_rust(async function disableAndEnable_immediate2() { UrlbarPrefs.set("weather.featureGate", false); assertDisabled({ message: "After disabling again", - pendingFetchCount: 1, }); - // Re-enable it. A new fetch should start, so now there will be two pending - // fetches. - let fetchPromise = QuickSuggest.weather.waitForFetches(); + // Re-enable it. A new fetch should start. + let fetchPromise = waitForNewWeatherFetch(); UrlbarPrefs.set("weather.featureGate", true); - assertEnabled({ + await assertEnabled({ message: "Immediately after re-enabling again", hasSuggestion: false, - pendingFetchCount: 2, }); - // Wait for both fetches to finish. + // Wait for it to finish. await fetchPromise; - assertEnabled({ + await assertEnabled({ message: "Immediately after re-enabling again", hasSuggestion: true, - pendingFetchCount: 0, }); // Wait for keywords to be re-synced from remote settings. @@ -334,10 +322,9 @@ add_tasks_with_rust(async function disableAndEnable_immediate2() { // A fetch that doesn't return a suggestion should cause the last-fetched // suggestion to be discarded. add_tasks_with_rust(async function noSuggestion() { - assertEnabled({ + await assertEnabled({ message: "Sanity check initial state", hasSuggestion: true, - pendingFetchCount: 0, }); let histograms = MerinoTestUtils.getAndClearHistograms({ @@ -350,10 +337,9 @@ add_tasks_with_rust(async function noSuggestion() { await QuickSuggest.weather._test_fetch(); - assertEnabled({ + await assertEnabled({ message: "After fetch", hasSuggestion: false, - pendingFetchCount: 0, }); Assert.equal( QuickSuggest.weather._test_merino.lastFetchStatus, @@ -381,19 +367,17 @@ add_tasks_with_rust(async function noSuggestion() { // Clean up by forcing another fetch so the suggestion is non-null for the // remaining tasks. await QuickSuggest.weather._test_fetch(); - assertEnabled({ + await assertEnabled({ message: "On cleanup", hasSuggestion: true, - pendingFetchCount: 0, }); }); // A network error should cause the last-fetched suggestion to be discarded. add_tasks_with_rust(async function networkError() { - assertEnabled({ + await assertEnabled({ message: "Sanity check initial state", hasSuggestion: true, - pendingFetchCount: 0, }); let histograms = MerinoTestUtils.getAndClearHistograms({ @@ -411,10 +395,9 @@ add_tasks_with_rust(async function networkError() { QuickSuggest.weather._test_setTimeoutMs(-1); - assertEnabled({ + await assertEnabled({ message: "After fetch", hasSuggestion: false, - pendingFetchCount: 0, }); Assert.equal( QuickSuggest.weather._test_merino.lastFetchStatus, @@ -440,19 +423,17 @@ add_tasks_with_rust(async function networkError() { // Clean up by forcing another fetch so the suggestion is non-null for the // remaining tasks. await QuickSuggest.weather._test_fetch(); - assertEnabled({ + await assertEnabled({ message: "On cleanup", hasSuggestion: true, - pendingFetchCount: 0, }); }); // An HTTP error should cause the last-fetched suggestion to be discarded. add_tasks_with_rust(async function httpError() { - assertEnabled({ + await assertEnabled({ message: "Sanity check initial state", hasSuggestion: true, - pendingFetchCount: 0, }); let histograms = MerinoTestUtils.getAndClearHistograms({ @@ -463,10 +444,9 @@ add_tasks_with_rust(async function httpError() { MerinoTestUtils.server.response = { status: 500 }; await QuickSuggest.weather._test_fetch(); - assertEnabled({ + await assertEnabled({ message: "After fetch", hasSuggestion: false, - pendingFetchCount: 0, }); Assert.equal( QuickSuggest.weather._test_merino.lastFetchStatus, @@ -494,20 +474,18 @@ add_tasks_with_rust(async function httpError() { MerinoTestUtils.server.reset(); MerinoTestUtils.server.response.body.suggestions = [WEATHER_SUGGESTION]; await QuickSuggest.weather._test_fetch(); - assertEnabled({ + await assertEnabled({ message: "On cleanup", hasSuggestion: true, - pendingFetchCount: 0, }); }); // A fetch that doesn't return a suggestion due to a client timeout should cause // the last-fetched suggestion to be discarded. add_tasks_with_rust(async function clientTimeout() { - assertEnabled({ + await assertEnabled({ message: "Sanity check initial state", hasSuggestion: true, - pendingFetchCount: 0, }); let histograms = MerinoTestUtils.getAndClearHistograms({ @@ -528,10 +506,9 @@ add_tasks_with_rust(async function clientTimeout() { await QuickSuggest.weather._test_fetch(); - assertEnabled({ + await assertEnabled({ message: "After fetch", hasSuggestion: false, - pendingFetchCount: 0, }); Assert.equal( QuickSuggest.weather._test_merino.lastFetchStatus, @@ -573,10 +550,9 @@ add_tasks_with_rust(async function clientTimeout() { // Clean up by forcing another fetch so the suggestion is non-null for the // remaining tasks. await QuickSuggest.weather._test_fetch(); - assertEnabled({ + await assertEnabled({ message: "On cleanup", hasSuggestion: true, - pendingFetchCount: 0, }); }); @@ -662,10 +638,9 @@ async function doLocaleTest({ shouldRunTask, osUnit, unitsByLocale }) { return; } - assertEnabled({ + await assertEnabled({ message: "Sanity check initial state", hasSuggestion: true, - pendingFetchCount: 0, }); // Sanity check initial locale info. @@ -716,10 +691,9 @@ async function doLocaleTest({ shouldRunTask, osUnit, unitsByLocale }) { // Blocks a result and makes sure the weather pref is disabled. add_tasks_with_rust(async function block() { // Sanity check initial state. - assertEnabled({ + await assertEnabled({ message: "Sanity check initial state", hasSuggestion: true, - pendingFetchCount: 0, }); Assert.ok( UrlbarPrefs.get("suggest.weather"), @@ -775,17 +749,16 @@ add_tasks_with_rust(async function block() { }); // Re-enable the pref and clean up. - let fetchPromise = QuickSuggest.weather.waitForFetches(); + let fetchPromise = waitForNewWeatherFetch(); UrlbarPrefs.set("suggest.weather", true); await fetchPromise; // Wait for keywords to be re-synced from remote settings. await QuickSuggestTestUtils.forceSync(); - assertEnabled({ + await assertEnabled({ message: "On cleanup", hasSuggestion: true, - pendingFetchCount: 0, }); }); @@ -849,16 +822,12 @@ async function doWakeTest({ // Advance the clock and simulate wake. info("Sending wake notification"); + let fetchPromise = waitForNewWeatherFetch(); let nowOnWake = nowOnStart + sleepIntervalMs; dateNowStub.returns(nowOnWake); QuickSuggest.weather.observe(null, "wake_notification", ""); Assert.equal( - QuickSuggest.weather._test_pendingFetchCount, - 0, - "After wake, next fetch should not have immediately started" - ); - Assert.equal( QuickSuggest.weather._test_lastFetchTimeMs, nowOnStart, "After wake, last fetch time should be unchanged" @@ -891,18 +860,13 @@ async function doWakeTest({ // Wait for the fetch. If the wake didn't trigger it, then the caller should // have passed in a `sleepIntervalMs` that will make it start soon. info("Waiting for fetch after wake"); - await QuickSuggest.weather.waitForFetches(); + await fetchPromise; Assert.equal( QuickSuggest.weather._test_fetchTimerMs, QuickSuggest.weather._test_fetchIntervalMs, "After post-wake fetch, timer period should remain full fetch interval" ); - Assert.equal( - QuickSuggest.weather._test_pendingFetchCount, - 0, - "After post-wake fetch, no more fetches should be pending" - ); dateNowStub.restore(); } @@ -958,11 +922,6 @@ async function doOnlineTestWithSuggestion({ topic, dataValues }) { QuickSuggest.weather.observe(null, topic, data); Assert.equal( - QuickSuggest.weather._test_pendingFetchCount, - 0, - "Fetch should not have started" - ); - Assert.equal( QuickSuggest.weather._test_fetchTimer, timer, "Timer should not have been recreated" @@ -1033,11 +992,6 @@ async function doOnlineTestWithNullSuggestion({ "Suggestion should remain null" ); Assert.equal( - QuickSuggest.weather._test_pendingFetchCount, - 0, - "Fetch should not have started" - ); - Assert.equal( QuickSuggest.weather._test_fetchTimer, timer, "Timer should not have been recreated" @@ -1055,13 +1009,9 @@ async function doOnlineTestWithNullSuggestion({ Assert.ok(!QuickSuggest.weather.suggestion, "Suggestion should be null"); info("Sending notification: " + JSON.stringify({ topic, data })); + let fetchPromise = waitForNewWeatherFetch(); QuickSuggest.weather.observe(null, topic, data); - Assert.equal( - QuickSuggest.weather._test_pendingFetchCount, - 0, - "Fetch should not have started yet" - ); Assert.notEqual( QuickSuggest.weather._test_fetchTimer, 0, @@ -1081,13 +1031,8 @@ async function doOnlineTestWithNullSuggestion({ timer = QuickSuggest.weather._test_fetchTimer; info("Waiting for fetch after notification"); - await QuickSuggest.weather.waitForFetches(); + await fetchPromise; - Assert.equal( - QuickSuggest.weather._test_pendingFetchCount, - 0, - "Fetch should not be pending" - ); Assert.notEqual( QuickSuggest.weather._test_fetchTimer, 0, @@ -1164,20 +1109,23 @@ async function doManyNotificationsTest(notifications) { MerinoTestUtils.WEATHER_SUGGESTION, ]; + let { fetchPromise: oldFetchPromise } = QuickSuggest.weather; + let fetchPromise = waitForNewWeatherFetch(); + // Send the notifications. for (let [topic, data] of notifications) { info("Sending notification: " + JSON.stringify({ topic, data })); QuickSuggest.weather.observe(null, topic, data); + Assert.equal( + QuickSuggest.weather.fetchPromise, + oldFetchPromise, + "No new fetch should have started yet" + ); } info("Waiting for fetch after notifications"); - await QuickSuggest.weather.waitForFetches(); + await fetchPromise; - Assert.equal( - QuickSuggest.weather._test_pendingFetchCount, - 0, - "Fetch should not be pending" - ); Assert.notEqual( QuickSuggest.weather._test_fetchTimer, 0, @@ -1241,7 +1189,7 @@ add_tasks_with_rust(async function vpn() { // Simulate the link status changing. Since the mock link service still // indicates a VPN is detected, the suggestion should remain null. - let fetchPromise = QuickSuggest.weather.waitForFetches(); + let fetchPromise = waitForNewWeatherFetch(); QuickSuggest.weather.observe(null, "network:link-status-changed", "changed"); await fetchPromise; Assert.ok(!QuickSuggest.weather.suggestion, "Suggestion should remain null"); @@ -1251,7 +1199,7 @@ add_tasks_with_rust(async function vpn() { Ci.nsINetworkLinkService.NONE_DETECTED; // Simulate the link status changing again. The suggestion should be fetched. - fetchPromise = QuickSuggest.weather.waitForFetches(); + fetchPromise = waitForNewWeatherFetch(); QuickSuggest.weather.observe(null, "network:link-status-changed", "changed"); await fetchPromise; Assert.ok(QuickSuggest.weather.suggestion, "Suggestion should be fetched"); @@ -1264,10 +1212,9 @@ add_tasks_with_rust(async function vpn() { // weather record. add_tasks_with_rust(async function nimbusOverride() { // Sanity check initial state. - assertEnabled({ + await assertEnabled({ message: "Sanity check initial state", hasSuggestion: true, - pendingFetchCount: 0, }); let defaultResult = makeWeatherResult(); @@ -1349,7 +1296,7 @@ add_tasks_with_rust(async function nimbusOverride() { }); }); -function assertEnabled({ message, hasSuggestion, pendingFetchCount }) { +async function assertEnabled({ message, hasSuggestion }) { info("Asserting feature is enabled"); if (message) { info(message); @@ -1360,20 +1307,20 @@ function assertEnabled({ message, hasSuggestion, pendingFetchCount }) { hasSuggestion, "Suggestion is null or non-null as expected" ); + Assert.ok(QuickSuggest.weather._test_merino, "Merino client is non-null"); + + await TestUtils.waitForCondition( + () => QuickSuggest.weather._test_fetchTimer, + "Waiting for fetch timer to become non-zero" + ); Assert.notEqual( QuickSuggest.weather._test_fetchTimer, 0, "Fetch timer is non-zero" ); - Assert.ok(QuickSuggest.weather._test_merino, "Merino client is non-null"); - Assert.equal( - QuickSuggest.weather._test_pendingFetchCount, - pendingFetchCount, - "Expected pending fetch count" - ); } -function assertDisabled({ message, pendingFetchCount }) { +function assertDisabled({ message }) { info("Asserting feature is disabled"); if (message) { info(message); @@ -1394,9 +1341,4 @@ function assertDisabled({ message, pendingFetchCount }) { null, "Merino client is null" ); - Assert.equal( - QuickSuggest.weather._test_pendingFetchCount, - pendingFetchCount, - "Expected pending fetch count" - ); } diff --git a/browser/components/urlbar/tests/quicksuggest/unit/test_weather_keywords.js b/browser/components/urlbar/tests/quicksuggest/unit/test_weather_keywords.js index efa5922c3e..559e0cc1fa 100644 --- a/browser/components/urlbar/tests/quicksuggest/unit/test_weather_keywords.js +++ b/browser/components/urlbar/tests/quicksuggest/unit/test_weather_keywords.js @@ -23,6 +23,15 @@ add_setup(async () => { prefs: [["suggest.quicksuggest.nonsponsored", true]], }); await MerinoTestUtils.initWeather(); + + // When `add_tasks_with_rust()` disables the Rust backend and forces sync, the + // JS backend will sync `Weather` with remote settings. Since keywords are + // present in remote settings at that point (we added them above), `Weather` + // will then start fetching. The fetch may or may not be done before our test + // task starts. To make sure it's done, queue another fetch and await it. + registerAddTasksWithRustSetup(async () => { + await QuickSuggest.weather._test_fetch(); + }); }); // * Settings data: none @@ -89,9 +98,9 @@ add_tasks_with_rust(async function () { // // JS backend only. The Rust component expects settings data to contain // min_keyword_length. -add_task( +add_tasks_with_rust( { - skip_if: () => UrlbarPrefs.get("quickSuggestRustEnabled"), + skip_if_rust_enabled: true, }, async function () { await doKeywordsTest({ @@ -128,9 +137,9 @@ add_task( // // JS backend only. The Rust component doesn't treat minKeywordLength == 0 as a // special case. -add_task( +add_tasks_with_rust( { - skip_if: () => UrlbarPrefs.get("quickSuggestRustEnabled"), + skip_if_rust_enabled: true, }, async function () { await doKeywordsTest({ @@ -296,9 +305,9 @@ add_tasks_with_rust(async function () { // // JS backend only. The Rust component expects settings data to contain // min_keyword_length. -add_task( +add_tasks_with_rust( { - skip_if: () => UrlbarPrefs.get("quickSuggestRustEnabled"), + skip_if_rust_enabled: true, }, async function () { await doKeywordsTest({ @@ -338,9 +347,9 @@ add_task( // // JS backend only. The Rust component doesn't treat minKeywordLength == 0 as a // special case. -add_task( +add_tasks_with_rust( { - skip_if: () => UrlbarPrefs.get("quickSuggestRustEnabled"), + skip_if_rust_enabled: true, }, async function () { await doKeywordsTest({ @@ -578,9 +587,9 @@ add_tasks_with_rust(async function () { // TODO bug 1879209: This doesn't work with the Rust backend because if // min_keyword_length isn't specified on ingest, the Rust database will retain // the last known good min_keyword_length, which interferes with this task. -add_task( +add_tasks_with_rust( { - skip_if: () => UrlbarPrefs.get("quickSuggestRustEnabled"), + skip_if_rust_enabled: true, }, async function () { await doKeywordsTest({ @@ -618,9 +627,9 @@ add_task( // TODO bug 1879209: This doesn't work with the Rust backend because if // min_keyword_length isn't specified on ingest, the Rust database will retain // the last known good min_keyword_length, which interferes with this task. -add_task( +add_tasks_with_rust( { - skip_if: () => UrlbarPrefs.get("quickSuggestRustEnabled"), + skip_if_rust_enabled: true, }, async function () { await doKeywordsTest({ @@ -817,7 +826,7 @@ async function doKeywordsTest({ !UrlbarPrefs.get("quickSuggestRustEnabled") && (nimbusValues?.weatherKeywords || settingsData?.keywords) ) { - fetchPromise = QuickSuggest.weather.waitForFetches(); + fetchPromise = waitForNewWeatherFetch(); } let nimbusCleanup; @@ -838,7 +847,6 @@ async function doKeywordsTest({ if (fetchPromise) { info("Waiting for fetch"); - assertFetchingStarted({ pendingFetchCount: 1 }); await fetchPromise; info("Got fetch"); } @@ -872,10 +880,10 @@ async function doKeywordsTest({ await nimbusCleanup?.(); - fetchPromise = null; if (!QuickSuggest.weather.suggestion) { - fetchPromise = QuickSuggest.weather.waitForFetches(); + fetchPromise = waitForNewWeatherFetch(); } + await QuickSuggestTestUtils.setRemoteSettingsRecords([ { type: "weather", @@ -905,28 +913,8 @@ async function doMatchingQuickSuggestTest(pref, isSponsored) { let keyword = "test"; let attachment = isSponsored - ? { - id: 1, - url: "http://example.com/amp", - title: "AMP Suggestion", - keywords: [keyword], - click_url: "http://example.com/amp-click", - impression_url: "http://example.com/amp-impression", - advertiser: "Amp", - iab_category: "22 - Shopping", - icon: "1234", - } - : { - id: 2, - url: "http://example.com/wikipedia", - title: "Wikipedia Suggestion", - keywords: [keyword], - click_url: "http://example.com/wikipedia-click", - impression_url: "http://example.com/wikipedia-impression", - advertiser: "Wikipedia", - iab_category: "5 - Education", - icon: "1234", - }; + ? QuickSuggestTestUtils.ampRemoteSettings({ keywords: [keyword] }) + : QuickSuggestTestUtils.wikipediaRemoteSettings({ keywords: [keyword] }); // Add a remote settings result to quick suggest. let oldPrefValue = UrlbarPrefs.get(pref); @@ -943,27 +931,6 @@ async function doMatchingQuickSuggestTest(pref, isSponsored) { ]); // First do a search to verify the quick suggest result matches the keyword. - let payload; - if (!UrlbarPrefs.get("quickSuggestRustEnabled")) { - payload = { - source: "remote-settings", - provider: "AdmWikipedia", - sponsoredImpressionUrl: attachment.impression_url, - sponsoredClickUrl: attachment.click_url, - sponsoredBlockId: attachment.id, - }; - } else { - payload = { - source: "rust", - provider: isSponsored ? "Amp" : "Wikipedia", - }; - if (isSponsored) { - payload.sponsoredImpressionUrl = attachment.impression_url; - payload.sponsoredClickUrl = attachment.click_url; - payload.sponsoredBlockId = attachment.id; - } - } - info("Doing first search for quick suggest result"); await check_results({ context: createContext(keyword, { @@ -971,35 +938,9 @@ async function doMatchingQuickSuggestTest(pref, isSponsored) { isPrivate: false, }), matches: [ - { - type: UrlbarUtils.RESULT_TYPE.URL, - source: UrlbarUtils.RESULT_SOURCE.SEARCH, - heuristic: false, - payload: { - ...payload, - telemetryType: isSponsored ? "adm_sponsored" : "adm_nonsponsored", - qsSuggestion: keyword, - title: attachment.title, - url: attachment.url, - displayUrl: attachment.url.replace(/[/]$/, ""), - originalUrl: attachment.url, - icon: null, - sponsoredAdvertiser: attachment.advertiser, - sponsoredIabCategory: attachment.iab_category, - isSponsored, - descriptionL10n: isSponsored - ? { id: "urlbar-result-action-sponsored" } - : undefined, - helpUrl: QuickSuggest.HELP_URL, - helpL10n: { - id: "urlbar-result-menu-learn-more-about-firefox-suggest", - }, - isBlockable: true, - blockL10n: { - id: "urlbar-result-menu-dismiss-firefox-suggest", - }, - }, - }, + isSponsored + ? makeAmpResult({ keyword }) + : makeWikipediaResult({ keyword }), ], }); @@ -1398,7 +1339,7 @@ async function doIncrementTest({ !UrlbarPrefs.get("quickSuggestRustEnabled") && (nimbusValues?.weatherKeywords || settingsData?.weather?.keywords) ) { - fetchPromise = QuickSuggest.weather.waitForFetches(); + fetchPromise = waitForNewWeatherFetch(); } let nimbusCleanup; @@ -1419,7 +1360,6 @@ async function doIncrementTest({ if (fetchPromise) { info("Waiting for fetch"); - assertFetchingStarted({ pendingFetchCount: 1 }); await fetchPromise; info("Got fetch"); } @@ -1472,9 +1412,8 @@ async function doIncrementTest({ await nimbusCleanup?.(); - fetchPromise = null; if (!QuickSuggest.weather.suggestion) { - fetchPromise = QuickSuggest.weather.waitForFetches(); + fetchPromise = waitForNewWeatherFetch(); } await QuickSuggestTestUtils.setRemoteSettingsRecords([ { @@ -1485,19 +1424,3 @@ async function doIncrementTest({ UrlbarPrefs.clear("weather.minKeywordLength"); await fetchPromise; } - -function assertFetchingStarted() { - info("Asserting fetching has started"); - - Assert.notEqual( - QuickSuggest.weather._test_fetchTimer, - 0, - "Fetch timer is non-zero" - ); - Assert.ok(QuickSuggest.weather._test_merino, "Merino client is non-null"); - Assert.equal( - QuickSuggest.weather._test_pendingFetchCount, - 1, - "Expected pending fetch count" - ); -} diff --git a/browser/components/urlbar/tests/quicksuggest/unit/xpcshell.toml b/browser/components/urlbar/tests/quicksuggest/unit/xpcshell.toml index ceab478795..1f0e226684 100644 --- a/browser/components/urlbar/tests/quicksuggest/unit/xpcshell.toml +++ b/browser/components/urlbar/tests/quicksuggest/unit/xpcshell.toml @@ -49,3 +49,4 @@ skip-if = ["true"] # Bug 1880214 ["test_weather.js"] ["test_weather_keywords.js"] +skip-if = ["verify"] # Bug 1880214 - Takes a very long time due to add_tasks_with_rust() |