Bug 1169179 (mozscreenshots)

Run mozscreenshots via mochitest-browser-chrome

RESOLVED FIXED

Status

enhancement
RESOLVED FIXED
4 years ago
10 months ago

People

(Reporter: MattN, Assigned: MattN)

Tracking

(Depends on 5 bugs)

unspecified
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

()

Attachments

(1 attachment, 2 obsolete attachments)

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
`
Attachment #8612148 - Attachment is obsolete: true
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)
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+
https://hg.mozilla.org/mozilla-central/rev/162b1fb7424a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Alias: mozscreenshots
[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.
See Also: → 1373172
Depends on: 1373551
Depends on: 1373563
Depends on: 1373783
Component: General → mozscreenshots
Product: Firefox → Testing
Target Milestone: Firefox 46 → ---
You need to log in before you can comment on or make changes to this bug.