Closed Bug 1169179 (mozscreenshots) Opened 9 years ago Closed 8 years ago

Run mozscreenshots via mochitest-browser-chrome

Categories

(Testing :: mozscreenshots, enhancement)

enhancement
Not set
normal

Tracking

(firefox46 fixed)

RESOLVED FIXED
Tracking Status
firefox46 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

(Depends on 4 open bugs, )

Details

Attachments

(1 file, 2 obsolete files)

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
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)
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+
Depends on: 1241284
https://hg.mozilla.org/mozilla-central/rev/162b1fb7424a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
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
[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: 1268619
Depends on: 1268648
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
Depends on: 1369754
Depends on: 1371339
See Also: → 1373172
Depends on: 1373178
Depends on: 1373551
Depends on: 1373563
Depends on: 1373783
Depends on: 1374827
Component: General → mozscreenshots
Product: Firefox → Testing
Target Milestone: Firefox 46 → ---
Depends on: 1385233
Depends on: 1416538
You need to log in before you can comment on or make changes to this bug.