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

Add --skip-yarn option to the plugin generator #30238

Merged

Conversation

bogdanvlviv
Copy link
Contributor

No description provided.

@rails-bot
Copy link

r? @sgrif

(@rails-bot has picked a reviewer for you, use r? to override)

@@ -148,6 +148,32 @@ def test_generating_adds_dummy_app_without_sprockets
end
end

def test_generator_for_yarn
Copy link
Member

Choose a reason for hiding this comment

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

Rather than adding a test to PluginGeneratorTest, how about moving the test at AppGeneratorTest to SharedGeneratorTests?

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test test_generator_for_yarn in railties/test/generators/app_generator_test.rb different from test_generator_for_yarn in railties/test/generators/plugin_generator_test.rb

Expample:

assert_file "package.json", /dependencies/ #  railties/test/generators/app_generator_test.rb
assert_file "test/dummy/package.json", /dependencies/ # railties/test/generators/plugin_generator_test.rb

Copy link
Member

Choose a reason for hiding this comment

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

How about letting specify the application path from each test? As described below.

diff --git a/railties/test/generators/app_generator_test.rb b/railties/test/generators/app_generator_test.rb
index ccb437c..cb46000 100644
--- a/railties/test/generators/app_generator_test.rb
+++ b/railties/test/generators/app_generator_test.rb
+
+    def application_path
+      destination_root
+    end
 end
diff --git a/railties/test/generators/plugin_generator_test.rb b/railties/test/generators/plugin_generator_test.rb
index bd76af8..f55b89a 100644
--- a/railties/test/generators/plugin_generator_test.rb
+++ b/railties/test/generators/plugin_generator_test.rb
@@ -150,6 +150,18 @@ def test_generating_adds_dummy_app_without_sprockets
     end
   end
 
+  private
+    def application_path
+      "#{destination_root}/test/dummy"
+    end
 end
diff --git a/railties/test/generators/shared_generator_tests.rb b/railties/test/generators/shared_generator_tests.rb
index c73b91e..d584fbb 100644
--- a/railties/test/generators/shared_generator_tests.rb
+++ b/railties/test/generators/shared_generator_tests.rb
@@ -114,6 +114,13 @@ def test_skip_git
     assert_no_directory(".git")
   end
 
+  def test_generator_for_yarn
+    run_generator([destination_root])
+
+    assert_file "#{application_path}/package.json", /dependencies/
+    assert_file "#{application_path}/config/initializers/assets.rb", /node_modules/
+  end

Copy link
Contributor Author

@bogdanvlviv bogdanvlviv Aug 15, 2017

Choose a reason for hiding this comment

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

@y-yagi I've just implemented it.
Itl definitely will help me to do more dryer tests in this PR: #30101.
Also, I've have prepared new changes to dry tests in railties/test/generators/app_generator_test.rb and railties/test/generators/plugin_generator_test.rb.
When it merges, I will open new PR with this changes.

Thank you!

@@ -148,6 +148,32 @@ def test_generating_adds_dummy_app_without_sprockets
end
end

def test_generator_for_yarn
Copy link
Member

Choose a reason for hiding this comment

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

👍

@bogdanvlviv bogdanvlviv force-pushed the add-skip_yarn-for-plugin_generator branch from 8d2c178 to 3a3f4dd Compare August 15, 2017 16:07
@rafaelfranca
Copy link
Member

Could you squash your commits?

Add SharedGeneratorTests#application_path
This method will help to DRY in files app_generator_test.rb, plugin_generator_test.rb
@bogdanvlviv bogdanvlviv force-pushed the add-skip_yarn-for-plugin_generator branch from 3a3f4dd to 1a1f319 Compare August 15, 2017 19:36
@bogdanvlviv
Copy link
Contributor Author

Done!

@rafaelfranca rafaelfranca merged commit 692fab2 into rails:master Aug 15, 2017
@bogdanvlviv bogdanvlviv deleted the add-skip_yarn-for-plugin_generator branch August 15, 2017 20:52
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.

5 participants