Replace generated dummy test_app with single file rails application #2281

jhawthorn · 2017-10-12T04:14:11Z

This PR removes the need to run rake test_app before running specs. Migrations are automatically run before specs if the database is new or has missing migrations.

$ rspec
Re-running migrations
rake db:reset VERBOSE=false
Dropped database 'db/solidus_backend_test.sqlite3'
Created database 'db/solidus_backend_test.sqlite3'

Randomized with seed 14974
..................

This is done by adding a checked-in single-file(ish) Rails application requirable at spree/testing_support/dummy_app. This replaces the need to generate the spec/dummy application, and allows us to make changes to the dummy app's configuration and have them apply without re-running rake test_app.

The rake test_app task has been replaced by a new rake db:reset task, which calls db:drop, db:create, and db:migrate as one would expect. Most users will never have to run this, as the spec_helper for each project will detect the need to re-run migrations and automatically run rake db:reset. test_app has been kept as an alias for db:reset, which will be helpful to keep in the short-term to allow git bisects. Top-level rake tasks have been similarly adjusted.

jhawthorn@muta:~/src/solidus/core
$ rake -T
rake console     # Open a sandboxed console in the test environment
rake db:create   # Creates the test database
rake db:drop     # Drops the test database
rake db:migrate  # Migrates the test database
rake db:reset    # Recreates the test database and re-runs all migrations
rake spec        # Run RSpec code examples

A new rake console command opens up a sandboxed (in a transaction) pry console in the test environment. I'm hoping this meets most of the needs of developers who would previously cd into spec/dummy and fire up rails c.

jhawthorn@muta:~/src/solidus/core
$ rake console
Loading test environment in sandbox (Rails 5.1.4)
Any modifications you make will be rolled back on exit
[1] pry(main)> require "spree/testing_support/factories"
[2] pry(main)> order = FactoryGirl.create(:order)
[3] pry(main)> variant = FactoryGirl.create(:variant)
[4] pry(main)> order.contents.add(variant)
=> #<Spree::LineItem:0x0055fb94895220
 id: 1,
 variant_id: 2,
 order_id: 2,
 quantity: 1,
 price: BigDecimal(19.99),
 created_at: Fri, 13 Oct 2017 22:32:55 UTC +00:00,
 updated_at: Fri, 13 Oct 2017 22:32:55 UTC +00:00,
 cost_price: BigDecimal(17.0),
 tax_category_id: 1,
 adjustment_total: BigDecimal(0.0),
 additional_tax_total: BigDecimal(0.0),
 promo_total: BigDecimal(0.0),
 included_tax_total: BigDecimal(0.0)>
[4] pry(main)> exit
   (0.4ms)  rollback transaction

Todo:

Convert api and frontend
Investigate entirely eliminating rake test_app (auto run migrations)
Rebase and make sensible commits
Prefix databases with project name
I haven't investigated using this is extensions, but I would like to tackle separately

houndci-bot · 2017-10-12T04:14:22Z

core/lib/spree/testing_support/dummy_app.rb

+      end
+    end
+    config.paths['db/migrate'].concat migration_dirs
+    #binding.pry


Missing space after #.

houndci-bot · 2017-10-12T04:14:22Z

core/lib/spree/testing_support/dummy_app.rb

+    config.secret_key_base                            = 'SECRET_TOKEN'
+
+    # FIXME: needs to add ALL engines
+    #config.paths['db/migrate'] << File.expand_path('../../../../db/migrate', __FILE__)


Missing space after #.

houndci-bot · 2017-10-12T04:14:22Z

core/lib/spree/testing_support/dummy_app.rb

+  end
+
+  def self.rails_root
+    Pathname.new(gem_root).join(*%w[spec dummy])


Pass array contents as separate arguments.

houndci-bot · 2017-10-12T04:14:22Z

core/lib/spree/testing_support/dummy_app.rb

+
+module Dummy
+  def self.gem_root
+    Pathname.new(File.dirname ENV['BUNDLE_GEMFILE'])


Add parentheses to nested method call File.dirname ENV['BUNDLE_GEMFILE'].

houndci-bot · 2017-10-13T22:32:29Z

core/lib/spree/testing_support/dummy_app/rake_tasks.rb

+  require 'rails/commands/console/console_command'
+  Rails::Console.new(Rails.application, sandbox: true, environment: "test").start
+end
+


1 trailing blank lines detected.

houndci-bot · 2017-10-13T22:32:29Z

core/lib/spree/testing_support/dummy_app/auto_migrate.rb

+end
+
+Dummy::AutoMigrate.new.migrate
+


1 trailing blank lines detected.

houndci-bot · 2017-10-13T22:32:30Z

core/lib/spree/testing_support/dummy_app/auto_migrate.rb

+        sh 'rake db:reset VERBOSE=false'
+
+        # We might have a brand new database, so we must re-establish our connection
+        ActiveRecord::Base::establish_connection


Do not use :: for method calls.

houndci-bot · 2017-10-13T22:32:30Z

core/lib/spree/testing_support/dummy_app/auto_migrate.rb

+  class AutoMigrate
+    # Ensure database exists
+    def database_exists?
+      begin


Redundant begin block detected.

tvdeyen · 2017-10-24T07:52:33Z

I have some difficulties to build this locally.

First try in root of Solidus

Pulled fresh branch, ran bundle update and

tvd and ruby-2.4.2 in solidus/ on jhawthorn-dummy_app
› be rake spec

-----------
Solidus api
-----------
rake spec
/Users/tvd/.rbenv/versions/2.4.2/bin/ruby -I/Users/tvd/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rspec-core-3.6.0/lib:/Users/tvd/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rspec-support-3.6.0/lib /Users/tvd/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rspec-core-3.6.0/exe/rspec --pattern spec/\*\*\{,/\*/\*\*\}/\*_spec.rb
Re-running migrations
rake db:reset VERBOSE=false
Dropped database 'db/solidus_api_test.sqlite3'
Created database 'db/solidus_api_test.sqlite3'

[...]

Failures:

  1) Spree::Api::ReturnAuthorizationsController as the order owner cannot delete a return authorization
     Failure/Error: delete spree.api_order_return_authorization_path(order, 0)

     NoMethodError:
       undefined method `spree_path' for #<Module:0x00007f9ba9236288>
     # ./spec/requests/spree/api/return_authorizations_controller_spec.rb:57:in `block (3 levels) in <module:Spree>'

  [...]

Fails with hundreds of errors. All the same. Seems to be routes related.

Second try in `./api`

tvd and ruby-2.4.2 in api/ on jhawthorn-dummy_app
› be rake test_app
[Spree WARNING] Missing migrations.
[Spree WARNING] add_lft_and_rgt_indexes_to_taxons from spree is missing.
[Spree WARNING] solidus_one_four from spree is missing.
[Spree WARNING] Run `bundle exec rake railties:install:migrations` to get them.

Database 'db/solidus_api_test.sqlite3' does not exist
Created database 'db/solidus_api_test.sqlite3'

Prints warnings, but it builds successfully.

tvd and ruby-2.4.2 in api/ on jhawthorn-dummy_app
› beer

[Spree WARNING] Missing migrations.
[Spree WARNING] add_lft_and_rgt_indexes_to_taxons from spree is missing.
[Spree WARNING] solidus_one_four from spree is missing.
[Spree WARNING] Run `bundle exec rake railties:install:migrations` to get them.

[...]

Finished in 1 minute 56.92 seconds (files took 4.86 seconds to load)
579 examples, 0 failures, 3 pending

Randomized with seed 1494

The same with `./core`

Conclusion:

Builds successfully for individual tests suites, but fails from root
The warning about missing migrations seems to be wrong and we should find a way to avoid it

tvdeyen

This is awesome! Thanks for working on this John

I have some nits about changing behaviour in this PR.

tvdeyen · 2017-10-24T07:54:29Z

core/lib/spree/testing_support/dummy_app.rb

+    config.action_mailer.delivery_method              = :test
+    config.action_controller.allow_forgery_protection = false
+    config.active_support.deprecation                 = :stderr
+    config.eager_load                                 = false


duplicated line 44

tvdeyen · 2017-10-24T07:55:09Z

core/lib/spree/testing_support/dummy_app.rb

+  class Application < ::Rails::Application
+    config.root                                       = Dummy.rails_root
+    config.eager_load                                 = Rails.env.production?
+    config.cache_classes                              = !Rails.env.development?


Other settings have hard coded bools, while these doesn't. I think it's fine that we hard code all booleans. This dummy app is not meant to be run in other Rails env, isn't it?

tvdeyen · 2017-10-24T07:58:13Z

core/lib/spree/testing_support/dummy_app.rb

+  Spree::Deprecation.behavior = :raise
+end
+
+Spree.user_class = 'Spree::LegacyUser'


Should we consider to move all Spree config into a file (../dummy_app/spree_defaults.rb)?

tvdeyen · 2017-10-24T07:59:50Z

core/lib/spree/testing_support/dummy_app/auto_migrate.rb

+  else
+    []
+  end
+end


This seems to be nearly the same as in ../dummy_app.rb:61-67. Can we get rid of this duplication?

tvdeyen · 2017-10-24T08:13:41Z

frontend/spec/controllers/spree/home_controller_spec.rb

-        expect(response).to render_template(layout: 'layouts/application')
+        expect {
+          get :index
+        }.to raise_error(ActionView::MissingTemplate, %r{Missing template layouts/my_super_awesome_layout})


I think we should not change the spec like this. Is there a way to tell the dummy app the view paths like you did with the assets?

I do, but can send a separate PR

tvdeyen · 2017-10-24T08:16:31Z

frontend/app/controllers/spree/orders_controller.rb

@@ -1,7 +1,6 @@
 module Spree
  class OrdersController < Spree::StoreController
    before_action :check_authorization
-    rescue_from ActiveRecord::RecordNotFound, with: :render_404


I think it is very common in stores to overwrite render_404 and do stuff instead of rendering the 404 page. Building a dynamic 404 page for instance. Is there a way to still provide this functionality for stores? Could you tell the dummy app where to find the views like you did with the assets paths for instance?

I think this should be removed anyways because it's a bad way to override the 404 behaviour.

Stores could regain this functionality by either:

Using config.exceptions_app, the proper way to have a dynamic 404 page

Setting up the rescue_from from ApplicationController

Class-evaling (or sending) the rescue_from (if overrides are being done anyways)

But I can submit this as a separate PR if it will get this merged

Right. I can't remove this because the render_404 behaviour is very unusual.

render status: :not_found, file: "#{::Rails.root}/public/404", formats: [:html], layout: nil

This doesn't use views, it renders the public/404.html file relative to the Rails.root directory. There's really no way to get around this, it's bad code.

TIL config.exceptions_app 🎉

You are right, the code is bad and should be removed. Although it's blocking your PR I think we need a separate PR due to visibility reasons. I think we can forego with deprecating this, though.

tvdeyen · 2017-10-24T08:17:51Z

frontend/app/controllers/spree/products_controller.rb

@@ -3,7 +3,6 @@ class ProductsController < Spree::StoreController
    before_action :load_product, only: :show
    before_action :load_taxon, only: :index

-    rescue_from ActiveRecord::RecordNotFound, with: :render_404


kennyadsl

Thanks @jhawthorn, looking forward to run specs without running the test_app task!

Should we add a line to the changelog?

Also, I agree with most of the @tvdeyen notes despite I don't have a strong opinion on render_404 issue.

cbrunsdon

Other than the api migrations being handled elsewhere, I am good with the rest.

cbrunsdon · 2017-10-24T17:46:16Z

api/db/migrate/20100107141738_add_api_key_to_spree_users.rb

@@ -1,7 +0,0 @@
-class AddApiKeyToSpreeUsers < ActiveRecord::Migration[4.2]


I think we should deal with these migrations in a separate PR. The weirdness of them came up when I did the roll-up but I didn't want to change any behaviour.

Removed this change.

Replaced the db:migrate task with a custom migration task which runs migrations one engine at a time (which simulates running railties:install:migrations before db:migrate).

houndci-bot

Some files could not be reviewed due to errors:

.rubocop.yml: Style/FileName has the wrong namespace - should be Naming

.rubocop.yml: Style/FileName has the wrong namespace - should be Naming
.rubocop.yml: Style/PredicateName has the wrong namespace - should be Naming
.rubocop.yml: Style/AccessorMethodName has the wrong namespace - should be Naming
.rubocop.yml: Style/VariableNumber has the wrong namespace - should be Naming
Error: The `Style/OpMethod` cop has been renamed and moved to `Naming/BinaryOperatorParameterName`.
(obsolete configuration found in .rubocop.yml, please update it)

jhawthorn · 2017-10-24T23:56:49Z

I've addressed most of the feedback.

Builds successfully for individual tests suites, but fails from root

This should be fixed. We were incorrectly running specs using the top-level Gemfile instead of the individual project Gemfiles. It now runs bundle exec from a clean environment under each project.

The warning about missing migrations seems to be wrong and we should find a way to avoid it

This is because Spree::Migrations is detecting the old dummy_app migrations (it assumes the migrations paths instead of checking the app's config.paths as it should). rm -Rf spec/dummy or rake clean at the top-level will fix it. We could detect if we're running inside DummyApp from Spree::Migrations, but I'm not sure that's worth doing.

I was able to re-add the application/layout, but can't find a reasonable alternative to render_404.

tvdeyen

The render_404 change will need a separate PR, but other than that a huge improvement I really like. All concerns addressed 👍

Thanks again John 🐈

tvdeyen · 2017-10-25T07:13:20Z

frontend/app/controllers/spree/orders_controller.rb

@@ -1,7 +1,6 @@
 module Spree
  class OrdersController < Spree::StoreController
    before_action :check_authorization
-    rescue_from ActiveRecord::RecordNotFound, with: :render_404


TIL config.exceptions_app 🎉

You are right, the code is bad and should be removed. Although it's blocking your PR I think we need a separate PR due to visibility reasons. I think we can forego with deprecating this, though.

This removes the need to run rake test_app. Instead, we have a checked in single-file Rails application and can re-run migrations when necessary.

This allows running db:reset db:drop and db:create from the top-level solidus directory. This also renames `rake test` to `rake spec` and `rake test_app` to `rake db:reset`, with aliases added for the previous names.

We never want the user to run this directly, instead an extension will have an alias, so this shouldn't have a description to avoid including it in `rake -T`

This hasn't been required here for a long time

We don't need to require rake from a Rakefile, if we have run the Rakefile we can assume that has been done by rake. We don't need thor/group anymore. It was required ages ago when sandbox generation was a part of the top-level Rakefile.

This fixes issues running the new dummy_app based rake tasks at the top level.

It doesn't make sense to put these under "vendor". We aren't maintaining a standard rails structure for this app.

This matches the filename better, and makes it seem less strange to have modules under this namespace (like DummyApp::Migrations)

It's cleaner to call a method (DummyApp::Migrations.auto_migrate) than to expect side effects from requiring that file.

jhawthorn added the WIP label Oct 12, 2017

houndci-bot reviewed Oct 12, 2017

View reviewed changes

jhawthorn force-pushed the dummy_app branch from d41fba4 to c259fc1 Compare October 13, 2017 22:32

houndci-bot reviewed Oct 13, 2017

View reviewed changes

jhawthorn force-pushed the dummy_app branch 2 times, most recently from 7791b36 to 06df2b6 Compare October 16, 2017 23:27

jhawthorn changed the title ~~[WIP] Replace generated dummy test_app with single file rails application~~ Replace generated dummy test_app with single file rails application Oct 16, 2017

jhawthorn added Code review needed changelog:solidus_core Changes to the solidus_core gem and removed WIP labels Oct 16, 2017

jhawthorn force-pushed the dummy_app branch 3 times, most recently from 2dedc3c to ecbbc58 Compare October 17, 2017 22:46

tvdeyen reviewed Oct 24, 2017

View reviewed changes

kennyadsl reviewed Oct 24, 2017

View reviewed changes

cbrunsdon suggested changes Oct 24, 2017

View reviewed changes

jhawthorn force-pushed the dummy_app branch from 1e38ccd to a45ac92 Compare October 24, 2017 23:01

houndci-bot reviewed Oct 24, 2017

View reviewed changes

jhawthorn force-pushed the dummy_app branch from a45ac92 to 1663bd3 Compare October 24, 2017 23:31

tvdeyen approved these changes Oct 25, 2017

View reviewed changes

cbrunsdon approved these changes Oct 25, 2017

View reviewed changes

jhawthorn mentioned this pull request Oct 26, 2017

Remove render_404 from frontend #2329

Merged

jhawthorn added 6 commits October 27, 2017 12:06

Avoid running rake tasks from migrations

baeda12

Replace spec/dummy with checked in dummy_app

6f25aa5

This removes the need to run rake test_app. Instead, we have a checked in single-file Rails application and can re-run migrations when necessary.

Remove test_app from top-level build scripts

b050098

Remove mentions of test_app from READMEs

09cf638

Allow running all rake tasks from top level

da43ca7

This allows running db:reset db:drop and db:create from the top-level solidus directory. This also renames `rake test` to `rake spec` and `rake test_app` to `rake db:reset`, with aliases added for the previous names.

Remove description from common:test_app

679b37d

We never want the user to run this directly, instead an extension will have an alias, so this shouldn't have a description to avoid including it in `rake -T`

jhawthorn added 11 commits October 27, 2017 12:06

Remove common_rake from top-level Rakefile

43a67e2

This hasn't been required here for a long time

Use LIB_NAME as DB prefix

167b70e

Simplify dummy_app

ce0a625

Add Bundler.with_clean_env to Rakefile tasks

c98f473

This fixes issues running the new dummy_app based rake tasks at the top level.

Move asset paths in dummy_app

e3fb8df

It doesn't make sense to put these under "vendor". We aren't maintaining a standard rails structure for this app.

Move Dummy namespace to DummyApp

2f4b68c

This matches the filename better, and makes it seem less strange to have modules under this namespace (like DummyApp::Migrations)

Rename auto_migrate to DummyApp::Migrations

df0c5f7

It's cleaner to call a method (DummyApp::Migrations.auto_migrate) than to expect side effects from requiring that file.

Run dummy migrations one engine at a time

6367761

Add dummy application layout

5e072c4

Run bundle update from top-level rake if necessary

9efb37c

jhawthorn force-pushed the dummy_app branch from 1663bd3 to 9efb37c Compare October 27, 2017 19:07

jhawthorn merged commit 6add84f into solidusio:master Oct 27, 2017

jhawthorn mentioned this pull request Nov 24, 2017

Raise on deprecation warnings again #2402

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace generated dummy test_app with single file rails application #2281

Replace generated dummy test_app with single file rails application #2281

jhawthorn commented Oct 12, 2017 •

edited

Loading

houndci-bot Oct 12, 2017

houndci-bot Oct 12, 2017

houndci-bot Oct 12, 2017

houndci-bot Oct 12, 2017

houndci-bot Oct 13, 2017

houndci-bot Oct 13, 2017

houndci-bot Oct 13, 2017

houndci-bot Oct 13, 2017

tvdeyen commented Oct 24, 2017

tvdeyen left a comment

tvdeyen Oct 24, 2017

tvdeyen Oct 24, 2017

tvdeyen Oct 24, 2017

tvdeyen Oct 24, 2017

tvdeyen Oct 24, 2017

jhawthorn Oct 24, 2017

tvdeyen Oct 24, 2017

jhawthorn Oct 24, 2017

jhawthorn Oct 24, 2017 •

edited

Loading

tvdeyen Oct 25, 2017

tvdeyen Oct 24, 2017

kennyadsl left a comment

cbrunsdon left a comment

cbrunsdon Oct 24, 2017

jhawthorn Oct 24, 2017

houndci-bot left a comment

jhawthorn commented Oct 24, 2017

tvdeyen left a comment

tvdeyen Oct 25, 2017

		@@ -1,7 +0,0 @@
		class AddApiKeyToSpreeUsers < ActiveRecord::Migration[4.2]

		end

		Dummy::AutoMigrate.new.migrate

Replace generated dummy test_app with single file rails application #2281

Replace generated dummy test_app with single file rails application #2281

Conversation

jhawthorn commented Oct 12, 2017 • edited Loading

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

tvdeyen commented Oct 24, 2017

First try in root of Solidus

Fails with hundreds of errors. All the same. Seems to be routes related.

Second try in ./api

Prints warnings, but it builds successfully.

The same with ./core

Conclusion:

tvdeyen left a 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

jhawthorn Oct 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

cbrunsdon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

houndci-bot left a comment

Choose a reason for hiding this comment

jhawthorn commented Oct 24, 2017

tvdeyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhawthorn commented Oct 12, 2017 •

edited

Loading

Second try in `./api`

The same with `./core`

jhawthorn Oct 24, 2017 •

edited

Loading