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

switching between truncation and transaction clean-up #2619

Merged
merged 1 commit into from
Oct 4, 2021

Conversation

akostadinov
Copy link
Contributor

@akostadinov akostadinov commented Sep 19, 2021

#2579 switched from default truncation clean-up to default transaction clean-up. Switching between the two only worked for @javascript tagged scenarios instead of the intended list of scenario tags. It turned out to be a problem for postgres database and Groups and permissions cucumber feature.

This PR removes special @javascript tag handling by cucumber-rails (by implementing a null strategy) and uses simple DatabaseCleaner strategy switching in Before hooks.

For more information about @javscript handling see documentation at [1], in-built hooks [2] and the strategies these hooks apply to [3]. Also a discussion describing the different kinds of problems cucumber/cucumber-rails#166.

[1] https://github.com/cucumber/cucumber-rails/blob/561ee249c418f1c8cd7b629e7cf414442ce62709/features/choose_javascript_database_strategy.feature
[2] https://github.com/cucumber/cucumber-rails/blob/561ee249c418f1c8cd7b629e7cf414442ce62709/lib/cucumber/rails/hooks/active_record.rb
[3] https://github.com/cucumber/cucumber-rails/tree/0b2a8fddc74dff6f773468b919ed9f625685db2a/lib/cucumber/rails/database
[4] cucumber/cucumber-rails#166

@akostadinov akostadinov self-assigned this Sep 19, 2021
@akostadinov akostadinov changed the title [WIP] test PR to check Postgres issue switching between truncation and transaction clean-up Sep 23, 2021
Presently cucumber-rails is exercising special treatment of `@javascript`
tagged scenarios [1] wrt connection sharing between threads by
pre-defined hooks [2].

To prevent that and allow switching between desired mode of operation
depending on scenario tags, we implement a custom clean-up strategy class and change DatabaseCleaner strategy in our own hooks.

[1] https://github.com/cucumber/cucumber-rails/blob/561ee249c418f1c8cd7b629e7cf414442ce62709/features/choose_javascript_database_strategy.feature
[2]
https://github.com/cucumber/cucumber-rails/blob/561ee249c418f1c8cd7b629e7cf414442ce62709/lib/cucumber/rails/hooks/active_record.rb
@@ -1,3 +1,4 @@
@no-txn
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be needed. It was not before upgrade to Cucumber 7. Also it is not needed for MySQL

I think the issue lies on some DB feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hallelujah , before #2579 test ran in truncation mode. If you check references in description, especially cucumber/cucumber-rails#166 people report that drivers are not thread safe and strange effects may appear.

This PR makes switching between truncation and transaction mode work. Before it, only @javascript tagged scenarios ran in truncation mode.

I don't know what kind of database feature may affect this scenario. Please provide any suggestions. It might be something specific about how Postgresql driver works because some people reported one thread doing a query and another thread receiving the result.

end

Before non_transactional.join(' or ') do
Cucumber::Rails::Database.javascript_strategy = :truncation
Cucumber::Rails::Database.before_non_js if Cucumber::Rails::Database.autorun_database_cleaner
DatabaseCleaner.strategy = :truncation
Copy link
Contributor

Choose a reason for hiding this comment

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

The Cucumber::Rails::Database is supposed to abstract the underlying DatabaseCleaner

I think we should not expose DatabaseCleaner here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not. Cucumber::Rails::Database.javascript_strategy only affects @javascript tagged scenarios and nothing else.

See https://github.com/cucumber/cucumber-rails/blob/561ee249c418f1c8cd7b629e7cf414442ce62709/lib/cucumber/rails/hooks/active_record.rb

I have another commit that allows switching between truncation, transaction with threading hack and transaction without threading hack but it is needlessly complicated and we don't need it. See 2a16ffbeab79b9258a200b3d6dd8621e3223b7f7. Also see my cucumber/cucumber-rails#521 where I discuss this with upstream project.

# The :transaction strategy is faster, but might give you threading problems.
# See https://github.com/cucumber/cucumber-rails/blob/master/features/choose_javascript_database_strategy.feature
Cucumber::Rails::Database.javascript_strategy = :truncation
require_relative 'cucumber_database_null_javascript_strategy'
Cucumber::Rails::Database.javascript_strategy = ::ThreeScale::CucumberDatabaseNullJavascriptStrategy
Copy link
Contributor

@hallelujah hallelujah Sep 28, 2021

Choose a reason for hiding this comment

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

I do not understand why we had to separate the concerns.

The cucumber database strategy should call before_js and such ... Those methods then set/unset the DatabaseCleaner strategy. Now you are separating concerns which is confusing.
I think the issue is the order of when those methods are called (as they are callbacks)

# I think this should be enough:

Before non_transactional do
  Cucumber::Rails::Database.javascript_strategy = :truncation
end

But the callbacks should be called like this:

  • Cucumber::Rails::Database.javascript_strategy = :truncation
  • Cucumber::Rails::Database.strategy.before_js
  • run your tests
  • Cucumber::Rails::Database.strategy.after

My guess is that the order of the call of the callbacks is wrong

The issue is probably before_js (of transaction strategy) is called before setting the javascript_strategy = :truncation ... which makes it enter a transaction then truncate which will autocommit as it is a DDL statement

This is what I debugged for MySQL. And I am deeply sorry I did not dig deeper for other DBs (really my fault) as I was supposing it was the same for other DB

Probably there is something either in the code or in the cucumber-* or rails-* or oracle-* gems that creates this issue for Postgres and Oracle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In-built hooks run after our defined hooks and mess-up everything. They only regard @javascript and not @javascript so our tag lists become irrelevant.

Copy link
Contributor

@hallelujah hallelujah left a comment

Choose a reason for hiding this comment

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

:shipit:

@hallelujah hallelujah merged commit 3b443ee into 3scale:master Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants