Closed Bug 1257380 Opened 4 years ago Closed 4 years ago

Intermittent test_ext_bookmarks.html | Bookmark has the expected title - Expected: Toolbar Item, Actual: MØzillä

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
48.1 - Mar 21
Tracking Status
firefox48 --- fixed

People

(Reporter: KWierso, Assigned: bsilverberg)

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.1 - Mar 21
I don't think we can rely on the bookmarks being created in a specific order, although that mostly seems to happen,
so rather than check for an exact list of bookmarks returned from getRecent(), we can just check that the bookmarks
we get back are in reverse chronological order and that the most recently created bookmark is in there.

Review commit: https://reviewboard.mozilla.org/r/40757/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40757/
Attachment #8731628 - Flags: review?(kmaglione+bmo)
Oops, I addressed this yesterday but neglected to mark anyone as a reviewer. Just fixed that.
Attachment #8731628 - Flags: review?(kmaglione+bmo)
Comment on attachment 8731628 [details]
MozReview Request: Bug 1257380 - Fix Intermittent test_ext_bookmarks.html, r?kmag

https://reviewboard.mozilla.org/r/40757/#review37651

::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:404
(Diff revision 1)
>    }).then(results => {
>      browser.test.assertEq(5, results.length, "Expected number of results returned by getRecent");
> -    browser.test.assertEq("About Mozilla", results[0].title, "Bookmark has the expected title");
> -    browser.test.assertEq("Firefox", results[1].title, "Bookmark has the expected title");
> -    browser.test.assertEq("Mozilla Corporation", results[2].title, "Bookmark has the expected title");
> -    browser.test.assertEq("Mozilla", results[3].title, "Bookmark has the expected title");
> +    let mostRecentBookmarkFound = false;
> +    let prevDate = results[0].dateAdded;
> +    for (let bookmark of results) {
> +      if (bookmark.title === "About Mozilla") mostRecentBookmarkFound = true;

ESLint will reject this line for multiple reasons.

::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:408
(Diff revision 1)
> -    browser.test.assertEq("Mozilla Corporation", results[2].title, "Bookmark has the expected title");
> -    browser.test.assertEq("Mozilla", results[3].title, "Bookmark has the expected title");
> -    browser.test.assertEq("Toolbar Item", results[4].title, "Bookmark has the expected title");
> +    for (let bookmark of results) {
> +      if (bookmark.title === "About Mozilla") mostRecentBookmarkFound = true;
> +      browser.test.assertTrue(bookmark.dateAdded <= prevDate, "The recent bookmarks are sorted by dateAdded");
> +      prevDate = bookmark.dateAdded;
> +    }
> +    browser.test.assertTrue(mostRecentBookmarkFound, "The most recently added bookmark was returned");

Let's just keep the previous tests, but sort by title beforehand.
Comment on attachment 8731628 [details]
MozReview Request: Bug 1257380 - Fix Intermittent test_ext_bookmarks.html, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40757/diff/1-2/
Attachment #8731628 - Attachment description: MozReview Request: Bug 1257380 - Fix Intermittent test_ext_bookmarks.html → MozReview Request: Bug 1257380 - Fix Intermittent test_ext_bookmarks.html, r?kmag
Attachment #8731628 - Flags: review?(kmaglione+bmo)
https://reviewboard.mozilla.org/r/40757/#review37651

> ESLint will reject this line for multiple reasons.

FTR `mach eslint` does not have any problems with it, but I guess that's a limitation of the rules defined.

> Let's just keep the previous tests, but sort by title beforehand.

This won't address the issue because we don't know which 5 bookmarks will be returned. In the intermittent failure a bookmark with the title of `MØzillä` was found, but we did not expect that. I believe it is happening because the bookmarks added with `Promise.all` do not always end up getting added in the order in which we wrote the promises, which makes sense. This is why I chose to verify that a) the bookmarks are sorted in reverse chronological order and b) we at least got the last bookmark that we added, which should be guaranteed as it is added individually (not as part of a `Promise.all`). If I change the test to only check the most recent 4 bookmarks then sorting by title will work, so I will do that. I just wanted to explain why I am doing that prior to submitting the change.
Note also that I am currently on PTO and will be out of the country for a week starting tomorrow morning, so if we can get this finished up today (Saturday) that would be ideal, as I assume we don't want to leave an intermittent hanging around for a week. If it's not possible to finish today, and it's important that it be resolved before the end of next week, feel free to take it from me and assign it to someone else.
https://reviewboard.mozilla.org/r/40757/#review37651

> FTR `mach eslint` does not have any problems with it, but I guess that's a limitation of the rules defined.

Humph. I'll fix that.

> This won't address the issue because we don't know which 5 bookmarks will be returned. In the intermittent failure a bookmark with the title of `MØzillä` was found, but we did not expect that. I believe it is happening because the bookmarks added with `Promise.all` do not always end up getting added in the order in which we wrote the promises, which makes sense. This is why I chose to verify that a) the bookmarks are sorted in reverse chronological order and b) we at least got the last bookmark that we added, which should be guaranteed as it is added individually (not as part of a `Promise.all`). If I change the test to only check the most recent 4 bookmarks then sorting by title will work, so I will do that. I just wanted to explain why I am doing that prior to submitting the change.

I don't think that means it's guaranteed, just less likely be wrong. But I guess it's fine.
Comment on attachment 8731628 [details]
MozReview Request: Bug 1257380 - Fix Intermittent test_ext_bookmarks.html, r?kmag

https://reviewboard.mozilla.org/r/40757/#review37761

::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:412
(Diff revisions 1 - 2)
> +      return a.title.localeCompare(b.title);
> +    });
> +    browser.test.assertEq("About Mozilla", bookmarksByTitle[0].title, "Bookmark has the expected title");
> +    browser.test.assertEq("Firefox", bookmarksByTitle[1].title, "Bookmark has the expected title");
> +    browser.test.assertEq("Mozilla", bookmarksByTitle[2].title, "Bookmark has the expected title");
> +    browser.test.assertEq("Mozilla Corporation", bookmarksByTitle[3].title, "Bookmark has the expected title");

I don't think this is going to work, given your comment in the last revision. If a fifth, unexpected bookmark winds up in this set, it's going could wind up anywhere in the list.

So, maybe just query the 4 most recent bookmarks, rather than 5?
Attachment #8731628 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8731628 [details]
MozReview Request: Bug 1257380 - Fix Intermittent test_ext_bookmarks.html, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40757/diff/2-3/
https://reviewboard.mozilla.org/r/40757/#review37761

> I don't think this is going to work, given your comment in the last revision. If a fifth, unexpected bookmark winds up in this set, it's going could wind up anywhere in the list.
> 
> So, maybe just query the 4 most recent bookmarks, rather than 5?

Ah, I see the problem. I will change it to only ask for the most recent 4 bookmarks.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c37b2bad11ed
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.