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

Upgrade from 'feature' to 'system' test #3568

Closed

Conversation

gonzar11
Copy link

@gonzar11 gonzar11 commented Mar 25, 2020

Description
I couldn't remove database cleaner because some specs on backends started to fail. Then I realized that it used just to truncate the database before starting run all specs. This is because some migrations are filling the database.

It closes #3564

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)
  • I have attached screenshots to this PR for visual changes (if needed)

@@ -94,3 +102,5 @@

Kernel.srand config.seed
end

Choose a reason for hiding this comment

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

Layout/TrailingBlankLines: 2 trailing blank lines detected.

config.before(:each, type: :system) do
driven_by :rack_test
end

Choose a reason for hiding this comment

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

Layout/TrailingWhitespace: Trailing whitespace detected.

ActionView::Base.raise_on_missing_translations = true

Capybara.default_max_wait_time = ENV['DEFAULT_MAX_WAIT_TIME'].to_f if ENV['DEFAULT_MAX_WAIT_TIME'].present?

ActiveJob::Base.queue_adapter = :test


Choose a reason for hiding this comment

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

Layout/EmptyLines: Extra blank line detected.

@gonzar11 gonzar11 force-pushed the gonzar11/switch-to-system-test branch 2 times, most recently from 02af214 to 3368775 Compare March 25, 2020 20:24
@@ -93,4 +101,4 @@
config.order = :random

Kernel.srand config.seed
end
end

Choose a reason for hiding this comment

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

Layout/TrailingBlankLines: Final newline missing.

@gonzar11 gonzar11 force-pushed the gonzar11/switch-to-system-test branch from 3368775 to 6f31382 Compare March 25, 2020 22:24
@@ -44,6 +44,7 @@
Capybara.exact = true

require "selenium/webdriver"
Capybara.javascript_driver = (ENV['CAPYBARA_DRIVER'] || :selenium_chrome_headless).to_sym
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this line necessary? isn't

config.before(:each, type: :system, js: true) do
  driven_by :selenium_chrome_headless
end

already taking care of defining the driver?

Copy link
Author

@gonzar11 gonzar11 Mar 30, 2020

Choose a reason for hiding this comment

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

Rails uses as default :selenium . If I don't define Capybara.javascript_driver, rails will assume that we use :selenium. In my case it fails because I didn't have geckodriver( Mozilla driver used with selenium).
Then with config.before(:each, type: :system, js: true) we redefine capybara Capybara javascript driver.

Copy link
Member

@kennyadsl kennyadsl Apr 3, 2020

Choose a reason for hiding this comment

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

That's odd. I checked the branch locally and tried commenting this line and all the driven_by below and the result is the same. I think that for some reason it's just using the default ignoring the driven_by calls.

Copy link
Author

Choose a reason for hiding this comment

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

I've tried to print which driver is being used. Please add this line puts Capybara.current_driver in both config.before(:each, type: :system) and config.before(:each, type: :system, js: true) before and after using driven_by. You will see the difference between use and don's use driven_by

Copy link
Member

Choose a reason for hiding this comment

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

Ok thanks, I'm giving it a try

Copy link
Member

Choose a reason for hiding this comment

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

if I comment out

# Capybara.javascript_driver = (ENV['CAPYBARA_DRIVER'] || :selenium_chrome_headless).to_sym

no matter what Capybara.current_driver prints, it is opening Firefox to run specs so it's definitely not headless.
I think there's some misconfiguration happening but I couldn't get exactly where yet.

I've also tried with this syntax, which seems to be the default to make exactly the same thing that we do with our custom driver:

driven_by :selenium, using: :headless_chrome, screen_size: [1920, 1080]

but it still opens firefox to run specs. 😢

Copy link
Member

Choose a reason for hiding this comment

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

@gonzar11 did you get the chance to take a look?

Copy link
Contributor

@blocknotes blocknotes May 25, 2020

Choose a reason for hiding this comment

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

@gonzar11, I worked on a similar PR in a Solidus extension (solidus_starter_frontend) and I would like to add my thoughts.

  • Capybara.javascript_driver = ... is not necessary; Rails testing guide (but for Minitest) says that default Capybara driver settings should be changed via driven_by;
  • in my PR I choose to have 2 different variables (CAPYBARA_DRIVER/CAPYBARA_JS_DRIVER), it can be useful sometimes;
  • I made some tests in this PR commenting out the Capybara.javascript_driver = ... line in spec_helper.rb; running a JS spec with CAPYBARA_DRIVER=selenium_chrome bin/rspec backend/spec/system/admin/configuration/payment_methods_spec.rb:36 first opens a Firefox empty window then a Chrome window with the specified test.
    As a test, if I comment out the following lines, I see that it works as expected:
    if RSpec.current_example.metadata[:js] && page.driver.browser.respond_to?(:url_blacklist)
      page.driver.browser.url_blacklist = ['http://fonts.googleapis.com']
    end

@gonzar11 gonzar11 force-pushed the gonzar11/switch-to-system-test branch from 6f31382 to 004b9c7 Compare March 30, 2020 13:30
end

config.before(:each, type: :system, js: true) do
driven_by (ENV['CAPYBARA_DRIVER'] || :selenium_chrome_headless).to_sym

Choose a reason for hiding this comment

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

Lint/ParenthesesAsGroupedExpression: (...) interpreted as grouped expression.

end

config.before(:each, type: :system, js: true) do
driven_by (ENV['CAPYBARA_DRIVER'] || :selenium_chrome_headless).to_sym

Choose a reason for hiding this comment

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

Lint/ParenthesesAsGroupedExpression: (...) interpreted as grouped expression.

@@ -44,6 +44,7 @@
Capybara.exact = true

require "selenium/webdriver"
Capybara.javascript_driver = (ENV['CAPYBARA_DRIVER'] || :selenium_chrome_headless).to_sym
Copy link
Contributor

@blocknotes blocknotes May 25, 2020

Choose a reason for hiding this comment

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

@gonzar11, I worked on a similar PR in a Solidus extension (solidus_starter_frontend) and I would like to add my thoughts.

  • Capybara.javascript_driver = ... is not necessary; Rails testing guide (but for Minitest) says that default Capybara driver settings should be changed via driven_by;
  • in my PR I choose to have 2 different variables (CAPYBARA_DRIVER/CAPYBARA_JS_DRIVER), it can be useful sometimes;
  • I made some tests in this PR commenting out the Capybara.javascript_driver = ... line in spec_helper.rb; running a JS spec with CAPYBARA_DRIVER=selenium_chrome bin/rspec backend/spec/system/admin/configuration/payment_methods_spec.rb:36 first opens a Firefox empty window then a Chrome window with the specified test.
    As a test, if I comment out the following lines, I see that it works as expected:
    if RSpec.current_example.metadata[:js] && page.driver.browser.respond_to?(:url_blacklist)
      page.driver.browser.url_blacklist = ['http://fonts.googleapis.com']
    end

@waiting-for-dev
Copy link
Contributor

Hey @gonzar11, it's been a while, but would you be willing to keep working on it? 🙂

@kennyadsl kennyadsl added the type:enhancement Proposed or newly added feature label Aug 24, 2022
@waiting-for-dev waiting-for-dev added the changelog:solidus_backend Changes to the solidus_backend gem label Aug 30, 2022
@kennyadsl
Copy link
Member

Closing as this has no activity anymore. Thanks anyway!

@kennyadsl kennyadsl closed this Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_backend Changes to the solidus_backend gem type:enhancement Proposed or newly added feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Swtich feature tests to system tests
6 participants