Closed
Bug 1257380
Opened 8 years ago
Closed 8 years ago
Intermittent test_ext_bookmarks.html | Bookmark has the expected title - Expected: Toolbar Item, Actual: MØzillä
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox48 fixed)
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: KWierso, Assigned: bsilverberg)
Details
(Keywords: intermittent-failure)
Attachments
(1 file)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.1 - Mar 21
Assignee | ||
Comment 1•8 years ago
|
||
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/
Assignee | ||
Updated•8 years ago
|
Attachment #8731628 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 2•8 years ago
|
||
Oops, I addressed this yesterday but neglected to mark anyone as a reviewer. Just fixed that.
Updated•8 years ago
|
Attachment #8731628 -
Flags: review?(kmaglione+bmo)
Comment 3•8 years ago
|
||
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.
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
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.
Assignee | ||
Comment 6•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41ee887da515
Assignee | ||
Comment 7•8 years ago
|
||
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.
Comment 8•8 years ago
|
||
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 9•8 years ago
|
||
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+
Assignee | ||
Comment 10•8 years ago
|
||
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/
Assignee | ||
Comment 11•8 years ago
|
||
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.
Assignee | ||
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3824a63f87cd
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/c37b2bad11ed
Keywords: checkin-needed
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c37b2bad11ed
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•