Closed Bug 1419346 Opened 7 years ago Closed 7 years ago

browser_favicon_load.js doesn't pass locally

Categories

(Core :: Networking, enhancement, P3)

Product:
Core
Component:
Networking
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: florian, Assigned: kershaw)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file, 5 obsolete files)

Move browser_favicon_load.js to favicon folder, r=florian
(deleted), patch
kershaw
: review+
Details | Diff | Splinter Review
Bug 1247843 introduced the browser_favicon_load.js test in the performance folder. This test really doesn't look like a performance test. Given its name, it looks like it was meant to go in the favicon folder. Additionally, this test prevents running the other tests of the performance folder locally because it has undeclared dependencies on support files from the browser/components/originattributes/ folder. Due to this, currently the only way to run locally the performance test is to run first: ./mach test browser/components/originattributes/
Flags: needinfo?(kechang)
Assignee: nobody → kechang
Priority: -- → P3
Whiteboard: [necko-triaged]
(In reply to Florian Quèze [:florian] from comment #0) > Bug 1247843 introduced the browser_favicon_load.js test in the performance > folder. This test really doesn't look like a performance test. Given its > name, it looks like it was meant to go in the favicon folder. > > Additionally, this test prevents running the other tests of the performance > folder locally because it has undeclared dependencies on support files from > the browser/components/originattributes/ folder. > > Due to this, currently the only way to run locally the performance test is > to run first: > ./mach test browser/components/originattributes/ Sorry for the delay. I'll try to fix this soon.
Flags: needinfo?(kechang)
Attached patch Part1: Move the test file to favicons folder (obsolete) (deleted) — — Splinter Review
Attachment #8934378 - Flags: review?(florian)
Attached patch Part2: Modify the paths in test file (obsolete) (deleted) — — Splinter Review
Summary: - Copy some necessary support files to favicon folder. - Modify the file paths in browser_favicon_load.js. Hi Florian, Could you take a look at these patches? Thanks.
Attachment #8934379 - Flags: review?(florian)
Comment on attachment 8934378 [details] [diff] [review] Part1: Move the test file to favicons folder Review of attachment 8934378 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks like you are deleting and then re-adding the file instead of moving it.
Attachment #8934378 - Flags: review?(florian) → review-
Comment on attachment 8934379 [details] [diff] [review] Part2: Modify the paths in test file Review of attachment 8934379 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/favicons/browser.ini @@ +9,5 @@ > rich_moz_2.png > + file_favicon.html > + file_favicon.png > + file_favicon.png^headers^ > + file_favicon_thirdParty.html Did you forget to hg copy these files in the favicons folder?
Attachment #8934379 - Flags: review?(florian) → review-
Attached patch Part1: Move the test file to favicons folder (obsolete) (deleted) — — Splinter Review
Obviously I can't use git to export my patch, so using hg instead.
Attachment #8934378 - Attachment is obsolete: true
Attachment #8934379 - Attachment is obsolete: true
Attachment #8934778 - Flags: review?(florian)
Attached patch Part2: Modify the paths in test file (obsolete) (deleted) — — Splinter Review
Summary: - Use "hg copy" for file_favicon.html file_favicon.png, and file_favicon.png^headers^. - Note that the favicon path in file_favicon_thirdParty.html is different than the original one, so I have to create a new file. Thanks.
Attachment #8934779 - Flags: review?(florian)
Comment on attachment 8934779 [details] [diff] [review] Part2: Modify the paths in test file Review of attachment 8934779 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Kershaw Chang [:kershaw] from comment #6) > Obviously I can't use git to export my patch, so using hg instead. I gave you the hg commands because hg is the version control tool for mozilla-central. If you want to use git that's up to you, but let's not blame the tools for your mistakes. Note: the hg patches are all exported in the 'git' format anyway. ::: browser/base/content/test/favicons/browser.ini @@ +15,5 @@ > [browser_multiple_icons_in_short_timeframe.js] > [browser_rich_icons.js] > [browser_icon_discovery.js] > [browser_preferred_icons.js] > +[browser_favicon_load.js] Please do the 'hg move' of this file in the same attachment (ie put the 2 parts into a single patch). ::: browser/base/content/test/favicons/file_favicon_thirdParty.html @@ +1,1 @@ > +<!DOCTYPE HTML> Is there a reason for this file to be 'hg add'ed when the other 3 files were 'hg copy'ed?
Attachment #8934779 - Flags: review?(florian) → review-
Attachment #8934778 - Flags: review?(florian)
(In reply to Florian Quèze [:florian] from comment #8) > Comment on attachment 8934779 [details] [diff] [review] > Part2: Modify the paths in test file > > Review of attachment 8934779 [details] [diff] [review]: > ----------------------------------------------------------------- > > (In reply to Kershaw Chang [:kershaw] from comment #6) > > > Obviously I can't use git to export my patch, so using hg instead. > > I gave you the hg commands because hg is the version control tool for > mozilla-central. If you want to use git that's up to you, but let's not > blame the tools for your mistakes. Note: the hg patches are all exported in > the 'git' format anyway. > I mean there is no similar command like "hg copy" in git, if I was correct. https://github.com/glandium/git-cinnabar/issues/97 > > Is there a reason for this file to be 'hg add'ed when the other 3 files were > 'hg copy'ed? Because the favicon link in file_favicon_thirdParty.html is changed, using "hg copy" may cause conflict in the future? Could you correct me if I am wrong? I've never used "hg copy" before. Will the change made to the original file also be merged to the copied one?
(In reply to Kershaw Chang [:kershaw] from comment #9) > Because the favicon link in file_favicon_thirdParty.html is changed, using > "hg copy" may cause conflict in the future? I don't think so. > Could you correct me if I am wrong? I've never used "hg copy" before. Will > the change made to the original file also be merged to the copied one? After a copy the 2 files will be different (a change to one of them won't affect the other), the point of doing a copy is just to let hg record that the 2 files share the same history.
Attached patch Move browser_favicon_load.js to favicon folder (obsolete) (deleted) — — Splinter Review
Summary: - Merge two patches into one - Use "hg copy" for file_favicon_thirdParty.html Thanks.
Attachment #8934778 - Attachment is obsolete: true
Attachment #8934779 - Attachment is obsolete: true
Attachment #8935227 - Flags: review?(florian)
Comment on attachment 8935227 [details] [diff] [review] Move browser_favicon_load.js to favicon folder Looks good to me, thanks!
Attachment #8935227 - Flags: review?(florian) → review+
Keywords: checkin-needed
Pushed by cbrindusan@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b9e618665a96 Move browser_favicon_load.js to favicon folder, r=florian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b9e618665a96
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: