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

take screenshots of correct window #245

Closed
wants to merge 1 commit into from

Conversation

dreyks
Copy link
Contributor

@dreyks dreyks commented Dec 6, 2018

fixes #143

unfortunately couldn't figure out how to properly test this - base capybara driver is not capable of switching windows

@mattheworiordan
Copy link
Owner

Thanks for this contribution @dreyks, however this code is not threadsafe AFAICT. Capybara Screenshot does indeed share static variables, however unless there is a bug, the idea is that session (test) specific state is not stored at a static class level for obvious reasons (parallel tests). Would you be able to look into a way to use a test local variable instead?

@dreyks
Copy link
Contributor Author

dreyks commented Jan 27, 2019

🤔 At first I've been thinking about that but then I saw some other places of direct writing into a static variable like this

Capybara::Screenshot.final_session_name = original_session_name

But I think I can store the offending window in the capybara session (or page) itself

@mattheworiordan
Copy link
Owner

At first I've been thinking about that but then I saw some other places of direct writing into a static variable like this

Mmm, that's not good. I'll raise an issue to fix that too down the line.

@trcarden
Copy link

@mattheworiordan and @dreyks Has this stalled out? I found myself also in a situation where it would be quite nice to the the screenshot from the new window.

@mattheworiordan
Copy link
Owner

Has this stalled out?

Yup, waiting on @dreyks to update I am afraid.

@cgunther
Copy link
Contributor

@trcarden check out #287

@mattheworiordan
Copy link
Owner

Closing as superseded by #287

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Screenshots are not taken when using multiple windows
4 participants