Closed Bug 1806159 Opened 3 years ago Closed 1 year ago

Create tests for default bookmarks

Categories

(Firefox :: Migration, task, P3)

Product:
Firefox
Component:
Migration
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
134 Branch
Tracking Flags:
Tracking Status
firefox134 --- fixed

People

(Reporter: kpatenio, Assigned: rking, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file)

Bug 1806159 - Create tests for default bookmarks - r=kpatenio
48 bytes, text/x-phabricator-request
Details | Review

We currently don't have any tests verifying default bookmarks. We'd likely need marionette-like tests that start up the browser from a fresh browser.

Update: I added more details below.

--

Some notes about default bookmarks:

  • The file containing default bookmarks is in browser/base/content/default-bookmarks.html. The default bookmarks differ depending on whether we're loading a nightly, beta, or release version of Firefox. (For context, see our release schedule).
  • On Firefox, default bookmarks are usually displayed on the Bookmarks Toolbar (ex. "Get Involved" is one of the default bookmarks on Nightly)
  • For this Bugzilla ticket, let's try to add a test to verify bookmarks according to Firefox version. We can use AppConstants in our test file to determine what version of Firefox we're using.
  • There are other tests that verify default bookmarks (ex. see browser/components/places/tests/unit/test_browserGlue_corrupt_nobackup_default.js). They however don't check default bookmarks for other Firefox versions, which is what we want to do.

Some steps for guidance:

  • For more information about writing tests, see this Firefox Source Docs article on testing.
  • As for where to add the test, maybe adding it here would be a good start.
  • We can fetch bookmarks from the toolbar by calling await PlacesUtils.bookmarks.fetch with parentGuid as PlacesUtils.bookmarks.toolbarGuid. Use searchfox to find examples.
  • Also see this example of forced import for default bookmarks by Marco below
Severity: -- → N/A
Priority: -- → P3

Hi Katherine, do you think this could make a good first bug? If so, would you be willing to add next steps and/or be a mentor?

Flags: needinfo?(kpatenio)

Hi Cieara, I think this could be a suitable good first bug. I'll mark it as such and add more details. Thanks for asking!

Flags: needinfo?(kpatenio)
Keywords: good-first-bug
Mentor: kpatenio
Assignee: nobody → rking

I have this so far, and am currently trying to figure out how to retrieve the expected bookmarks list for each build type. I just wanted to check in to make sure I'm on the right track:


add_task(async function checkBookmarks() {
    let nightlyBookmarks;
    let betaBookmarks;
    let releaseBookmarks;

    let bookmarkList = await PlacesUtils.bookmarks.fetch({
            parentGuid: PlacesUtils.bookmarks.toolbarGuid,
            index: 0,
        }
    );

    if(AppConstants.NIGHTLY_BUILD) {
        Assert.equal(bookmarkList, nightlyBookmarks, 'Should return default bookmark list for the nightly build');
    } else if (AppConstants.EARLY_BETA_OR_EARLIER) {
        Assert.equal(bookmarkList, betaBookmarks, 'Should return default bookmark list for the beta build');
    } else {
        Assert.equal(bookmarkList, releaseBookmarks, 'Should return default bookmark list for the release build');
    }
})

(In reply to Becca from comment #4)

I have this so far, and am currently trying to figure out how to retrieve the expected bookmarks list for each build type. I just wanted to check in to make sure I'm on the right track:


add_task(async function checkBookmarks() {
    let nightlyBookmarks;
    let betaBookmarks;
    let releaseBookmarks;

    let bookmarkList = await PlacesUtils.bookmarks.fetch({
            parentGuid: PlacesUtils.bookmarks.toolbarGuid,
            index: 0,
        }
    );

    if(AppConstants.NIGHTLY_BUILD) {
        Assert.equal(bookmarkList, nightlyBookmarks, 'Should return default bookmark list for the nightly build');
    } else if (AppConstants.EARLY_BETA_OR_EARLIER) {
        Assert.equal(bookmarkList, betaBookmarks, 'Should return default bookmark list for the beta build');
    } else {
        Assert.equal(bookmarkList, releaseBookmarks, 'Should return default bookmark list for the release build');
    }
})

Hi rking, thanks for looking into this bug. I think you're on the right track of fetching bookmarks from PlacesUtils and then comparing the results according to the current release version. I've however noticed that the latest versions of Firefox no longer display the default bookmarks on the toolbar, and that the defaults are now found in the Bookmarks Menu. It turns out that a patch for Bug 1904630 removed the defaults from the toolbar sometime ago.

Despite this, we could still probably add a test:

  • We could look at the guid PlacesUtils.bookmarks.menuGuid instead, which I believe holds the default bookmarks folder found in the Bookmarks menu. From there, we could try counting the number of children in this folder.
  • We could also try verifying the individual bookmarks themselves. One way to do so is by passing in the bookmark url to PlacesUtils.bookmarks.fetch and validating if we get a null result or not

Actually, let's check in first on the state of default bookmarks. Meg, is your team planning to remove default bookmarks entirely? Or do the messaging changes concern only the ones found previously in the toolbar? Just wanted to make sure, since I didn't see any mention of the default bookmarks outside the toolbar being affected after a quick skim through the technical doc for Bug 1901261.

Flags: needinfo?(mviar)

Hi @kpatenio, our changes only concern the ones formerly in the toolbar.

Flags: needinfo?(mviar)

I updated the test to check for specific urls based on the default bookmarks file, like so:


add_task(async function checkBookmarks() {
    if(AppConstants.NIGHTLY_BUILD) {
        Assert.ok(await PlacesUtils.bookmarks.fetch({url: "https://www.mozilla.org/contribute/?utm_medium=firefox-desktop&utm_source=bookmarks-toolbar&utm_campaign=new-users-nightly&utm_content=-global"}), "Nightly contribute link verified");
        Assert.ok(await PlacesUtils.bookmarks.fetch({url: "https://blog.nightly.mozilla.org/"}), "Nightly blog link verified");
        Assert.ok(await PlacesUtils.bookmarks.fetch({url: "https://bugzilla.mozilla.org/"}), "Nightly bugzilla link verified");
        Assert.ok(await PlacesUtils.bookmarks.fetch({url: "https://developer.mozilla.org/"}), "Nightly developer link verified");
        Assert.ok(await PlacesUtils.bookmarks.fetch({url: "https://addons.mozilla.org/firefox/addon/nightly-tester-tools/"}), "Nightly tester tools addon verified");
        Assert.ok(await PlacesUtils.bookmarks.fetch({url: "about:crashes"}), "Nightly about:crashes link verified");
        Assert.ok(await PlacesUtils.bookmarks.fetch({url: "https://planet.mozilla.org/"}), "Nightly planet mozilla link verified");
    } else if (AppConstants.EARLY_BETA_OR_EARLIER) {
        Assert.ok(await PlacesUtils.bookmarks.fetch({url: "https://support.mozilla.org/products/firefox"}), "Beta firefox support link verified");
        Assert.ok(await PlacesUtils.bookmarks.fetch({url: "https://support.mozilla.org/kb/customize-firefox-controls-buttons-and-toolbars?utm_source=firefox-browser&utm_medium=default-bookmarks&utm_campaign=customize"}), "Beta customize firefox link verified");
        Assert.ok(await PlacesUtils.bookmarks.fetch({url: "https://www.mozilla.org/contribute/?utm_medium=firefox-desktop&utm_source=bookmarks-toolbar&utm_campaign=new-users-beta&utm_content=-global"}), "Beta contribute link verified");
    } else {
        //Release
        Assert.ok(await PlacesUtils.bookmarks.fetch({url: "https://support.mozilla.org/products/firefox"}), "Release firefox support link verified");
        Assert.ok(await PlacesUtils.bookmarks.fetch({url: "https://support.mozilla.org/kb/customize-firefox-controls-buttons-and-toolbars?utm_source=firefox-browser&utm_medium=default-bookmarks&utm_campaign=customize"}), "Release customize firefox link verified");
        Assert.ok(await PlacesUtils.bookmarks.fetch({url: "https://www.mozilla.org/contribute/"}), "Release contribute link verified");
        Assert.ok(await PlacesUtils.bookmarks.fetch({url: "https://www.mozilla.org/about/"}), "Release about mozilla link verified");
    }
})

But when I run the test, I'm not sure how to check for each release version

(In reply to Meg Viar [:mviar] from comment #7)

Hi @kpatenio, our changes only concern the ones formerly in the toolbar.

Thanks. I'd say we're safe with testing the defaults in the bookmark menu then.

(In reply to Becca from comment #8)

I updated the test to check for specific urls based on the default bookmarks file, like so:
...
But when I run the test, I'm not sure how to check for each release version

Admittedly, I never thought that far ahead when I originally wrote out this bug. :D

Not sure if this is the best way, but I suppose you could try running a simulation: https://wiki.mozilla.org/Sheriffing/How_To/Beta_simulations. You will probably want 4 different pushes, one for each build:

  1. Nightly
  2. Early beta
  3. Late beta
  4. Release

I ran simulations for these builds when I worked on https://phabricator.services.mozilla.com/D164610. It's however been some time (2 years ago!) and I only used them to verify changes in the build. I think we can verify tests as well using simulations. In my opinion, it's regardless still worth looking into.

Also feel free to submit your current changes on phabricator as a WIP patch. Doing so will make it easier to see your code and allow me to also pull them down if I need to test things locally.

I started running the test for the Nightly build, and I'm getting a null result whether I use the parent guid or the exact url. I checked the bookmarks menu, and the expected list is there- not sure why it's coming back null.

I moved the test to browser/components/places/tests/browser to get an initialization of the browser in the test, and I have the Nightly tests passing now. Pushed my latest to phabricator and going to try simulating the other build types.

I'm having some trouble figuring out the beta simulations. The docs say not to create central-as-beta simulations as artifact builds, and I currently only have an artifact build- do you know if I need to complete a full build before trying to run any simulations, or is that not what that means?

Flags: needinfo?(kpatenio)

It means you should pass --no-artifact to mach try.

We determined that there's a way to run the simulations locally (see https://firefox-source-docs.mozilla.org/setup/configuring_build_options.html#building-as-beta-or-release), although requiring a full build.

Patch is now r+ed and ready to land.

Flags: needinfo?(kpatenio)
Pushed by kpatenio@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/296f2bee3e80 Create tests for default bookmarks - r=kpatenio,places-reviewers,lina

(In reply to Sandor Molnar[:smolnar] from comment #17)

Backed out for causing perma bc failures @ browser_bookmarks_checkDefaultBookmarks.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/0ff1f7cd1637eeecfe4c913094766c5f6a897eff

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_bookmarks_checkDefaultBookmarks.js

Update pushed to phabricator for review

Flags: needinfo?(rking)
Pushed by kpatenio@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/007ba6522aad Create tests for default bookmarks - r=kpatenio,places-reviewers,lina

Backed out for causing bc leakcheck failures on browser.toml

Flags: needinfo?(rking)
Flags: needinfo?(kpatenio)

I looked a little at this leak. I was able to reproduce it locally on a MacOS debug build. The notable thing in the set of leaked objects (besides the BackstagePass, which is a chrome JS global) is the nsUserIdleService.

The leaked window is being held alive via a nsXPCWrappedJS (nsIObserver) that is located in the BackstagePass global. Based on the fields of the nsIObserver object (like _intervalWhileActive and _intervalWhileIdle), it appears to be the SessionSaverInternal object. That does get registered with the idle service and not removed, but I have no idea why there's a leak during this test but not normally.

I did notice that the only other use of test-force-places-init is in a unit test (which I think means XPCShell) but this is a browser test so maybe that is causing problems. Hopefully somebody who is more familiar with places can figure out what the problem is from there.

test-force-places-init will actually force a second initialization that means this is called twice https://searchfox.org/mozilla-central/rev/4ac5b1f6cc9d8c186986f52369a2d9a537d10474/browser/components/BrowserGlue.sys.mjs#3694 the idl says you can add the same observer twice at https://searchfox.org/mozilla-central/rev/4ac5b1f6cc9d8c186986f52369a2d9a537d10474/widget/nsIUserIdleService.idl#48 that to me sounds like it's adding 2 observers (the code seems to confirm that).
Though only one idle observer is removed at shutdown.

in xpcshell the idle service is a mock by default, iirc.

(In reply to Marco Bonardo [:mak] from comment #22)

test-force-places-init will actually force a second initialization that means this is called twice https://searchfox.org/mozilla-central/rev/4ac5b1f6cc9d8c186986f52369a2d9a537d10474/browser/components/BrowserGlue.sys.mjs#3694 the idl says you can add the same observer twice at https://searchfox.org/mozilla-central/rev/4ac5b1f6cc9d8c186986f52369a2d9a537d10474/widget/nsIUserIdleService.idl#48 that to me sounds like it's adding 2 observers (the code seems to confirm that).
Though only one idle observer is removed at shutdown.

in xpcshell the idle service is a mock by default, iirc.

Does that mean that this part of the test should be removed? I copied the simulatePlacesInit from browser/components/places/tests/unit/test_browserGlue_prefs.js, and I copied promiseTopicObserved from toolkit/components/places/tests/head_common.js. I'm not sure what's safe to remove of that. Or maybe the topic and topic data need to be changed?

Flags: needinfo?(rking) → needinfo?(mak)

(In reply to Becca from comment #23)

Does that mean that this part of the test should be removed? I copied the simulatePlacesInit from browser/components/places/tests/unit/test_browserGlue_prefs.js, and I copied promiseTopicObserved from toolkit/components/places/tests/head_common.js. I'm not sure what's safe to remove of that. Or maybe the topic and topic data need to be changed?

If you need to simulate multiple restore of bookmarks on startup, you'll have to change the browserGlue code so that it can effectively run multiple times. It is not expecting to be run like that, so it's adding the idle observer twice (or more), but removing it only once.
That may be as easy as adding to browserGlue a boolean property tracking if the idle observer was already added, and don't add it twice. And maybe add a comment above _initPlaces to tell the method should handle the possibility of being executed multiple times by tests.

Flags: needinfo?(mak)

I need a little more guidance on to how to implement that. Can you point me to an example of something that works similarly?

Flags: needinfo?(mak)

See _placesBrowserInitComplete in BrowserGlue, imagine adding another boolean property like _isObservingIdle, then you can change code to

if (!this._isObservingIdle) {
  this._userIdleService.addIdleObserver(this, idleTime);
  this._isObservingIdle = true
}

The second time you'll pass through this code, the boolean is set and you won't add another observer.

Flags: needinfo?(mak)
Pushed by kpatenio@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5dd59f9c63db Create tests for default bookmarks - r=kpatenio,places-reviewers,lina,firefox-desktop-core-reviewers ,mak

https://hg.mozilla.org/mozilla-central/rev/5dd59f9c63db

Status: NEW → RESOLVED
Closed: 1 year ago
status-firefox134: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 134 Branch
Flags: needinfo?(kpatenio)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: