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

Declare test dependencies explicitly in gemspecs #2359

Closed

Conversation

jhawthorn
Copy link
Contributor

Build on #2358, which is split out for visibility.

This moves our spec dependencies out of common_spree_dependencies.rb and into the individual gemspecs for each project. This adds explicitness (good) at the expense of repetition (bad) and makes the
spec suites faster (good).

This isn't necessary when running through the `rspec` command, but is
necessary when running files manually (ruby -Ispec spec/foo_spec.rb).
Previously we lumped all spec-related dependencies in the same place,
but each project has slightly different requirements.

This moves the declarations from a top-level Gemfile (well, file
required from Gemfile) to individual projects' gemspecs. This adds
explicitness (good) at the expense of repetition (bad) and makes the
spec suites faster (good).
We aren't going to be running in dev mode from within the Solidus
project itself, so we don't need listen for code reloading.
It will be required when running the rubocop executable.
@jhawthorn jhawthorn force-pushed the remove_common_dependencies branch from d1bcd10 to a93c9ae Compare November 7, 2017 23:24
@jhawthorn jhawthorn added the changelog:solidus_core Changes to the solidus_core gem label Nov 9, 2017
@ccarruitero
Copy link
Contributor

I'm agree to move testing dependencies from common_spree_dependencies, but in order to reduce the repetition I think that we could introduce new files for wrap common testing dependencies.

I was thinking in one file like common_testing_dependencies where add testing gems that are repeated in all solidus gems, excluding sample. I think will be mainly the gems added in core without timecop and with_model.

An another like common_integration_testing_dependencies where add common integration gems. Here I think will be only capybara and capybara-screenshot for now.

@@ -1 +1,3 @@
Rails.application.config.assets.precompile << 'solidus_core_manifest.js'
if Rails.application.config.respond_to?(:assets)
Copy link
Contributor

@ccarruitero ccarruitero Nov 12, 2017

Choose a reason for hiding this comment

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

I think we should remove this changes since are in #2358

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is built on top of #2358, which is necessary for this to pass.

@@ -73,8 +73,10 @@ class Application < ::Rails::Application

config.action_controller.include_all_helpers = false

config.assets.paths << File.expand_path('../dummy_app/assets/javascripts', __FILE__)
config.assets.paths << File.expand_path('../dummy_app/assets/stylesheets', __FILE__)
if config.respond_to?(:assets)
Copy link
Contributor

@ccarruitero ccarruitero Nov 12, 2017

Choose a reason for hiding this comment

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

same as comment above

Copy link
Member

@gmacdougall gmacdougall left a comment

Choose a reason for hiding this comment

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

I'm good with these changes as is.

I don't see a large amount of value in extracting things to common sets of dependencies. This makes it clear what each gem requires to build itself in development.

👍

@jhawthorn
Copy link
Contributor Author

Closing in favour of #2407

@jhawthorn jhawthorn closed this Dec 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_core Changes to the solidus_core gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants