Andreas Tolfsen ❲:ato❳ Assignee
	Description • 6 years ago

We have support for taking full document screenshots in Marionette,
but it isn’t exposed in geckodriver yet since it is a proprietary
non-WebDriver API.  WebDriver only expects the capture area to
be the viewport.  We do, however, want to expose this under a
Mozilla-specific namespace in geckodriver.

Originally reported in https://github.com/mozilla/geckodriver/issues/570.

That extension command be something like /session/{session
id}/moz/screenshot/full or something, which should call down to
the Marionette WebDriver:TakeScreenshot command with {full: true}.

	snpanigrahi9
	Comment 1 • 6 years ago

Any updated news on this issue.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 2 • 6 years ago

This bug is not a priority for us at the moment. But if you are interested in and could imagine to contribute code, Andreas might be able to mentor you through the process of implementation.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 3 • 6 years ago

There’s also a discussion over in
https://bugzilla.mozilla.org/show_bug.cgi?id=1434313 about whether
the semantics for taking a full document screenshot are correct.
I don’t necessarily think it is a blocker for exposing the command
endpoint as such, since it is a proprietary extension to the protocol.
However, of course, it would be nice not to make semantic changes
after publisising the new command.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 5 • 6 years ago
mozreview-review

Comment on attachment 8957743 [details]
Bug 1431148 - Added /moz/screenshot/full extension endpoint

https://reviewboard.mozilla.org/r/226708/#review234570

Thanks again for an excellent patch!  If you in your commit messages
say "Bug XXX - Foobar; r?ato", I will be automatically notified
about the incoming review.  Sorry for missing this earlier.

I’m not thrilled about not having tests for this, but at the same
time I am not sure what the best testing strategy would be.  The
only harness where we use geckodriver is WPT (the Wd job on
TaskCluster/try) and it is meant for testing the WebDriver
specification.

Since this endpoint is a proprietary vendor extension, I wonder if
it would be OK to include it there.  We would need some way to mark
the test as Mozilla-only so that it does not get picked up by other
driver implementations.  Let’s ask jgraham if we have any such
technique available to wdspec tests.

Finally a question: Do you have commit access level 1 or higher?
If not we should request it for you so you can trigger your own try
runs.

::: commit-message-415e9:1
(Diff revision 1)
> +Bug 1431148 - Added /moz/screenshot/full extension endpoint

Please use normative language, e.g. s/Added/Add/.

::: testing/geckodriver/src/marionette.rs:76
(Diff revision 1)
> +             (Method::Get, "/session/{sessionId}/moz/screenshot/full",
> +                GeckoExtensionRoute::TakeFullScreenshot)];

You also need to mention this in testing/geckodriver/CHANGES.md.

	James Graham [:jgraham]
	Comment 6 • 6 years ago

So in theory testing/web-platform/mozilla/tests/webdriver/ would be the right place. But I am not 100% sure if we pass the right path in to the manifest generation to have them detected as the right test type. I would try adding them there and if it turns out that they aren't added to the manifest then we should go ahead and fix things so that they are.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 7 • 6 years ago

I brought the same question up in a meeting some weeks ago and I was told that the mozilla folder might go away in the future. So is that really the right place to add a test? Or should this become a Marionette unit test instead?

	Greg Fraley
	Comment 9 • 6 years ago

So at the time, I do not have level 1 access.

I can go ahead and add tests to testing/web-platform/mozilla/tests/webdriver/ while we wait to find out its fate.  Should I add the tests as a separate commit on top of the first, or squash the two together when the time comes?

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 10 • 6 years ago

(In reply to Greg Fraley from comment #9)
> So at the time, I do not have level 1 access.

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1447285.

> I can go ahead and add tests to
> testing/web-platform/mozilla/tests/webdriver/ while we wait to
> find out its fate.  Should I add the tests as a separate commit on
> top of the first, or squash the two together when the time comes?

Depends how extensive they are I suppose, but I don’t think I
would object to either.

	Greg Fraley
	Comment 11 • 6 years ago

So simply adding a test script as testing/web-platform/mozilla/tests/webdriver/full_document_screenshot.py doesn't allow me to run the test via ./mach wpt.  How should I go about adding it to the manifest?

	James Graham [:jgraham]
	Comment 12 • 6 years ago

Run mach wpt-manifest-update after creating the file.

	Greg Fraley
	Comment 15 • 6 years ago

The attached tests work, but not when placed in the "mozilla" directory under web-platform, even after running the manifest-update command.  Instead, running the tests results in "Ran 0 tests in {some number of seconds}".  Are there any further steps I should take to get the tests to register as tests?

	James Graham [:jgraham]
	Comment 17 • 6 years ago

So there are a couple of issues here:
1) Test functions need to start with test_
2) The conftest file was missing so the fixtures couldn't be found.

Fixing 2) in partcular is a little involved so I've attached a patch with a somewhat hacky (but probably OK) solution. Try applying it to your branch and let me know if things work.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 18 • 6 years ago

This adds a new proprietary extension command to geckodriver for
taking a screenshot of the full document.  The new endpoint is GET
/session/{session id}/moz/screenshot/full and returns, like the
Take Screenshot and Take Element Screenshot, a Base64 encoded string.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 19 • 6 years ago

This adds some test utilities to the WebDriver WPT tests for dealing
with screenshots and document dimensions.

Depends on D6886

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 20 • 6 years ago

This hooks up the Mozilla-specific WPT WebDriver tests directory
to use fixtures from the upstream public repository.

The tests for the new TakeFullScreenshot command are somewhat more
thorough than those for Take Screenshot and Take Element Screenshot,
but this will be addressed later as part of bug 1494208.

Depends on D6887

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 21 • 6 years ago

Comment on attachment 9012071 [details]
bug 1431148: geckodriver: add TakeFullScreenshot command;

Henrik Skupin (:whimboo) has approved the revision.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 22 • 6 years ago

Comment on attachment 9012072 [details]
bug 1431148: webdriver: add screenshot test utilities;

Henrik Skupin (:whimboo) has approved the revision.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 23 • 6 years ago

Using the execfile hack you drafted in
https://bug1431148.bmoattachments.org/attachment.cgi?id=8962201
causes globals, such as _current_session to disappear.  I checked
that testing/web-platform/mozilla/test/conftest.py doesn’t have
this global in its scope, so passing globals() to execfile will not
work.

As I understand it, the effect we want to achieve is to mirror the
WPT upstream conftest.py in here, but with a modified import path.
Do you have an idea how we can also gain access to the globals the
WPT conftest is holding?

You can see how this fails in automation:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=381761730ce0e1a59653ce3b333f052efccf5b61

Running test_full_screenshot.py by itself works fine and it starts
a new session.  Once you run another test before it, a session
already exists in geckodriver but _current_session is not set,
causing it to try to start a second session which fails.

It is possible to reproduce it this way:

> MOZ_HEADLESS=1 ./mach wpt --webdriver-arg=-vv testing/web-platform/tests/webdriver/tests/element_click/click.py testing/web-platform/mozilla/tests/webdriver/take_full_screenshot.py

	James Graham [:jgraham]
	Comment 24 • 6 years ago

Oh, hmm. I don't think I have a good solution off the top of my head. We might need to investigate if pytest has a mecahnism to change the conftest files that we use.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 25 • 6 years ago

Raphael or Dave, could you have a look at comment 23/24? Is there such a way in pytest? If not what would you propose to do given your better knowledge with pytest? Thanks!

	Dave Hunt [:davehunt] [he/him] ⌚BST
	Comment 26 • 6 years ago

Do I understand correctly that we have two sibling paths, and we want a conftest.py to be shared between them? Is there any way we can bump the conftest.py to the shared parent directory? Any conftest.py files that are discovered effect the tests at the same or deeper level on the filesystem.

A conftest.py is a local plugin, so another option is to create an actual plugin. This would require a package that must be installed, such as wptrunner, which would register an pytest plugin as an entry point. The plugin can provide the fixtures that will be available whenever pytest is executed with the package installed. See https://docs.pytest.org/en/latest/writing_plugins.html for more information on writing a plugin.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 27 • 6 years ago

 Dave Hunt [:davehunt] ⌚️UTC+1 from comment #26)

> Do I understand correctly that we have two sibling paths, and we
> want a conftest.py to be shared between them? Is there any way
> we can bump the conftest.py to the shared parent directory? Any
> conftest.py files that are discovered effect the tests at the same
> or deeper level on the filesystem.

testing/web-platform/tests is overlayed into our repository from the
downstream https://github.com/w3c/web-platform-tests repository, so
we can’t move conftest.py to a parent directory because it needs to
be accessible to other browser vendors running the tests.

The effect we want to achieve here is for this particular config to
_also_ be available to our internal tests in a different part of the
tree.

> A conftest.py is a local plugin, so another option is to
> create an actual plugin. This would require a package
> that must be installed, such as wptrunner, which would
> register an pytest plugin as an entry point. The plugin
> can provide the fixtures that will be available whenever
> pytest is executed with the package installed. See
> https://docs.pytest.org/en/latest/writing_plugins.html for more
> information on writing a plugin.

This is good to know, and I will look into this, but it seems
somewhat heavy-handed when all we really want is for conftest.py to
be symlinked into this other directory with some modified import
paths…

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 28 • 6 years ago

To make it possible for other non-WPT tests to pick up and use
the WebDriver WPT fixtures, we need to turn them into a separate
Python module.

Consumers can then make the module discoverable on their Python
search path and use pytest_plugins to include them in their own
conftest.py files.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 29 • 6 years ago

Depends on D9418

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 30 • 6 years ago

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13757 for changes under testing/web-platform/tests

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 31 • 6 years ago

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/13757
* continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/447737520?utm_source=github_status&utm_medium=notification)
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/WHLIe1eLQJ2oTDNg0oHZ7w)

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 32 • 6 years ago

Andreas, I wonder why all of the commits have been closed? Nothing landed yet as far as I can see. In which state is this bug?

	Pulsebot
	Comment 33 • 6 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/113b68d053f8
webdriver: make wpt fixtures a module; r=davehunt
https://hg.mozilla.org/integration/autoland/rev/8af1c0a6ca87
webdriver: load wpt fixtures as pytest plugin; r=davehunt
https://hg.mozilla.org/integration/autoland/rev/3c4a7d54a7cd
geckodriver: add TakeFullScreenshot command; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/e30ce73ca4f1
webdriver: add screenshot test utilities; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/fef96678ad06
geckodriver: add tests for TakeFullScreenshot; r=whimboo

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 34 • 6 years ago

Pulsebot was down, and as such the comments weren't added for any recent landing from today. This has been fixed now.

	Raul Gurzau (:RaulG)
	Comment 35 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/113b68d053f8
https://hg.mozilla.org/mozilla-central/rev/8af1c0a6ca87
https://hg.mozilla.org/mozilla-central/rev/3c4a7d54a7cd
https://hg.mozilla.org/mozilla-central/rev/e30ce73ca4f1
https://hg.mozilla.org/mozilla-central/rev/fef96678ad06

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 36 • 6 years ago

Upstream PR merged