Bug 1169179 (mozscreenshots)

Run mozscreenshots via mochitest-browser-chrome

RESOLVED FIXED in Firefox 46

Status

()

Firefox
General
--
enhancement
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: MattN, Assigned: MattN)

Tracking

(Depends on: 10 bugs)

unspecified
Firefox 46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 8612148 [details] [diff] [review]
v0.1 Add browser_screenshots.js in the screenshots subsuite

Being able to run mozscreenshots from mach and in automation will make it much more convenient to use and therefore likely to be used more often to detect visual regressions and to perform UI reviews while features are in development.

I think running it via the mochitest-browser-chrome harness is probably easiest and I think it can be a subsuite so it won't run by default but can have it's own buildbot job type in automation triggered by try syntax.

Some things to figure out:
* Where to get the XPI from (e.g. build in m-c or fetch from another repo?)
* Where the test file should live

Once we have this figured out I can file follow-ups for getting this running in automation.
Example try syntax with this patch applied:
`
try: -b o -p linux64,macosx64,win32,win64 -u mochitest-bc[Ubuntu,10.6,10.10,Windows XP,Windows 7,Windows 8] -t none --subsuite screenshots --setenv MOZSCREENSHOTS_SETS=DevEdition,TabsInTitlebar,Tabs,WindowSize,Toolbars
`
Blocks: 1231408
Attachment #8612148 - Attachment is obsolete: true
Created attachment 8697332 [details]
MozReview Request: Bug 1169179 - Run mozscreenshots as a mochitest-browser-chrome test. r=felipe,glandium

Bug 1169179 - Run mozscreenshots as a mochitest-browser-chrome test. r=felipe,glandium
Attachment #8697332 - Flags: review?(mh+mozilla)
Attachment #8697332 - Flags: review?(felipc)
Comment on attachment 8697332 [details]
MozReview Request: Bug 1169179 - Run mozscreenshots as a mochitest-browser-chrome test. r=felipe,glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27559/diff/1-2/
Comment on attachment 8697332 [details]
MozReview Request: Bug 1169179 - Run mozscreenshots as a mochitest-browser-chrome test. r=felipe,glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27559/diff/2-3/
Comment on attachment 8697332 [details]
MozReview Request: Bug 1169179 - Run mozscreenshots as a mochitest-browser-chrome test. r=felipe,glandium

https://reviewboard.mozilla.org/r/27559/#review25023

::: browser/tools/mozscreenshots/mozscreenshots/extension/Makefile.in:10
(Diff revision 3)
> +libs-preqs = \
> +  $(call mkdir_deps,$(TEST_EXTENSIONS_DIR)) \
> +  $(NULL)

You can replace this by GENERATED_DIRS = $(TEST_EXTENSIONS_DIR)

(see e.g. dom/workers/test/extensions/bootstrap/Makefile.in)

::: browser/tools/mozscreenshots/mozscreenshots/extension/Makefile.in:16
(Diff revision 3)
> +	$(NSINSTALL) -D $(DEPTH)/_tests/testing/mochitest/extensions/mozscreenshots

You should just add $(DEPTH)/_tests/testing/mochitest/extensions/mozscreenshots to GENERATED_DIRS

::: browser/tools/mozscreenshots/mozscreenshots/extension/Makefile.in:17
(Diff revision 3)
> +	cp -RL $(DEPTH)/testing/mozscreenshots/mozscreenshots $(DEPTH)/_tests/testing/mochitest/extensions

This seems like a convoluted way to put things in _tests/testing/mochitest/extensions. Why not just put them there in the first place, instead of "buffering" in testing/mozscreenshots/mozscreenshots?
Attachment #8697332 - Flags: review?(mh+mozilla)
Created attachment 8698751 [details]
MozReview Request: Bug 1169179 - Run mozscreenshots as a mochitest-browser-chrome test. r=felipe,glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27559/diff/3-4/
Attachment #8698751 - Flags: review?(mh+mozilla)
Attachment #8698751 - Flags: review?(felipc)
Comment on attachment 8698751 [details]
MozReview Request: Bug 1169179 - Run mozscreenshots as a mochitest-browser-chrome test. r=felipe,glandium

https://reviewboard.mozilla.org/r/27559/#review25155

::: browser/tools/mozscreenshots/mozscreenshots/extension/Makefile.in:7
(Diff revision 4)
> +GENERATED_DIRS += $(DEPTH)/_tests/testing/mochitest/extensions/mozscreenshots

Considering the content of this Makefile now, this line shouldn't be needed at all.
Attachment #8698751 - Flags: review?(mh+mozilla) → review+
Attachment #8697332 - Attachment is obsolete: true
Attachment #8697332 - Flags: review?(felipc)
Comment on attachment 8697332 [details]
MozReview Request: Bug 1169179 - Run mozscreenshots as a mochitest-browser-chrome test. r=felipe,glandium

https://reviewboard.mozilla.org/r/27559/#review27917

::: .eslintignore:87
(Diff revision 4)
> +browser/tools/mozscreenshots/mozscreenshots/extension/bootstrap.js
> +browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm

let's see if we can remove this

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:45
(Diff revision 4)
> +        screenshotPath = FileUtils.getFile("TmpD", ["mozscreenshots"]).path + "\\";
> +        break;
> +      case "Darwin":
> +      case "Linux":
> +        screenshotPath = "/tmp/mozscreenshots/";

why not use TmpD on OSX/Linux too?

::: browser/tools/mozscreenshots/mozscreenshots/extension/bootstrap.js:1
(Diff revision 4)
> +#if 0
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +# You can obtain one at http://mozilla.org/MPL/2.0/.
> +#endif

maybe wrapping this in a comment block can keep this file lintable and make the pre-processor happy
Attachment #8697332 - Flags: review+
Attachment #8698751 - Flags: review?(felipc) → review+
Comment on attachment 8697332 [details]
MozReview Request: Bug 1169179 - Run mozscreenshots as a mochitest-browser-chrome test. r=felipe,glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27559/diff/4-5/
Attachment #8697332 - Attachment is obsolete: false
Attachment #8697332 - Flags: review?(mh+mozilla)
Comment on attachment 8698751 [details]
MozReview Request: Bug 1169179 - Run mozscreenshots as a mochitest-browser-chrome test. r=felipe,glandium

mozreview seems to be confused with the two attachments
Attachment #8698751 - Attachment is obsolete: true
Attachment #8697332 - Flags: review?(mh+mozilla) → review+

Comment 11

a year ago
https://hg.mozilla.org/integration/fx-team/rev/162b1fb7424a
Depends on: 1241284

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/162b1fb7424a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Depends on: 1241633
Alias: mozscreenshots
Depends on: 1241648
Depends on: 1242066
Depends on: 1242083
Depends on: 1242101
Depends on: 1242103
Depends on: 1245364
Depends on: 1245719
Depends on: 1246455
Depends on: 1246843
Depends on: 1246847
Depends on: 1246849
Depends on: 1247149
Depends on: 1111047
Depends on: 1247808
Depends on: 1248027
Depends on: 1248087

Comment 13

a year ago
[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
STR: Not clear. Developer specific testing

Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Actual Results: 
As expected

Expected Results: 
Please improve STR.
Depends on: 1261946
Depends on: 1268619
Depends on: 1268648

Updated

10 months ago
Depends on: 1283804
Depends on: 1287935
Depends on: 1294568
Depends on: 1331211
Depends on: 1332727
Depends on: 1332760
Depends on: 1332821
Depends on: 1332945
Depends on: 1342819
Depends on: 1342825
Depends on: 1343049
You need to log in before you can comment on or make changes to this bug.