Remove redundant configs for Capybara #3947

mayorova · 2024-11-26T09:32:03Z

What this PR does / why we need it:

There are some configs for Capybara that I believe are not needed - see comments inline.

Which issue(s) this PR fixes

Verification steps

Make sure all tests works :)

Special notes for your reviewer:

features/support/capybara.rb

mayorova · 2024-11-26T09:38:36Z

features/support/capybara.rb

-  Capybara.current_driver = DEFAULT_JS_DRIVER
-end
-
-Before '@chrome' do


This is done by https://github.com/teamcapybara/capybara/blob/3.40.0/lib/capybara/cucumber.rb#L22-L27

mayorova · 2024-11-26T09:39:08Z

features/support/capybara.rb

-Capybara.default_driver = :rack_test
-Capybara.javascript_driver = DEFAULT_JS_DRIVER
-Capybara.default_selector    = :css
-Capybara.disable_animation = true
-
-# Capybara 3 changes the default server to Puma. It can be reverted to the previous default of WEBRick by specifying:
-Capybara.server = :webrick


I think it doesn't make sense to set the same configs both here, and then inside the Capybara.configure block. They seem to have the same effect.

mayorova · 2024-11-26T09:40:00Z

features/support/capybara.rb

-  Capybara.current_driver = :firefox
+  # Capybara 3 changes the default server to Puma. It can be reverted to the previous default of WEBRick by specifying:
+  config.server = :webrick
+  config.disable_animation = true


This disable_animation is the only one that is not documented here: https://rubydoc.info/github/jnicklas/capybara/Capybara#configure-class_method

But the tests pass, so either it works fine, or this setting is irrelevant.

In this case I would remove it and see if the tests pass. The more we can cleanup the better.

codecov · 2024-11-26T10:17:08Z

Codecov Report

Attention: Patch coverage is 64.28571% with 10 lines in your changes missing coverage. Please review.

Project coverage is 93.80%. Comparing base (b002a35) to head (d757111).
Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
features/support/capybara.rb	54.54%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3947      +/-   ##
==========================================
- Coverage   93.85%   93.80%   -0.05%     
==========================================
  Files        3001     2999       -2     
  Lines       97351    96925     -426     
  Branches      629      629              
==========================================
- Hits        91368    90924     -444     
- Misses       5959     5977      +18     
  Partials       24       24

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

akostadinov

looks nice! good to check if there are useful ideas also in #3911

mayorova · 2024-11-26T11:09:38Z

looks nice! good to check if there are useful ideas also in #3911

Oh wow! I missed that one completely 🤦

Thanks, I'll take a look.

jlledom · 2024-12-05T16:44:04Z

features/step_definitions/provider_steps.rb

@@ -237,6 +237,8 @@ def setup_provider(login)
  set_current_domain(@provider.external_admin_domain)
  stub_integration_errors_dashboard

+  @provider.users.first.user_sessions.create! # Prevents welcome flash from showing up


I understand the reason, but doesn't this make the login param useless? it will create a session anyway... and doesn't this break any test?

features/step_definitions/web_steps.rb

mayorova · 2024-12-09T16:32:21Z

features/step_definitions/pagination_steps.rb

@@ -22,7 +22,8 @@
      click_link page
    end
  else
-    input = find('input[aria-label="Current page"]')
+    # Pagination controls are rendered twice: at the top and at the bottom of the data table
+    input = find('input[aria-label="Current page"]', match: :first)


This change is not really needed anymore, it was needed when I tried config.match = :smart match mode for Cabybara.

But probably we can leave it here to state that we are aware of this duplication, and have it fixed, if we ever switch to :smart mode again. (currently there were too many changes to perform here)

mayorova · 2024-12-09T16:33:49Z

features/support/capybara.rb

 Capybara.configure do |config|
  config.default_driver = :rack_test
-  config.javascript_driver = DEFAULT_JS_DRIVER
+  config.default_selector = :css


These first three lines are actually the defaults, so they could be removed potentially. I decided to keep them, because I think it's clearer if they are explicit and not hidden within Capybara's code (or docs).

But if anybody thinks they are redundant, we can drop them.

Did you finally remove them?

akostadinov · 2024-12-09T18:48:39Z

features/step_definitions/cms/sidebar_steps.rb

@@ -5,8 +5,6 @@
 end

 When /^I switch to (builtin|3scale) content$/ do |group|
-  ensure_javascript


why is this removed?

I guess it's not strictly needed 🤔
I imagine that if this is used in a non-JS scenario, then just some following step will fail.

I think it's nice to stay so that we know this is expected to fail without js. Otherwise it may require more cycles to figure out.

features/support/capybara.rb

akostadinov · 2024-12-09T18:54:23Z

features/support/capybara.rb

-Capybara.register_driver :firefox do |app|
-  Capybara::Selenium::Driver.new(app, browser: :firefox)
-end


we wont be able to run with firefox?

Apparently, not 😬
Again, I just applied Josemi's suggestion.

I don't think I have run automated tests in firefox myself, but if you have and want to bring it back, I'll do it.

Yeah, I think it is nice to be able to run something with it in case we see strange results with chrome. This is the only browser we ship with RHEL. And Chrome is evil.

I agree Chrome is evil, in fact we should use only Firefox, but I guess clients use Chrome. IMO it's fine to keep both.

Remove session creation Revert to :prefer_exact capybara matching Remove default capybara values

features/support/capybara.rb

features/support/0001_world.rb

jlledom · 2024-12-12T08:26:40Z

features/support/capybara.rb

 Capybara.configure do |config|
  config.default_driver = :rack_test
-  config.javascript_driver = DEFAULT_JS_DRIVER
+  config.default_selector = :css


Did you finally remove them?

features/support/capybara.rb

mayorova commented Nov 26, 2024

View reviewed changes

features/support/capybara.rb Show resolved Hide resolved

mayorova commented Nov 26, 2024

View reviewed changes

akostadinov previously approved these changes Nov 26, 2024

View reviewed changes

jlledom previously approved these changes Nov 26, 2024

View reviewed changes

mayorova dismissed stale reviews from jlledom and akostadinov via 4203836 December 5, 2024 14:08

jlledom reviewed Dec 5, 2024

View reviewed changes

mayorova force-pushed the refactor-capybara branch from 4203836 to 695ecf7 Compare December 9, 2024 12:22

mayorova commented Dec 9, 2024

View reviewed changes

mayorova mentioned this pull request Dec 9, 2024

🚧🥒 Reduce Capybara / Cucumber config #3911

Draft

akostadinov reviewed Dec 9, 2024

View reviewed changes

features/support/capybara.rb Show resolved Hide resolved

akostadinov reviewed Dec 9, 2024

View reviewed changes

mayorova added 6 commits December 10, 2024 11:27

Remove redundant configs for Capybara

0af3143

Some improvements from #3911

226b48c

Remove session creation Revert to :prefer_exact capybara matching Remove default capybara values

Fix test with pagination

b3b311c

Bring back ensure_javascript

6131036

Refactoring

424140f

Restore @Firefox driver for capybara

d757111

mayorova force-pushed the refactor-capybara branch from 0ad4d90 to d757111 Compare December 10, 2024 15:43

jlledom reviewed Dec 12, 2024

View reviewed changes

mayorova commented Dec 12, 2024

View reviewed changes

features/support/capybara.rb Show resolved Hide resolved

akostadinov approved these changes Dec 18, 2024

View reviewed changes

jlledom approved these changes Dec 18, 2024

View reviewed changes

mayorova merged commit 701fac1 into master Dec 19, 2024
32 of 35 checks passed

mayorova deleted the refactor-capybara branch December 19, 2024 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove redundant configs for Capybara #3947

Remove redundant configs for Capybara #3947

mayorova commented Nov 26, 2024

mayorova Nov 26, 2024

mayorova Nov 26, 2024

mayorova Nov 26, 2024

jlledom Nov 26, 2024

codecov bot commented Nov 26, 2024 •

edited

Loading

akostadinov left a comment

mayorova commented Nov 26, 2024

jlledom Dec 5, 2024

mayorova Dec 9, 2024

mayorova Dec 9, 2024

jlledom Dec 12, 2024

akostadinov Dec 9, 2024

mayorova Dec 10, 2024

akostadinov Dec 10, 2024

akostadinov Dec 9, 2024

mayorova Dec 9, 2024

akostadinov Dec 9, 2024

jlledom Dec 12, 2024

jlledom Dec 12, 2024

Remove redundant configs for Capybara #3947

Remove redundant configs for Capybara #3947

Conversation

mayorova commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 26, 2024 • edited Loading

Codecov Report

akostadinov left a comment

Choose a reason for hiding this comment

mayorova commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 26, 2024 •

edited

Loading