Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support takeScreenshot #11

Merged
merged 3 commits into from
Aug 9, 2024
Merged

feat: Support takeScreenshot #11

merged 3 commits into from
Aug 9, 2024

Conversation

tianfeng92
Copy link
Contributor

@tianfeng92 tianfeng92 commented Aug 8, 2024

Description

It can support .takeScreenshot() and .takeElementScreenshot() both.

Note: The TestCafe Official provider also takes screenshots for BiDi browser types. I've checked our documentation, and we haven't enabled BiDi yet in this provider. Additionally, supporting BiDi features requires webdriver@8.11.0+, which is a pure ESM package, causing several issues for now.

So, in this PR, I only added the functionality for taking screenshots for the W3C browser type.

@tianfeng92 tianfeng92 changed the title feat: Support take screenshot feat: Support takeScreenshot Aug 8, 2024
);
async takeScreenshot(
browserId: string,
screenshotPath: string,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The screenshotPath has already been converted to an absolute path and has the .png extension added by TestCafe. There's no need for further validation.
Additionally, it can create directories recursively, so we don't need to validate the directory's existence either.

) {
if (fullPage) {
console.warn(
'Taking a full-page screenshot on the remote browser is not supported.',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No fullPage is mentioned in TestCafe plugin-host.js, but it can still be passed via t.takeScreenshot(path, fullPage). It is better to provide a warning message to indicate that full-page screenshots do not work for remote browsers.

@tianfeng92 tianfeng92 marked this pull request as ready for review August 8, 2024 17:36
@tianfeng92 tianfeng92 requested a review from a team as a code owner August 8, 2024 17:36
Copy link
Contributor

@mhan83 mhan83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, how exactly does takeElementScreenshot support work? I don't see a call to webdriver.takeElementScreenshot anywhere?

@tianfeng92
Copy link
Contributor Author

tianfeng92 commented Aug 8, 2024

Just curious, how exactly does takeElementScreenshot support work? I don't see a call to webdriver.takeElementScreenshot anywhere?

takeScreenshot and takeElementScreenshot are both using the same screenshot command and calling takeScreenshot from the provider.

Then extra crop would be applied after getting the screenshot.

@tianfeng92 tianfeng92 requested a review from mhan83 August 8, 2024 23:40
@tianfeng92 tianfeng92 merged commit 710d006 into main Aug 9, 2024
1 check passed
@tianfeng92 tianfeng92 deleted the DEVX-2786 branch August 9, 2024 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants