commit c46798b2217a5331061230224c19c0826154d5ef parent 596d8db2a1a0c7864ca3ada9eb810490d0d2babb Author: James Teow <jteow@mozilla.com> Date: Tue, 11 Nov 2025 02:37:13 +0000 Bug 1986285 - Part 4: Fix tests in toolkit/places - r=places-reviewers,daisuke Differential Revision: https://phabricator.services.mozilla.com/D264738 Diffstat:
9 files changed, 347 insertions(+), 266 deletions(-)
diff --git a/toolkit/components/places/tests/bookmarks/test_bookmarks_eraseEverything.js b/toolkit/components/places/tests/bookmarks/test_bookmarks_eraseEverything.js @@ -1,6 +1,14 @@ /* Any copyright is dedicated to the Public Domain. * http://creativecommons.org/publicdomain/zero/1.0/ */ +// This test originally expected bookmarks added after visits to boost frecency +// scores. The frecency algorithm has since changed - adding a bookmark after a +// visit doesn't retroactively change the frecency score for a site unless no +// visits exist for a bookmark. +// +// This test now verifies that frecency remains stable when bookmarks are added +// and removed after visits, and ensures eraseEverything() doesn't modify the +// frecency calculation. add_task(async function test_eraseEverything() { await PlacesTestUtils.addVisits({ uri: NetUtil.newURI("http://example.com/"), @@ -8,18 +16,33 @@ add_task(async function test_eraseEverything() { await PlacesTestUtils.addVisits({ uri: NetUtil.newURI("http://mozilla.org/"), }); - let frecencyForExample = await PlacesTestUtils.getDatabaseValue( + + let exampleFrecency = await PlacesTestUtils.getDatabaseValue( "moz_places", "frecency", - { url: "http://example.com/" } + { + url: "http://example.com/", + } + ); + Assert.greater( + exampleFrecency, + 0, + "http://example.com should have a non-zero frecency." ); - let frecencyForMozilla = await PlacesTestUtils.getDatabaseValue( + + let mozillaFrecency = await PlacesTestUtils.getDatabaseValue( "moz_places", "frecency", - { url: "http://mozilla.org/" } + { + url: "http://mozilla.org/", + } + ); + Assert.greater( + mozillaFrecency, + 0, + "http://mozilla.org should have a non-zero frecency." ); - Assert.greater(frecencyForExample, 0); - Assert.greater(frecencyForMozilla, 0); + let unfiledFolder = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid, type: PlacesUtils.bookmarks.TYPE_FOLDER, @@ -75,17 +98,19 @@ add_task(async function test_eraseEverything() { checkBookmarkObject(toolbarBookmarkInFolder); await PlacesFrecencyRecalculator.recalculateAnyOutdatedFrecencies(); - Assert.greater( + Assert.equal( await PlacesTestUtils.getDatabaseValue("moz_places", "frecency", { url: "http://example.com/", }), - frecencyForExample + exampleFrecency, + "Frecency should not have changed." ); - Assert.greater( + Assert.equal( await PlacesTestUtils.getDatabaseValue("moz_places", "frecency", { - url: "http://example.com/", + url: "http://mozilla.org/", }), - frecencyForMozilla + mozillaFrecency, + "Frecency should not have changed." ); await PlacesUtils.bookmarks.eraseEverything(); @@ -95,13 +120,15 @@ add_task(async function test_eraseEverything() { await PlacesTestUtils.getDatabaseValue("moz_places", "frecency", { url: "http://example.com/", }), - frecencyForExample + exampleFrecency, + "Frecency should not have changed." ); Assert.equal( await PlacesTestUtils.getDatabaseValue("moz_places", "frecency", { - url: "http://example.com/", + url: "http://mozilla.org/", }), - frecencyForMozilla + mozillaFrecency, + "Frecency should not have changed." ); }); diff --git a/toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js b/toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js @@ -12,16 +12,6 @@ const TARGET_URI = Services.io.newURI( "https://test1.example.com/tests/toolkit/components/places/tests/browser/final.html" ); -const REDIRECT_SOURCE_VISIT_BONUS = Services.prefs.getIntPref( - "places.frecency.redirectSourceVisitBonus" -); -const PERM_REDIRECT_VISIT_BONUS = Services.prefs.getIntPref( - "places.frecency.permRedirectVisitBonus" -); -const TYPED_VISIT_BONUS = Services.prefs.getIntPref( - "places.frecency.typedVisitBonus" -); - // Ensure that decay frecency doesn't kick in during tests (as a result // of idle-daily). Services.prefs.setCharPref("places.frecency.decayRate", "1.0"); @@ -66,6 +56,10 @@ let firstRedirectBonus = 0; let nextRedirectBonus = 0; let targetBonus = 0; +/** + * A non-typed redirect source should have the same values as intermediate + * URLs and be lower than the target URL. + */ add_task(async function test_multiple_redirect() { // The redirect source bonus overrides the link bonus. let visitedPromise = waitVisitedNotifications(); @@ -79,18 +73,16 @@ add_task(async function test_multiple_redirect() { let redirectNotified = await visitedPromise; ok(redirectNotified, "The redirect should have been notified"); - firstRedirectBonus += REDIRECT_SOURCE_VISIT_BONUS; - await check_uri(REDIRECT_URI, firstRedirectBonus, 1); - nextRedirectBonus += REDIRECT_SOURCE_VISIT_BONUS; - await check_uri(INTERMEDIATE_URI_1, nextRedirectBonus, 1); - await check_uri(INTERMEDIATE_URI_2, nextRedirectBonus, 1); - // TODO Bug 487813 - This should be TYPED_VISIT_BONUS, however as we don't - // currently track redirects across multiple redirects, we fallback to the - // PERM_REDIRECT_VISIT_BONUS. - targetBonus += PERM_REDIRECT_VISIT_BONUS; - await check_uri(TARGET_URI, targetBonus, 0); + await checkRedirect( + REDIRECT_URI.spec, + TARGET_URI.spec, + [INTERMEDIATE_URI_1.spec, INTERMEDIATE_URI_2.spec], + false + ); } ); + + await PlacesUtils.history.clear(); }); add_task(async function test_multiple_redirect_typed() { @@ -107,21 +99,23 @@ add_task(async function test_multiple_redirect_typed() { let redirectNotified = await visitedPromise; ok(redirectNotified, "The redirect should have been notified"); - firstRedirectBonus += TYPED_VISIT_BONUS; - await check_uri(REDIRECT_URI, firstRedirectBonus, 1); - nextRedirectBonus += REDIRECT_SOURCE_VISIT_BONUS; - await check_uri(INTERMEDIATE_URI_1, nextRedirectBonus, 1); - await check_uri(INTERMEDIATE_URI_2, nextRedirectBonus, 1); - // TODO Bug 487813 - This should be TYPED_VISIT_BONUS, however as we don't - // currently track redirects across multiple redirects, we fallback to the - // PERM_REDIRECT_VISIT_BONUS. - targetBonus += PERM_REDIRECT_VISIT_BONUS; - await check_uri(TARGET_URI, targetBonus, 0); + await checkRedirect( + REDIRECT_URI.spec, + TARGET_URI.spec, + [INTERMEDIATE_URI_1.spec, INTERMEDIATE_URI_2.spec], + true + ); } ); + + await PlacesUtils.history.clear(); }); -add_task(async function test_second_typed_visit() { +/** + * Without any history, a typed redirect URL should consistently have a higher + * value than the target URL. + */ +add_task(async function test_multiple_typed_visit() { // The typed bonus wins because the redirect is permanent. PlacesUtils.history.markPageAsTyped(REDIRECT_URI); let visitedPromise = waitVisitedNotifications(); @@ -134,23 +128,39 @@ add_task(async function test_second_typed_visit() { info("Waiting for onVisits"); let redirectNotified = await visitedPromise; ok(redirectNotified, "The redirect should have been notified"); + } + ); + + PlacesUtils.history.markPageAsTyped(REDIRECT_URI); + visitedPromise = waitVisitedNotifications(); + await BrowserTestUtils.withNewTab( + { + gBrowser, + url: REDIRECT_URI.spec, + }, + async function () { + info("Waiting for onVisits"); + let redirectNotified = await visitedPromise; + ok(redirectNotified, "The redirect should have been notified"); - firstRedirectBonus += TYPED_VISIT_BONUS; - await check_uri(REDIRECT_URI, firstRedirectBonus, 1); - nextRedirectBonus += REDIRECT_SOURCE_VISIT_BONUS; - await check_uri(INTERMEDIATE_URI_1, nextRedirectBonus, 1); - await check_uri(INTERMEDIATE_URI_2, nextRedirectBonus, 1); - // TODO Bug 487813 - This should be TYPED_VISIT_BONUS, however as we don't - // currently track redirects across multiple redirects, we fallback to the - // PERM_REDIRECT_VISIT_BONUS. - targetBonus += PERM_REDIRECT_VISIT_BONUS; - await check_uri(TARGET_URI, targetBonus, 0); + await checkRedirect( + REDIRECT_URI.spec, + TARGET_URI.spec, + [INTERMEDIATE_URI_1.spec, INTERMEDIATE_URI_2.spec], + true + ); } ); + + await PlacesUtils.history.clear(); }); -add_task(async function test_subsequent_link_visit() { - // Another non typed visit. +/** + * Assume the user first typed a source re-direct. Then later visited the + * re-direct without typing. + */ +add_task(async function test_typed_then_redirect_visit() { + PlacesUtils.history.markPageAsTyped(REDIRECT_URI); let visitedPromise = waitVisitedNotifications(); await BrowserTestUtils.withNewTab( { @@ -161,17 +171,28 @@ add_task(async function test_subsequent_link_visit() { info("Waiting for onVisits"); let redirectNotified = await visitedPromise; ok(redirectNotified, "The redirect should have been notified"); + } + ); - firstRedirectBonus += REDIRECT_SOURCE_VISIT_BONUS; - await check_uri(REDIRECT_URI, firstRedirectBonus, 1); - nextRedirectBonus += REDIRECT_SOURCE_VISIT_BONUS; - await check_uri(INTERMEDIATE_URI_1, nextRedirectBonus, 1); - await check_uri(INTERMEDIATE_URI_2, nextRedirectBonus, 1); - // TODO Bug 487813 - This should be TYPED_VISIT_BONUS, however as we don't - // currently track redirects across multiple redirects, we fallback to the - // PERM_REDIRECT_VISIT_BONUS. - targetBonus += PERM_REDIRECT_VISIT_BONUS; - await check_uri(TARGET_URI, targetBonus, 0); + visitedPromise = waitVisitedNotifications(); + await BrowserTestUtils.withNewTab( + { + gBrowser, + url: REDIRECT_URI.spec, + }, + async function () { + info("Waiting for onVisits"); + let redirectNotified = await visitedPromise; + ok(redirectNotified, "The redirect should have been notified"); + + await checkRedirect( + REDIRECT_URI.spec, + TARGET_URI.spec, + [INTERMEDIATE_URI_1.spec, INTERMEDIATE_URI_2.spec], + false + ); } ); + + await PlacesUtils.history.clear(); }); diff --git a/toolkit/components/places/tests/browser/browser_redirect.js b/toolkit/components/places/tests/browser/browser_redirect.js @@ -6,16 +6,6 @@ const ROOT_URI = const REDIRECT_URI = Services.io.newURI(ROOT_URI + "redirect.sjs"); const TARGET_URI = Services.io.newURI(ROOT_URI + "redirect-target.html"); -const REDIRECT_SOURCE_VISIT_BONUS = Services.prefs.getIntPref( - "places.frecency.redirectSourceVisitBonus" -); -const LINK_VISIT_BONUS = Services.prefs.getIntPref( - "places.frecency.linkVisitBonus" -); -const TYPED_VISIT_BONUS = Services.prefs.getIntPref( - "places.frecency.typedVisitBonus" -); - // Ensure that decay frecency doesn't kick in during tests (as a result // of idle-daily). Services.prefs.setCharPref("places.frecency.decayRate", "1.0"); @@ -25,32 +15,10 @@ registerCleanupFunction(async function () { await PlacesUtils.history.clear(); }); -let redirectSourceFrecency = 0; -let redirectTargetFrecency = 0; - -async function check_uri(uri, frecency, hidden) { - is( - await PlacesTestUtils.getDatabaseValue("moz_places", "frecency", { - url: uri, - }), - frecency, - "Frecency of the page is the expected one" - ); - is( - await PlacesTestUtils.getDatabaseValue("moz_places", "hidden", { - url: uri, - }), - hidden, - "Hidden value of the page is the expected one" - ); -} - add_task(async function redirect_check_new_typed_visit() { // Used to verify the redirect bonus overrides the typed bonus. PlacesUtils.history.markPageAsTyped(REDIRECT_URI); - redirectSourceFrecency += REDIRECT_SOURCE_VISIT_BONUS; - redirectTargetFrecency += TYPED_VISIT_BONUS; let redirectNotified = false; let visitedPromise = PlacesTestUtils.waitForNotification( @@ -74,8 +42,7 @@ add_task(async function redirect_check_new_typed_visit() { await visitedPromise; ok(redirectNotified, "The redirect should have been notified"); - await check_uri(REDIRECT_URI, redirectSourceFrecency, 1); - await check_uri(TARGET_URI, redirectTargetFrecency, 0); + await checkRedirect(REDIRECT_URI.spec, TARGET_URI.spec, [], true); BrowserTestUtils.removeTab(tab); }); @@ -84,8 +51,6 @@ add_task(async function redirect_check_second_typed_visit() { // A second visit with a typed url. PlacesUtils.history.markPageAsTyped(REDIRECT_URI); - redirectSourceFrecency += REDIRECT_SOURCE_VISIT_BONUS; - redirectTargetFrecency += TYPED_VISIT_BONUS; let redirectNotified = false; let visitedPromise = PlacesTestUtils.waitForNotification( @@ -109,16 +74,13 @@ add_task(async function redirect_check_second_typed_visit() { await visitedPromise; ok(redirectNotified, "The redirect should have been notified"); - await check_uri(REDIRECT_URI, redirectSourceFrecency, 1); - await check_uri(TARGET_URI, redirectTargetFrecency, 0); + await checkRedirect(REDIRECT_URI.spec, TARGET_URI.spec, [], true); BrowserTestUtils.removeTab(tab); }); add_task(async function redirect_check_subsequent_link_visit() { // Another visit, but this time as a visited url. - redirectSourceFrecency += REDIRECT_SOURCE_VISIT_BONUS; - redirectTargetFrecency += LINK_VISIT_BONUS; let redirectNotified = false; let visitedPromise = PlacesTestUtils.waitForNotification( @@ -142,8 +104,7 @@ add_task(async function redirect_check_subsequent_link_visit() { await visitedPromise; ok(redirectNotified, "The redirect should have been notified"); - await check_uri(REDIRECT_URI, redirectSourceFrecency, 1); - await check_uri(TARGET_URI, redirectTargetFrecency, 0); + await checkRedirect(REDIRECT_URI.spec, TARGET_URI.spec, [], true); BrowserTestUtils.removeTab(tab); }); diff --git a/toolkit/components/places/tests/browser/browser_visited_notfound.js b/toolkit/components/places/tests/browser/browser_visited_notfound.js @@ -19,7 +19,7 @@ add_task(async function test() { "frecency", { url } ); - Assert.equal(frecency, 100, "Check initial frecency"); + Assert.greater(frecency, 0, "Check initial frecency"); // Used to verify errors are not marked as typed. PlacesUtils.history.markPageAsTyped(NetUtil.newURI(url)); diff --git a/toolkit/components/places/tests/browser/head.js b/toolkit/components/places/tests/browser/head.js @@ -94,3 +94,131 @@ async function assertLinkVisitedStatus( ); Assert.ok(true, "The visited state is corerct"); } + +async function checkFrecencyEqual(url1, url2) { + let frecency1 = await PlacesTestUtils.getDatabaseValue( + "moz_places", + "frecency", + { + url: url1, + } + ); + let frecency2 = await PlacesTestUtils.getDatabaseValue( + "moz_places", + "frecency", + { + url: url2, + } + ); + Assert.equal( + frecency1, + frecency2, + `Frecency of ${url1} is equal to frecency of ${url2}.` + ); +} + +async function checkFrecencyGreater(url1, url2) { + let frecency1 = await PlacesTestUtils.getDatabaseValue( + "moz_places", + "frecency", + { + url: url1, + } + ); + let frecency2 = await PlacesTestUtils.getDatabaseValue( + "moz_places", + "frecency", + { + url: url2, + } + ); + Assert.greater( + frecency1, + frecency2, + `Frecency of ${url1} is greater than frecency of ${url2}.` + ); +} + +async function checkFrecencyNonZero(url) { + Assert.greater( + await PlacesTestUtils.getDatabaseValue("moz_places", "frecency", { + url, + }), + 0, + `Frecency of ${url} is greater than 0.` + ); +} + +async function checkUrlHidden(url, expectedHidden) { + let hiddenState = await PlacesTestUtils.getDatabaseValue( + "moz_places", + "hidden", + { + url, + } + ); + Assert.equal( + hiddenState, + expectedHidden, + `URL ${url} is ${expectedHidden ? "hidden" : "not hidden."}` + ); +} + +async function checkRedirect( + redirectUrl, + targetUrl, + intermediateUrls = [], + isTyped = false +) { + // First, check redirect and intermediate urls are hidden and the target URL + // is not hidden. + await checkUrlHidden(redirectUrl, 1); + for (let intermediateUrl of intermediateUrls) { + await checkUrlHidden(intermediateUrl, 1); + } + await checkUrlHidden(targetUrl, 0); + + // Check the frecency values of redirect URLs are expected. + await checkFrecencyNonZero(redirectUrl); + for (let intermediateUrl of intermediateUrls) { + if (isTyped) { + await checkFrecencyGreater(redirectUrl, intermediateUrl); + } else { + await checkFrecencyEqual(redirectUrl, intermediateUrl); + } + } + + // Check intermediate urls have the same frecency. + let intermediateUrlObj = []; + for (let intermediateUrl of intermediateUrls) { + let frecency = await PlacesTestUtils.getDatabaseValue( + "moz_places", + "frecency", + { + url: intermediateUrl, + } + ); + intermediateUrlObj.push({ url: intermediateUrl, frecency }); + } + if (intermediateUrlObj.length) { + let expected = intermediateUrlObj[0]; + for (let i = 1; i < intermediateUrlObj.length; ++i) { + Assert.equal( + intermediateUrlObj[i].frecency, + expected.frecency, + `Frecency of intermediate url ${intermediateUrlObj[i].url} should match ${expected.url}` + ); + } + } + + // Check the target URL has a higher frecency than the intermediate URLs. + for (let intermediateUrl of intermediateUrls) { + await checkFrecencyGreater(targetUrl, intermediateUrl); + } + + if (isTyped && intermediateUrls.length) { + await checkFrecencyGreater(redirectUrl, targetUrl); + } else { + await checkFrecencyGreater(targetUrl, redirectUrl); + } +} diff --git a/toolkit/components/places/tests/sync/test_bookmark_observer_recorder.js b/toolkit/components/places/tests/sync/test_bookmark_observer_recorder.js @@ -248,6 +248,7 @@ add_task(async function test_update_frecencies() { await buf.finalize(); await PlacesUtils.bookmarks.eraseEverything(); await PlacesSyncUtils.bookmarks.reset(); + await PlacesUtils.history.clear(); }); async function setupLocalTree(localTimeSeconds) { @@ -488,7 +489,7 @@ add_task(async function test_apply_then_revert() { isTagging: false, title: "E", tags: "", - frecency: 0, + frecency: 1, hidden: false, visitCount: 0, dateAdded: dateAdded.getTime(), @@ -667,4 +668,5 @@ add_task(async function test_apply_then_revert() { await buf.finalize(); await PlacesUtils.bookmarks.eraseEverything(); await PlacesSyncUtils.bookmarks.reset(); + await PlacesUtils.history.clear(); }); diff --git a/toolkit/components/places/tests/unit/test_frecency_interactions.js b/toolkit/components/places/tests/unit/test_frecency_interactions.js @@ -2,9 +2,9 @@ http://creativecommons.org/publicdomain/zero/1.0/ */ /** - * Tests for integrating interaction table data with alternative frecency. - * If the interaction data is considered "interesting", alternative frecency - * should experience a small boost. + * Tests for integrating interaction table data with interactions based + * frecency. If the interaction data is considered "interesting", frecency + * should experience a boost. * * Since we don't know the precise values of the score, the tests typically * have a pattern of caching a baseline without interactions and then checking @@ -18,24 +18,24 @@ // places, the data is stored in milliseconds. const VIEWTIME_THRESHOLD = Services.prefs.getIntPref( - "places.frecency.pages.alternative.interactions.viewTimeSeconds" + "places.frecency.pages.interactions.viewTimeSeconds" ) * 1000; const MANY_KEYPRESSES_THRESHOLD = Services.prefs.getIntPref( - "places.frecency.pages.alternative.interactions.manyKeypresses" + "places.frecency.pages.interactions.manyKeypresses" ); // The Viewtime Threshold for Keypresses preference is stored in seconds. // When recorded in places, the data is stored in milliseconds. const VIEWTIME_IF_MANY_KEYPRESSES_THRESHOLD = Services.prefs.getIntPref( - "places.frecency.pages.alternative.interactions.viewTimeIfManyKeypressesSeconds" + "places.frecency.pages.interactions.viewTimeIfManyKeypressesSeconds" ) * 1000; const SAMPLED_VISITS_THRESHOLD = Services.prefs.getIntPref( - "places.frecency.pages.alternative.numSampledVisits" + "places.frecency.pages.numSampledVisits" ); const MAX_VISIT_GAP = Services.prefs.getIntPref( - "places.frecency.pages.alternative.interactions.maxVisitGapSeconds" + "places.frecency.pages.interactions.maxVisitGapSeconds" ); async function insertIntoMozPlacesMetadata( @@ -109,7 +109,6 @@ async function insertIntoMozPlaces({ url_hash, origin_id, frecency, - alt_frecency, }) { await PlacesUtils.withConnectionWrapper( "test_frecency_interactions::insertIntoMozPlaces", @@ -121,15 +120,13 @@ async function insertIntoMozPlaces({ guid, url_hash, origin_id, - frecency, - alt_frecency + frecency ) VALUES ( :url, :guid, :url_hash, :origin_id, - :frecency, - :alt_frecency + :frecency ) `, { @@ -138,7 +135,6 @@ async function insertIntoMozPlaces({ url_hash, origin_id, frecency, - alt_frecency, } ); } @@ -158,8 +154,6 @@ async function getPageWithUrl(url) { title: r.getResultByName("title"), frecency: r.getResultByName("frecency"), recalc_frecency: r.getResultByName("recalc_frecency"), - alt_frecency: r.getResultByName("alt_frecency"), - recalc_alt_frecency: r.getResultByName("recalc_alt_frecency"), }))[0]; } @@ -218,18 +212,18 @@ add_task(async function one_visit_one_matching_interaction() { await PlacesTestUtils.addVisits([url]); let page = await getPageWithUrl(url); - let oldAltFrecency = page.alt_frecency; + let oldFrecency = page.frecency; Assert.notEqual( - oldAltFrecency, - null, - "Alt frecency with a visit but no interaction should not be null." + oldFrecency, + 0, + "Frecency with a visit but no interaction should not be zero." ); await PlacesFrecencyRecalculator.recalculateAnyOutdatedFrecencies(); Assert.equal( - page.alt_frecency, - oldAltFrecency, - `Alt frecency of ${url} should not have changed.` + page.frecency, + oldFrecency, + `Frecency of ${url} should not have changed.` ); await insertIntoMozPlacesMetadata(page.id, test.interactionData); @@ -237,15 +231,15 @@ add_task(async function one_visit_one_matching_interaction() { page = await getPageWithUrl(url); if (test.expectIncrease) { Assert.greater( - page.alt_frecency, - oldAltFrecency, - `Alt frecency of ${url} should have increased.` + page.frecency, + oldFrecency, + `Frecency of ${url} should have increased.` ); } else { Assert.equal( - page.alt_frecency, - oldAltFrecency, - `Alt frecency of ${url} should not have changed.` + page.frecency, + oldFrecency, + `Frecency of ${url} should not have changed.` ); } await PlacesUtils.history.clear(); @@ -311,13 +305,13 @@ add_task(async function one_visit_one_non_matching_interaction() { await PlacesTestUtils.addVisits([url]); let page = await getPageWithUrl(url); - let oldAltFrecency = page.alt_frecency; + let oldFrecency = page.frecency; await PlacesFrecencyRecalculator.recalculateAnyOutdatedFrecencies(); Assert.equal( - page.alt_frecency, - oldAltFrecency, - `Alt frecency of ${url} should not have changed.` + page.frecency, + oldFrecency, + `Frecency of ${url} should not have changed.` ); await insertIntoMozPlacesMetadata(page.id, test.interactionData); @@ -326,15 +320,15 @@ add_task(async function one_visit_one_non_matching_interaction() { page = await getPageWithUrl(url); if (test.expectIncrease) { Assert.greater( - page.alt_frecency, - oldAltFrecency, - `Alt frecency of ${url} should have increased.` + page.frecency, + oldFrecency, + `Frecency of ${url} should have increased.` ); } else { Assert.equal( - page.alt_frecency, - oldAltFrecency, - `Alt frecency of ${url} should not have changed.` + page.frecency, + oldFrecency, + `Frecency of ${url} should not have changed.` ); } await PlacesUtils.history.clear(); @@ -348,14 +342,12 @@ add_task(async function zero_visits_one_interaction() { interactionData: { total_view_time: VIEWTIME_THRESHOLD - 1, }, - expectIncrease: false, }, { title: "View time is at threshold", interactionData: { total_view_time: VIEWTIME_THRESHOLD, }, - expectIncrease: true, }, ]; @@ -367,23 +359,21 @@ add_task(async function zero_visits_one_interaction() { url, url_hash: "1234567890", frecency: 0, - alt_frecency: null, }); let page = await getPageWithUrl(url); await PlacesFrecencyRecalculator.recalculateAnyOutdatedFrecencies(); - Assert.equal(page.alt_frecency, null, "Alt frecency is null"); + Assert.equal(page.frecency, 0, "Frecency is zero"); await insertIntoMozPlacesMetadata(page.id, test.interactionData); await PlacesFrecencyRecalculator.recalculateAnyOutdatedFrecencies(); page = await getPageWithUrl(url); - if (test.expectIncrease) { - Assert.notEqual(page.alt_frecency, null, "Alt frecency is non null"); - Assert.greater(page.alt_frecency, 0, "Alt frecency greater than zero"); - } else { - Assert.equal(page.alt_frecency, null, "Alt frecency is null"); - } + Assert.equal( + page.frecency, + 0, + "Frecency remains 0 because frecency was at zero." + ); await PlacesUtils.history.clear(); } @@ -432,53 +422,53 @@ add_task(async function max_samples_threshold() { let page = await getPageWithUrl(url); await PlacesFrecencyRecalculator.recalculateAnyOutdatedFrecencies(); - Assert.notEqual(page.alt_frecency, null, "Alt frecency is non-null"); + Assert.notEqual(page.frecency, 0, "Frecency is non-zero"); - let oldAltFrecency = page.alt_frecency; + let oldFrecency = page.frecency; await insertIntoMozPlacesMetadata(page.id, test.interactionData); await PlacesFrecencyRecalculator.recalculateAnyOutdatedFrecencies(); page = await getPageWithUrl(url); if (test.expectIncrease) { - Assert.notEqual(page.alt_frecency, null, "Alt frecency is non null"); + Assert.notEqual(page.frecency, 0, "Frecency is non zero"); Assert.greater( - page.alt_frecency, - oldAltFrecency, - "Alt frecency greater than the old value." + page.frecency, + oldFrecency, + "Frecency greater than the old value." ); } else { - Assert.equal( - page.alt_frecency, - oldAltFrecency, - "Alt frecency didn't change" - ); + Assert.equal(page.frecency, oldFrecency, "Frecency didn't change"); } await PlacesUtils.history.clear(); } }); -// Verify that the number of interactions contributing to alternative frecency -// is as expected. Avoid using actual visits, ensuring all interactions are -// treated as virtual visits. Each virtual visit should increase the alternative -// frecency score until the threshold is exceeded. Interactions are deliberately -// inserted sequentially in the past, as the date can affect the score. +// Verify that the number of interactions contributing to frecency +// is as expected. Insert an old visit to ensure the URL has a positive +// frecency. Ensure all interactions are treated as virtual visits by making +// them interesting and more recent than the oldest visit. +// Each virtual visit should increase the frecency score until +// the threshold is exceeded. Interactions are deliberately inserted +// sequentially in the past, as the date can affect the score. add_task(async function max_interesting_interactions() { const today = new Date(); + // We deliberately set a visit that can't be grouped with an interaction. + const yearAgo = new Date(); + yearAgo.setMonth(yearAgo.getMonth() - 12); let url = "https://testdomain1.moz.org/"; - await insertIntoMozPlaces({ + await PlacesTestUtils.addVisits({ url, - url_hash: "1234567890", - frecency: 0, - alt_frecency: null, + visitDate: yearAgo.getTime() * 1000, + transition: PlacesUtils.history.TRANSITIONS.LINK, }); let page = await getPageWithUrl(url); await PlacesFrecencyRecalculator.recalculateAnyOutdatedFrecencies(); - Assert.equal(page.alt_frecency, null, "Alt frecency is null"); + Assert.greater(page.frecency, 0, "Frecency is above zero"); - info("Insert interesting interactions until threshold is met."); + info("Insert interesting interactions."); for (let i = 0; i < SAMPLED_VISITS_THRESHOLD; ++i) { let pageBefore = await getPageWithUrl(url); await insertIntoMozPlacesMetadata(page.id, { @@ -488,10 +478,9 @@ add_task(async function max_interesting_interactions() { await PlacesFrecencyRecalculator.recalculateAnyOutdatedFrecencies(); let pageAfter = await getPageWithUrl(url); Assert.greater( - pageAfter.alt_frecency, - // The first run, alt frecency will be null. - pageBefore.alt_frecency ?? 0, - "Alt frecency has increased." + pageAfter.frecency, + pageBefore.frecency, + "Frecency has increased." ); } @@ -505,15 +494,15 @@ add_task(async function max_interesting_interactions() { await PlacesFrecencyRecalculator.recalculateAnyOutdatedFrecencies(); let pageAfter = await getPageWithUrl(url); Assert.equal( - pageBefore.alt_frecency, - pageAfter.alt_frecency, - "Alt frecency is the same." + pageBefore.frecency, + pageAfter.frecency, + "Frecency is the same." ); await PlacesUtils.history.clear(); }); -add_task(async function temp_redirect_alt_frecency() { +add_task(async function temp_redirect_frecency() { const yesterday = new Date(); let url1 = "http://testdomain1.moz.org/"; @@ -544,13 +533,13 @@ add_task(async function temp_redirect_alt_frecency() { await PlacesFrecencyRecalculator.recalculateAnyOutdatedFrecencies(); let page1 = await getPageWithUrl(url1); - Assert.notEqual(page1.alt_frecency, null, "Alt frecency is non-null"); + Assert.notEqual(page1.frecency, 0, "Frecency is non-zero"); let page2 = await getPageWithUrl(url2); - Assert.notEqual(page2.alt_frecency, null, "Alt frecency is non-null"); + Assert.notEqual(page2.frecency, 0, "Frecency is non-zero"); let page3 = await getPageWithUrl(url3); - Assert.notEqual(page3.alt_frecency, null, "Alt frecency is non-null"); + Assert.notEqual(page3.frecency, 0, "Frecency is non-zero"); info("For each visit, add an interesting interaction."); const created_at = yesterday.getTime(); @@ -574,25 +563,13 @@ add_task(async function temp_redirect_alt_frecency() { await PlacesFrecencyRecalculator.recalculateAnyOutdatedFrecencies(); let newPage1 = await getPageWithUrl(url1); - Assert.equal( - newPage1.alt_frecency, - page1.alt_frecency, - "Alt frecency is the same" - ); + Assert.equal(newPage1.frecency, page1.frecency, "Frecency is the same"); let newPage2 = await getPageWithUrl(url2); - Assert.equal( - newPage2.alt_frecency, - page2.alt_frecency, - "Alt frecency is the same" - ); + Assert.equal(newPage2.frecency, page2.frecency, "Frecency is the same"); let newPage3 = await getPageWithUrl(url3); - Assert.greater( - newPage3.alt_frecency, - page3.alt_frecency, - "Alt frecency has increased" - ); + Assert.greater(newPage3.frecency, page3.frecency, "Frecency has increased"); await PlacesUtils.history.clear(); }); @@ -615,7 +592,7 @@ add_task(async function interaction_visit_gap() { await PlacesFrecencyRecalculator.recalculateAnyOutdatedFrecencies(); let page = await getPageWithUrl(url); - Assert.notEqual(page.alt_frecency, null, "Alt frecency is not null"); + Assert.notEqual(page.frecency, 0, "Frecency is not zero"); info("Add an interaction just below the visit gap."); await insertIntoMozPlacesMetadata(page.id, { @@ -626,11 +603,7 @@ add_task(async function interaction_visit_gap() { await PlacesFrecencyRecalculator.recalculateAnyOutdatedFrecencies(); let updatedPage = await getPageWithUrl(url); - Assert.equal( - updatedPage.alt_frecency, - page.alt_frecency, - "Alt frecency didn't change." - ); + Assert.equal(updatedPage.frecency, page.frecency, "Frecency didn't change."); info("Add an interaction at the visit gap."); await insertIntoMozPlacesMetadata(page.id, { @@ -641,16 +614,12 @@ add_task(async function interaction_visit_gap() { await PlacesFrecencyRecalculator.recalculateAnyOutdatedFrecencies(); updatedPage = await getPageWithUrl(url); - Assert.greater( - updatedPage.alt_frecency, - page.alt_frecency, - "Alt frecency increased." - ); + Assert.greater(updatedPage.frecency, page.frecency, "Frecency increased."); await PlacesUtils.history.clear(); }); -add_task(async function old_visits_not_null() { +add_task(async function old_visits_not_zero() { let testCases = [ { daysOld: 365, description: "1 year old" }, { daysOld: 1825, description: "5 years old" }, @@ -672,13 +641,13 @@ add_task(async function old_visits_not_null() { let page = await getPageWithUrl(url); Assert.notEqual( - page.alt_frecency, - null, - `Frecency should not be NULL for ${testCase.description}` + page.frecency, + 0, + `Frecency should not be zero for ${testCase.description}` ); Assert.greater( - page.alt_frecency, + page.frecency, 0, `Frecency should be positive for ${testCase.description}` ); @@ -698,13 +667,9 @@ add_task(async function old_visits_new_interactions() { await PlacesFrecencyRecalculator.recalculateAnyOutdatedFrecencies(); let page = await getPageWithUrl(url); - let baselineFrecency = page.alt_frecency; + let baselineFrecency = page.frecency; - Assert.notEqual( - baselineFrecency, - null, - "Baseline frecency should not be NULL." - ); + Assert.notEqual(baselineFrecency, 0, "Baseline frecency should not be zero."); Assert.greater( baselineFrecency, 0, @@ -720,7 +685,7 @@ add_task(async function old_visits_new_interactions() { page = await getPageWithUrl(url); Assert.greater( - page.alt_frecency, + page.frecency, baselineFrecency, "Recent interaction should boost frecency of page with really old visit." ); diff --git a/toolkit/components/places/tests/unit/test_frecency_interactions_trigger_recalc.js b/toolkit/components/places/tests/unit/test_frecency_interactions_trigger_recalc.js @@ -4,15 +4,15 @@ https://creativecommons.org/publicdomain/zero/1.0/ */ "use strict"; const VIEWTIME_THRESHOLD_SECONDS = Services.prefs.getIntPref( - "places.frecency.pages.alternative.interactions.viewTimeSeconds" + "places.frecency.pages.interactions.viewTimeSeconds" ); const VIEWTIME_IF_MANY_KEYPRESSES_THRESHOLD_SECONDS = Services.prefs.getIntPref( - "places.frecency.pages.alternative.interactions.viewTimeIfManyKeypressesSeconds" + "places.frecency.pages.interactions.viewTimeIfManyKeypressesSeconds" ); const MANY_KEYPRESSES_THRESHOLD = Services.prefs.getIntPref( - "places.frecency.pages.alternative.interactions.manyKeypresses" + "places.frecency.pages.interactions.manyKeypresses" ); async function getPageWithUrl(url) { @@ -29,7 +29,6 @@ async function getPageWithUrl(url) { frecency: r.getResultByName("frecency"), recalc_frecency: r.getResultByName("recalc_frecency"), alt_frecency: r.getResultByName("alt_frecency"), - recalc_alt_frecency: r.getResultByName("recalc_alt_frecency"), }))[0]; } @@ -187,13 +186,9 @@ add_task(async function test_insertion_triggers() { page = await getPageWithUrl(url); if (test.expectRecalc) { - Assert.equal(page.recalc_alt_frecency, 1, "Should recalc alt frecency."); + Assert.equal(page.recalc_frecency, 1, "Should recalc frecency."); } else { - Assert.equal( - page.recalc_alt_frecency, - 0, - "Should not recalc alt frecency." - ); + Assert.equal(page.recalc_frecency, 0, "Should not recalc frecency."); } await PlacesUtils.history.clear(); @@ -321,29 +316,21 @@ add_task(async function test_update_triggers_to_recalc() { let page = await getPageWithUrl(url); await insertIntoMozPlacesMetadata(page.id, test.insert.interaction); page = await getPageWithUrl(url); - Assert.equal( - page.recalc_alt_frecency, - 0, - "Should not recalc alt frecency." - ); + Assert.equal(page.recalc_frecency, 0, "Should not recalc frecency."); await updateMozPlacesMetadata(1, test.update.interaction); page = await getPageWithUrl(url); if (test.expectRecalc) { - Assert.equal(page.recalc_alt_frecency, 1, "Should recalc alt frecency."); + Assert.equal(page.recalc_frecency, 1, "Should recalc frecency."); } else { - Assert.equal( - page.recalc_alt_frecency, - 0, - "Should not recalc alt frecency." - ); + Assert.equal(page.recalc_frecency, 0, "Should not recalc frecency."); } await PlacesUtils.history.clear(); } }); -// Once we've recalculated alternative frecency, don't recalculate it again +// Once we've recalculated frecency, don't recalculate it again // upon further updates to the same interaction. add_task(async function test_no_additional_recalc() { const TEST_CASES = [ @@ -404,23 +391,15 @@ add_task(async function test_no_additional_recalc() { let page = await getPageWithUrl(url); await insertIntoMozPlacesMetadata(page.id, test.insert.interaction); page = await getPageWithUrl(url); - Assert.equal(page.recalc_alt_frecency, 1, "Should recalc alt frecency."); + Assert.equal(page.recalc_frecency, 1, "Should recalc frecency."); await PlacesFrecencyRecalculator.recalculateAnyOutdatedFrecencies(); page = await getPageWithUrl(url); - Assert.equal( - page.recalc_alt_frecency, - 0, - "Should not recalc alt frecency." - ); + Assert.equal(page.recalc_frecency, 0, "Should not recalc frecency."); await updateMozPlacesMetadata(1, test.update.interaction); page = await getPageWithUrl(url); - Assert.equal( - page.recalc_alt_frecency, - 0, - "Should not recalc alt frecency." - ); + Assert.equal(page.recalc_frecency, 0, "Should not recalc frecency."); await PlacesUtils.history.clear(); } diff --git a/toolkit/components/places/tests/unit/xpcshell.toml b/toolkit/components/places/tests/unit/xpcshell.toml @@ -116,10 +116,8 @@ run-if = ["buildapp == 'browser'"] ["test_childlessTags.js"] ["test_frecency_interactions.js"] -prefs = ["places.frecency.pages.alternative.featureGate=true"] ["test_frecency_interactions_trigger_recalc.js"] -prefs = ["places.frecency.pages.alternative.featureGate=true"] ["test_frecency_observers.js"]