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

Capybara driver for Selenium needs to clean localStorage between tests #1702

Closed
taw opened this issue May 16, 2016 · 11 comments
Closed

Capybara driver for Selenium needs to clean localStorage between tests #1702

taw opened this issue May 16, 2016 · 11 comments

Comments

@taw
Copy link

taw commented May 16, 2016

I had cross-contamination between tests, which was a real hell to debug.

For workaround I ended up doing this:

RSpec.configure do |config|
  # Driver used
  Capybara.register_driver :chrome do |app|
    Selenium::WebDriver::Chrome.path = AppConfig.selenium_chrome_path if AppConfig.selenium_chrome_path?
    Capybara::Selenium::Driver.new(app, browser: :chrome)
  end
  Capybara.javascript_driver = :chrome

  # Workaround
  config.after do
    if self.class.include?(Capybara::DSL)
      if Capybara.current_driver == :chrome
        Capybara.execute_script "localStorage.clear()"
      end
    end
  end
end

But it's really a bug in capybara that it allows this cross-test contamination, and it should clear localStorage as well as any other kind of HTML5 browser state between tests.

Presumably this kind of code ought to go to Capybara::Selenium::Driver#reset!, and to similar places in other drivers supporting such storage mechanisms.

@twalpole
Copy link
Member

twalpole commented May 16, 2016

This has been discussed previously and the performance hit for doing this every test when most apps don't depend on localStorage was decided to be not worth it. If you have a need for it adding an after block as you have works fine. This is documented in the Session#reset method - http://www.rubydoc.info/gems/capybara/Capybara/Session#reset%21-instance_method - PRs documenting it in the README would be considered

@taw
Copy link
Author

taw commented May 16, 2016

This feels like a very bad decision. What developer sees without this reset is tests failing randomly (based on random seed) on CI, but every one of them working just fine when ran individually every time - and none of the standard ways to debug it are going to be helpful.

This hacky after hook (which I think only works reliably enough for single origin tests suites, there ought to be a more proper way) is not terribly complicated, but figuring out that your random CI fails have anything to do with capybara leaking state between tests is extremely difficult to debug. Even if documentation said something about localStorage (which is far more likely to be used by some framework than by app directly, so developer probably won't even know), there's no reasonable way to guess that.

This is a bug in capybara, there's really no other way to look at it. The only reasonable alternative to cleaning localStorage between tests would be to tell the browser to disable localStorage completely (--disable-local-storage for chrome). Having contaminated localStorage is just wrong.

And for that matter, does it really have significant performance impact? It feels unlikely compared with tons of setup being ran for every test.

@twalpole
Copy link
Member

@taw The fact that this has come up a couple of times in the last 3 years shows how little it actually affects people. The "hacky"??? after hook is exactly how it would be implemented in Capybara if it ever was, so I'm not really sure what your issue with that is.

As stated in the Session#reset documentation - the decision was made not to reset localstorage since Capybara has no way of knowing everything that an app would need to have reset between tests, so the developer has to make that decision. If that doesn't work for you, you are free to use any other free testing framework you feel like. If instead you want to make it clearer to others by submitting a PR for the README about session resets that would be considered.

@taw
Copy link
Author

taw commented May 17, 2016

Considering how difficult to debug problems like this are, the fact that it showed up multiple times suggest it probably affects a lot of people, but they just shrug it off with "capybara is inherently unreliable" (a very common attitude).

@twalpole twalpole removed the Feature label May 17, 2016
@srghma
Copy link

srghma commented Nov 24, 2017

@taw Thanks for script
this worked for me

RSpec.configure do |config|
  config.prepend_after(:each, type: :feature) do
    Capybara.execute_script 'localStorage.clear()'
  end
end

@liapretorius
Copy link

Just letting ya'll know that I'm running into this right now.

@taw
Copy link
Author

taw commented Jun 12, 2018

@liapretorius Yeah, the more people use localStorage and such features, the more automatic cleanup is needed.

@twalpole
Copy link
Member

@liapretorius This has been available since Capybara 2.11.0 via the clear_local_storage and clear_session_storage options for the Selenium driver.

swills pushed a commit to swills/gitlabhq that referenced this issue Aug 1, 2018
This is not done by default as it is said to incur a performance hit,
paired with local storage not always being used by the site being
tested.
(teamcapybara/capybara#1702 (comment))

GitLab uses localStorage, for things like remembering which tab you used
last (on the login page for example, between sign-in, ldap, and register)

Fixes: https://gitlab.com/gitlab-org/gitlab-qa/issues/303
@rbu
Copy link

rbu commented Nov 7, 2018

@twalpole how do you feel about making clear_session_storage and clear_local_storage the default with an update?

@twalpole
Copy link
Member

twalpole commented Nov 7, 2018

@rbu Not great because it has a few edge cases that will just lead to more confusion - It's probably possible to make it work fully properly with Chrome via direct CDP calls but not with any other browser at the moment. I did some work to make it a bit more robust in #1853 but never really finished that up

@twalpole
Copy link
Member

@rbu Thinking about this more - it's probably fine to make it the default since it shouldn't really break anything, is easy enough to disable, and the edge cases still exist anyway.

@lock lock bot locked and limited conversation to collaborators Aug 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants