Closed Bug 1431148 Opened 2 years ago Closed 1 year ago

Expose full document screen capture endpoint

Categories

(Testing :: geckodriver, defect, P2)

Version 3
defect

Tracking

(firefox65 fixed)

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug, )

Details

Attachments

(5 files, 5 obsolete files)

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}.
Any updated news on this issue.
Flags: needinfo?(hskupin)
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.
Flags: needinfo?(hskupin)
Priority: -- → P5
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.
Flags: needinfo?(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.
Attachment #8957743 - Flags: review-
Flags: needinfo?(ato) → needinfo?(james)
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.
Flags: needinfo?(james)
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?
Attachment #8957743 - Attachment is obsolete: true
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?
Flags: needinfo?(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.
Flags: needinfo?(ato)
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?
Flags: needinfo?(james)
Run mach wpt-manifest-update after creating the file.
Flags: needinfo?(james)
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?
Attachment #8961085 - Attachment is obsolete: true
Flags: needinfo?(james)
Attached patch test.diff (obsolete) — Splinter Review
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.
Flags: needinfo?(james)
Assignee: nobody → ato
Priority: P5 → P2
Attachment #8962201 - Attachment is obsolete: true
Attachment #8961094 - Attachment is obsolete: true
Attachment #8960374 - Attachment is obsolete: true
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.
This adds some test utilities to the WebDriver WPT tests for dealing
with screenshots and document dimensions.

Depends on D6886
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
Blocks: 1494208
Comment on attachment 9012071 [details]
bug 1431148: geckodriver: add TakeFullScreenshot command;

Henrik Skupin (:whimboo) has approved the revision.
Attachment #9012071 - Flags: review+
Comment on attachment 9012072 [details]
bug 1431148: webdriver: add screenshot test utilities;

Henrik Skupin (:whimboo) has approved the revision.
Attachment #9012072 - Flags: review+
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
Flags: needinfo?(james)
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.
Flags: needinfo?(james)
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!
Flags: needinfo?(rpierzina)
Flags: needinfo?(dave.hunt)
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.
Flags: needinfo?(dave.hunt)
 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…
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.
Attachment #9012071 - Attachment description: bug 1431148: geckodriver: add TakeFullScreenshot command → bug 1431148: geckodriver: add TakeFullScreenshot command;
Attachment #9012072 - Attachment description: bug 1431148: webdriver: add screenshot test utilities → bug 1431148: webdriver: add screenshot test utilities;
Attachment #9012073 - Attachment description: bug 1431148: geckodriver: add tests for TakeFullScreenshot → bug 1431148: geckodriver: add tests for TakeFullScreenshot;
Flags: needinfo?(rpierzina)
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13757 for changes under testing/web-platform/tests
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?
Status: NEW → ASSIGNED
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
Pulsebot was down, and as such the comments weren't added for any recent landing from today. This has been fixed now.
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.