1431148 - Expose full document screen capture endpoint

Status: RESOLVED FIXED

(Testing :: geckodriver, defect, P2)

Description

[Andreas Tolfsen ❲:ato❳]

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}.

Comment 1

[snpanigrahi9]

Any updated news on this issue.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 3

[Andreas Tolfsen ❲:ato❳]

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.

Comment 4

[Greg Fraley]

Attachment: Bug 1431148 - Added /moz/screenshot/full extension endpoint

Review commit: https://reviewboard.mozilla.org/r/226708/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226708/

Comment 5

[Andreas Tolfsen ❲:ato❳]

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.

Comment 6

[James Graham [:jgraham]]

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.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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?

Comment 8

[Greg Fraley]

Attachment: Bug 1431148 - Add /moz/screenshot/full extension endpoint

Review commit: https://reviewboard.mozilla.org/r/229158/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229158/

Comment 9

[Greg Fraley]

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?

Comment 10

[Andreas Tolfsen ❲:ato❳]

(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.

Comment 11

[Greg Fraley]

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?

Comment 12

[James Graham [:jgraham]]

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

Comment 13

[Greg Fraley]

Attachment: Bug 1431148 - Add full document screenshot test

Review commit: https://reviewboard.mozilla.org/r/229904/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229904/

Comment 14

[Greg Fraley]

Comment on attachment 8960374 [details]
Bug 1431148 - Add /moz/screenshot/full extension endpoint

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229158/diff/1-2/

Comment 15

[Greg Fraley]

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?

Comment 16

[Greg Fraley]

Attachment: Bug 1431148 - Add full document screenshot test

Review commit: https://reviewboard.mozilla.org/r/229910/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229910/

Comment 17

[James Graham [:jgraham]]

Attachment: test.diff

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.

Comment 18

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1431148: geckodriver: add TakeFullScreenshot command;

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.

Comment 19

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1431148: webdriver: add screenshot test utilities;

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

Depends on D6886

Comment 20

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1431148: geckodriver: add tests for TakeFullScreenshot;

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

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Henrik Skupin (:whimboo) has approved the revision.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Henrik Skupin (:whimboo) has approved the revision.

Comment 23

[Andreas Tolfsen ❲:ato❳]

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

Comment 24

[James Graham [:jgraham]]

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.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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!

Comment 26

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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.

Comment 27

[Andreas Tolfsen ❲:ato❳]

 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…

Comment 28

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1431148: webdriver: make wpt fixtures a module;

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.

Comment 29

[Andreas Tolfsen ❲:ato❳]

Attachment: bug 1431148: webdriver: load wpt fixtures as pytest plugin;

Depends on D9418

Comment 30

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

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

Comment 31

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

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)

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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?

Comment 33

[Pulsebot]

Pushed by [e-mail]:
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

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Comment 35

[Raul Gurzau (:RaulG)]

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

Comment 36

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged