elixir-wallaby / wallaby

Concurrent browser tests for your Elixir web apps.
https://twitter.com/elixir_wallaby
MIT License
1.68k stars 198 forks source link

screenshot_on_failure takes screenshot of all sessions #555

Closed cysiegel closed 4 years ago

cysiegel commented 4 years ago

When screenshot_on_failure is set to true - an automatic screenshot is taken of all the sessions in the feature - not just the session where the error happened

Is there a way to to set it so that there is only an automatic screenshot of the session where the error happened? (not sure if this is considered a bug or feature request)

Using Wallaby 0.26.2, chrome driver, elixir 1.10.4 and running on a mac

Thanks

mhanberg commented 4 years ago

I'm not sure if there is a way to tell which sessions caused the failure.

Failures could come from many places, an exceptions we raise ourselves based on the web driver returning an error, or from an assertion that you make yourself.

What problem are you running into regarding having multiple screenshots taken?

yonst commented 4 years ago

In the old wallaby versions, the screenshot on failure came straight from the find function so the screen shot was taken only from the relevant session.

Screenshot from all the sessions take a lot of space and make it difficult to find the relevant picture. The idea of the screenshot is that when element is missing I can see what the page condition.

mhanberg commented 4 years ago

I think as a user, that is non-intuitive (given you haven't seen the old behavior). One would expect that if you have screenshots on failure configured, that any test failure would produce a screenshot.

If you are using multiple sessions in a single test, they are most likely interacting with each other, so you probably want to have a screenshot of each session.

yonst commented 4 years ago

I'm using wallaby few years and I always had 2 sessions because In my company app I have 2 apps that works in a pipe (client side and customer side) Most of my tests includes action on both apps so I use 2 sessions for it.

When failure happens because of missing element I want screenshot of this session only. The fact that I get now 2 screenshots from every failure causing me a lot of problems (in debugging and in my code - I send the failure screenshots as slack message but now it causing me a problems)

I hope that you will understand that when you are doing big changing like that you may harm many users so you should try to listen to them. I am sure that I'm not the only one that have trouble with this change.

You can see se the old wallaby code of take_screenshot_on_failure here: https://github.com/elixir-wallaby/wallaby/blob/v0.22.0/lib/wallaby/browser.ex#L455

mhanberg commented 4 years ago

@yonst I appreciate your concern and I'm sorry the change is having negative effects on your workflow.

If I hear similar concerns from more users, I'll bring the topic back up for discussion.

yonst commented 4 years ago

@mhanberg As you can see, some user (@anduonghien - I don't know him) did 👍 on my comment and @cysiegel that opened this issue so probably they also have problems with this change.

michallepicki commented 4 years ago

@yonst I'll try to give some background reasoning and ideas on how you could proceed further:

I think the issue with the old approach inside find was that when tests were failing because of regular ExUnit assertions or other reasons like raised exceptions, the Wallaby user was left without additional info (other than the test failure message). To handle this, and other setup-related tasks, the Wallaby.Feature module and feature macro was introduced. There might be better ways to solve this particular problem than wrapping test code in try/rescue but the only ones that we saw at the time (custom ExUnit runner or formatter) were less appealing.

I think you can a) Follow the feature macro documentation on how to use Wallaby without use Wallaby.Feature and the feature macro. You can do that just the same way as Wallaby was used before it was introduced, but you will have to add code to take screenshots yourself. b) Copy the Wallaby.Feature.feature macro code and modify it slightly so that instead of Wallaby.Feature.Utils.take_screenshots_for_sessions it calls Wallaby.Browser.take_screenshot directly, and only for the session you want it to (you'll probably have to make an assumption as to how the main session binding is called in the test context map, e.g. just session). For reference: take_screenshots_for_sessions c) Propose additional option to start_session to register them with information that Wallaby shouldn't take screenshots for them, store that option in the session struct or Session store somehow and when taking screenshots, filter sessions according to that flag. I'm not promising the Wallaby team would accept such change

yonst commented 4 years ago

@michallepicki About a and b: How can I take screenshot by myself before the failure if I don't want to override wallaby code?