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

Fix issues with create screenshot #75

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

paulmskim
Copy link

@paulmskim paulmskim commented Nov 7, 2022

This PR fixes a couple issues with the createScreenshot() method:

  • If the element position is outside the window dimensions, and we are not setting the fullScreenShot config to true, a blank image is generated. To fix this, I've added a new config forceFullScreenShot config to capture the full screen to allow cropping to occur when the element position is outside the initial window dimensions.
  • If a single test runs any of the test methods more than once, and the fullScreenShot config is true, all of the screenshots after the first image returns a repeated image of the last iterated image. To fix this, I've added a script for the webdriver to set the window position back to 0, 0.

Not sure if the forceFullScreenShot config is the best way to do this, but the other option is to capture the full screen in all cases.

Copy link
Member

@Arhell Arhell left a comment

Choose a reason for hiding this comment

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

@paulmskim
Copy link
Author

@buresmi7 any thoughts on when you can address this?

@buresmi7
Copy link
Collaborator

@paulmskim thanks for your PR ;) new option looks good! it would be nice have some test for it.

@paulmskim
Copy link
Author

@buresmi7 I tried adding tests but the webdriver I'm using on my project (https://packagist.org/packages/codeception/module-webdriver#1.4.0) runs differently from the webdriver for the tests (the facebook webdriver does not respect the window size option when taking a screenshot, which is the problem here, and I can't run a test to show the issue).

One option is to update webdriver to a non-abandoned package, and maybe codeception too.

Any other thoughts on how to test this?

@ggiak
Copy link

ggiak commented Jan 18, 2023

@buresmi7 I tried adding tests but the webdriver I'm using on my project (https://packagist.org/packages/codeception/module-webdriver#1.4.0) runs differently from the webdriver for the tests (the facebook webdriver does not respect the window size option when taking a screenshot, which is the problem here, and I can't run a test to show the issue).

One option is to update webdriver to a non-abandoned package, and maybe codeception too.

Any other thoughts on how to test this?

#78 updates codeception, webdriver & php version.
Would you like check your PR against latest versions ?

@ggiak
Copy link

ggiak commented Jan 29, 2023

@paulmskim any updates on this ?

@paulmskim
Copy link
Author

@ggiak I've done some simple testing within the module, but not enough within other projects to be happy with this. I will say that jumping directly from php >=5.4 to >=8.0 is a bit extreme. There are also no incremental versions that follow codeception, webdriver, and other module releases. Projects that are still running on php 7.4 or older codeception versions, like the one I'm working on, cannot use this.

I'll keep this PR open, but will leave it to you and @buresmi7 to merge or close as you see fit.

@@ -490,7 +490,6 @@ private function createScreenshot($identifier, array $coords, array $excludeElem

$screenShotImage->resetIterator();
$fullShot = $screenShotImage->appendImages(true);
$fullShot->writeImage($elementPath);
Copy link
Author

Choose a reason for hiding this comment

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

if you decide to close this, at the least remove this (double writeImage)

@ggiak
Copy link

ggiak commented Jan 30, 2023

@ggiak I've done some simple testing within the module, but not enough within other projects to be happy with this. I will say that jumping directly from php >=5.4 to >=8.0 is a bit extreme. There are also no incremental versions that follow codeception, webdriver, and other module releases. Projects that are still running on php 7.4 or older codeception versions, like the one I'm working on, cannot use this.

I'll keep this PR open, but will leave it to you and @buresmi7 to merge or close as you see fit.

Hey @paulmskim, from my point of view we "jumped" so much in order support codeception version 5, not php8.

Unfortunately, codeception 5 requires php8 as seen here: https://github.com/Codeception/Codeception/blob/5.0/composer.json#L17

hence the huge refactoring. But you are right, Perhaps we should write something relevant to the readme

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.

4 participants