-
Notifications
You must be signed in to change notification settings - Fork 365
Cypress on rails #9633
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
Cypress on rails #9633
Conversation
|
||
if has_id_column | ||
# Get the maximum id using SQL | ||
max_id = connection.select_value("SELECT MAX(id) FROM #{connection.quote_table_name(table_name)}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, getting the max IDs when initializing the SeededDeletion object, so we can then ensure the cleanup will remove any newer IDs for each table... any table with no rows, will be skipped.
We can change this to use sequence but first wanted to get this here so it's reviewable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be a way to do this all in a single query even without sequences.
} | ||
); | ||
}); | ||
cy.app('clean') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an example of what this PR enables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. I'd love to move this out to a dedicated, published, gem if possible.
Enables: ManageIQ/manageiq-ui-classic#9633 We add this to core so the railtie in cypress-on-rails can hook into the rails app startup as needed: https://github.com/shakacode/cypress-playwright-on-rails/blob/d50de890b345879205124fa4de52bf899d9df2d0/lib/cypress_on_rails/railtie.rb#L6 Note, this could be pushed down to UI Classic, see below, but this has a few problems: 1) Core already has bundler groups for development and test that these nicely fit into. This allows us to easily exclude them from different installation scenarios such as smaller deployments with just the required runtime dependencies. 2) UI-Classic is a gem engine that gets pulled in, so only the runtime dependencies are resolved and available. Therefore, we would need to add these test gems as runtime dependencies which sends the wrong message and makes it difficult to exclude. Alternative setup (not suggested): a) Remove it from core: ```diff --git a/Gemfile b/Gemfile index cd45015..b62ae61 100644 --- a/Gemfile +++ b/Gemfile @@ -326,7 +326,4 @@ group :development, :test do gem "parallel_tests", "~>4.4", :require => false gem "routes_lazy_routes" gem "rspec-rails", "~>7.0" - gem "cypress-on-rails", "~>1.17.0" # Need railtie to be loaded with rails app - gem "factory_bot_rails" - gem "database_cleaner", "~>2.1" end ``` b) Require it in UI-Classic's Engine and add the gems to the gemspec: ```diff --git a/lib/manageiq/ui/classic/engine.rb b/lib/manageiq/ui/classic/engine.rb index dc72a001ec..cf420620c1 100644 --- a/lib/manageiq/ui/classic/engine.rb +++ b/lib/manageiq/ui/classic/engine.rb @@ -36,6 +36,8 @@ module ManageIQ config.assets.js_compressor = :manageiq_ui_classic_js_compressor end + require 'cypress-on-rails' if ENV['CYPRESS'].present? + def self.vmdb_plugin? true end diff --git a/manageiq-ui-classic.gemspec b/manageiq-ui-classic.gemspec index cef587a45a..3ed378abed 100644 --- a/manageiq-ui-classic.gemspec +++ b/manageiq-ui-classic.gemspec @@ -31,7 +31,10 @@ Gem::Specification.new do |s| s.add_dependency "uglifier", "~>4.2.0" s.add_dependency "webpacker", "~>2.0.0" - s.add_development_dependency "cypress-on-rails", "~> 1.0" + s.add_dependency "cypress-on-rails", "~>1.17.0" + s.add_dependency "factory_bot_rails" + s.add_dependency "database_cleaner", "~>2.1" + ```
cf0616c
to
c367e95
Compare
c367e95
to
7a4f47d
Compare
Enables: ManageIQ/manageiq-ui-classic#9633 We add this to core so the railtie in cypress-on-rails can hook into the rails app startup as needed: https://github.com/shakacode/cypress-playwright-on-rails/blob/d50de890b345879205124fa4de52bf899d9df2d0/lib/cypress_on_rails/railtie.rb#L6 Note, this could be pushed down to UI Classic, see below, but this has a few problems: 1) Core already has bundler groups for development and test that these nicely fit into. This allows us to easily exclude them from different installation scenarios such as smaller deployments with just the required runtime dependencies. 2) UI-Classic is a gem engine that gets pulled in, so only the runtime dependencies are resolved and available. Therefore, we would need to add these test gems as runtime dependencies which sends the wrong message and makes it difficult to exclude. Alternative setup (not suggested): a) Remove it from core: ```diff --git a/Gemfile b/Gemfile index cd45015..b62ae61 100644 --- a/Gemfile +++ b/Gemfile @@ -326,7 +326,4 @@ group :development, :test do gem "parallel_tests", "~>4.4", :require => false gem "routes_lazy_routes" gem "rspec-rails", "~>7.0" - gem "cypress-on-rails", "~>1.17.0" # Need railtie to be loaded with rails app - gem "factory_bot_rails" - gem "database_cleaner", "~>2.1" end ``` b) Require it in UI-Classic's Engine and add the gems to the gemspec: ```diff --git a/lib/manageiq/ui/classic/engine.rb b/lib/manageiq/ui/classic/engine.rb index dc72a001ec..cf420620c1 100644 --- a/lib/manageiq/ui/classic/engine.rb +++ b/lib/manageiq/ui/classic/engine.rb @@ -36,6 +36,8 @@ module ManageIQ config.assets.js_compressor = :manageiq_ui_classic_js_compressor end + require 'cypress-on-rails' if ENV['CYPRESS'].present? + def self.vmdb_plugin? true end diff --git a/manageiq-ui-classic.gemspec b/manageiq-ui-classic.gemspec index cef587a45a..3ed378abed 100644 --- a/manageiq-ui-classic.gemspec +++ b/manageiq-ui-classic.gemspec @@ -31,7 +31,10 @@ Gem::Specification.new do |s| s.add_dependency "uglifier", "~>4.2.0" s.add_dependency "webpacker", "~>2.0.0" - s.add_development_dependency "cypress-on-rails", "~> 1.0" + s.add_dependency "cypress-on-rails", "~>1.17.0" + s.add_dependency "factory_bot_rails" + s.add_dependency "database_cleaner", "~>2.1" + ```
a85a7f8
to
ce91a4f
Compare
cypress/support/e2e.js
Outdated
// capture the database table state before all tests | ||
before(() => { | ||
cy.appDbState('capture'); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This runs the capture once before all tests.
if defined?(VCR) | ||
VCR.eject_cassette # make sure we no cassette inserted before the next test starts | ||
VCR.turn_off! | ||
WebMock.disable! if defined?(WebMock) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, I was going to put this into a class/module but it's evaluated in the context of the cypress on rails command executor so finding/prepending classes was more effort than needed. If we want to move the logic into methods, we can do that later.
cypress/e2e/ui/Automation/Embedded-Automate/Explorer/class.cy.js
Outdated
Show resolved
Hide resolved
cypress/e2e/ui/Automation/Embedded-Automate/Explorer/namespace.cy.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we drop this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as below... I think we can...it's from their generator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, it's now used as suggested below... for our custom commands.
|
||
// Import commands.js using ES2015 syntax: | ||
// import 'cypress-on-rails/support/index' | ||
import './commands' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the purpose of these commands are when we already have command support elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, this is biolerplate stuff from the generator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words, it will return when we run the generator for future versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file generated? If so, maybe we want to move our custom commands to commands.js instead of making changes to this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM - some minor style comment and organizational stuff, but I'd be happy to merge as is and do that as followup. @jrafanie let me know what you want to do.
Let's get this merged. I think there's enough valid points to address in a followup PR. I'd like to also extract the gem and use it so this PR would just keep growing. I want to make sure we get this in and verify tests continue to pass without issue. |
bundle exec rails g cypress_on_rails:install --install_with=skip index.js contents were moved to e2e.js Other files were moved within cypress to the right locations. commented out stuff from cypress-on-rails in cypress.config.js
* paths are different * drop the examples for fixtures, factories, vcr, scenarios, etc. They can be found here: https://github.com/shakacode/cypress-playwright-on-rails/tree/d50de890b345879205124fa4de52bf899d9df2d0/lib/generators/cypress_on_rails/templates/spec/cypress/e2e/rails_examples
* Use Engine path for cypress directory * Guard using CYPRESS ENV * Remove not needed changes to cypress.config.js * Drop log_fail and add instructions on how to add it later if needed
d318caf
to
9b92f6c
Compare
* Add a start/setup hook to cache db ids before the test start * Create the strategy once in the initializer, so that start and clean can use it. * Create db_state command and route to capture/restore
If the block passed to disable referential integrity fails, triggers could be left disabled, so instead, make sure the disable, block contents, and enable are run in a transaction so they all run or nothing. Similar to https://www.github.com/rails/rails/pull/50196
Fix thread safety issue in rails #53883 This was causing random errors in the db restore, generally during the first run of the cypress test per rails boot. Sometimes, it would hang. Other times, I'd get a segmentation fault. Other times, Bad file descriptor (Errno::EBADF) in actioncable-7.2.2.2/lib/action_cable/subscription_adapter/postgresql.rb:107:in `wait_for_notify'. I found that the prior commit helps to ensure the trigger disable and enable are both run and not just disabled in the the case of an error. But, on top of that, I found that rails had fixed a thread safety issue in action cable subscriptions's usage of a database connections. See: https://www.github.com/rails/rails/pull/53891 and https://www.github.com/rails/rails/issues/53883 "Action Cable must avoid using a pinned connection to subscribe to notifications as it can't be done in a thread safe way. So it must ensure it has a dedicated connection for that."
Note: By default, Factory bot sets definition_file_paths to directories to search for factories: https://github.com/thoughtbot/factory_bot/blob/8446cb6c5b39ea046d8ba180197aabc66adf62ed/lib/factory_bot/find_definitions.rb#L10 Cypress on rails SmartFactoryWrapper is expecting files to be a pattern or a file that when evaluated with Dir[xxx], will return files, not a directory: https://github.com/shakacode/cypress-playwright-on-rails/blob/abb505c0691c29d5f2a57c2ba28aedbfd43d079e/lib/cypress_on_rails/smart_factory_wrapper.rb#L88 Therefore, to reuse the existing definition_file_paths, we must convert the directories to glob pattern matches.
Since the rails app Gemfile has these configured with require false, we need to do the require early in the engine. config.before_initialize is NOT early enough. config.before_configuration happens before config/application is required. Note, before_initialize didn't work as evidenced by the middlware not processing the requests for /__e2e__/command.
9b92f6c
to
1191a31
Compare
POC of using cypress on rails for factories, but also can be used for VCR, scenarios, and fixtures.
Core adds the gems as a dependency but we require them before rails configuration to ensure the railtie in cypress-on-rails is required early enough.
Depends on:
I've added a few test conversions to use factories and/or the db
cy.appDbState('restore');
.For now, we do the
cy.appDbState('capture');
before the tests to capture the max row ids for each table and the restore drops any new rows created beyond these max values. Note, it doesn't pick up changes to existing rows so you need to be careful about rolling back any changes to these on your own.New exposed endpoints
Note, in cypress mode, rails server run with CYPRESS=true ENV variable, there is a endpoint we can use to run these new commands and factories. Obviously, don't bypass the guards and do this in production.
I tested a few of these endpoints:
URL:
http://localhost:3000/__e2e__/command
POST
JSON body:
Captures the current max ids per table:
{"name":"db_state","options":"capture"}
Drops any new rows since capture was run:
{"name":"db_state","options":"restore"}
Create a new automation domain with TestDomain name:
{"name":"factory_bot","options":[["create","miq_ae_domain",{"name":"TestDomain"}]]}
Note, because small environment currently depends on EvmSpecHelper, which it calls local_miq_server, which requires rspec-mocks, which we use to stub the local server (and rspec-mocks doesn't want to run outside of tests, we need to comment out the creation of the local_miq_server or create a new factory that doesn't contain it:
{"name":"factory_bot","options":[["create","small_environment"]]}
Note2, these endpoints are due to be changed as we use it and find better terminology for each, but we should be able to create nearly all factories in cypress via these command/endpoints or from a script. We should be able to create compound factories similar to small environment for the purpose of testing various setups.