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

Improve railties generators tests #30164

Merged

Conversation

bogdanvlviv
Copy link
Contributor

@bogdanvlviv bogdanvlviv commented Aug 9, 2017

Improve plugin generator tests
Improve app generator tests
Ensure that generation config/application.rb is correct.
Related to #30123


--skip-action-cable pass throughs rails plugin new (i found this when i was improving tests)


Also, this PR will prevent making mistakes like: #30101 (comment)
r? @rafaelfranca

@bogdanvlviv bogdanvlviv force-pushed the improve-railties-generators-tests branch 2 times, most recently from 1344b97 to a96b558 Compare August 9, 2017 16:31
Copy link
Member

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

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

You can squash the first and third commits and the second and the fourth commit

@@ -106,11 +106,28 @@ def test_generating_test_files_in_full_mode_without_unit_test_files
end
end

def test_generating_adds_dummy_app_in_full_mode_without_sprockets
run_generator [destination_root, "-S", "--full"]
def test_generator_if_skip_sprockets_is_given
Copy link
Member

Choose a reason for hiding this comment

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

I prefer the previous name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after remove --full option

- test_generating_adds_dummy_app_in_full_mode_without_sprockets
+ test_generating_adds_dummy_app_without_sprockets

def test_generating_adds_dummy_app_in_full_mode_without_sprockets
run_generator [destination_root, "-S", "--full"]
def test_generator_if_skip_sprockets_is_given
run_generator [destination_root, "--skip-sprockets"]
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep the --full option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this test hasn't had any relations to --full option, so this is extra here.

end
end

def test_requires_if_skip_railties
Copy link
Member

Choose a reason for hiding this comment

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

test_default_frameworks_are_required_when_others_are_removed

assert_no_file "test"
assert_no_file "test/test_helper.rb"
assert_directory "spec/dummy"
assert_file "spec/dummy/config/application.rb", /#\s+require\s+["']rails\/test_unit\/railtie["']/
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to check this if it is checked in other place.

Copy link
Contributor Author

@bogdanvlviv bogdanvlviv Aug 10, 2017

Choose a reason for hiding this comment

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

It isn't checked in another place, we need to check it.
A generation of plugin --skip-test has the more complicated case than a generation of app.

If rails plugin new name_plugin --skip-test and --dummy-path is test/dummy(default value the same), it woun't create a dummy app.
(we have condition for creation dummy app: options[:skip_test].blank? || options[:dummy_path] != "test/dummy"),
so it isn't possible to test # require "rails/test_unit/railtie" for existence in config/application.rb,

but we can reach it by rails plugin new name_plugin --skip-test --dummy-path=spec/dummy


def test_action_cable_is_removed_from_frameworks_if_skip_action_cable_is_given
run_generator [destination_root, "--skip-action-cable"]
assert_file "test/dummy/config/application.rb", /#\s+require\s+["']action_cable\/engine["']/
Copy link
Member

Choose a reason for hiding this comment

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

Does this test pass only with the first commit?

Copy link
Contributor Author

@bogdanvlviv bogdanvlviv Aug 10, 2017

Choose a reason for hiding this comment

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

No, the test fails.
Only
assert_file "test/dummy/config/application.rb", /#\s+require\s+["']action_cable\/engine["']/ passes, it looks strange

Copy link
Contributor Author

@bogdanvlviv bogdanvlviv Aug 10, 2017

Choose a reason for hiding this comment

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

run_generator [destination_root, "--skip-action-cable"]
assert_file "test/dummy/config/application.rb", /#\s+require\s+["']action_cable\/engine["']/

I think i got why it passes on master,

test/dummy/ is generated by Rails::Generators::AppGenerator with filtered options,

https://github.com/rails/rails/blob/master/railties/lib/rails/generators/rails/plugin/plugin_generator.rb#L88-L104

then we replace some files in test/dummy/ with all options
https://github.com/rails/rails/blob/master/railties/lib/rails/generators/rails/plugin/plugin_generator.rb#L106-L112

Next example doesn't pass on master, because these files are generated by Rails::Generators::AppGenerator with filtered options, https://github.com/rails/rails/blob/master/railties/lib/rails/generators/rails/plugin/plugin_generator.rb#L88-L104

run_generator [destination_root, "--skip-action-cable"]
assert_no_file "test/dummy/config/cable.yml"
assert_no_file "test/dummy/app/assets/javascripts/cable.js"
assert_no_directory "test/dummy/app/channels"
assert_file "Gemfile" do |content|
  assert_no_match(/redis/, content)
end

it only will pass if add :skip_action_cable to PASSTHROUGH_OPTIONS. If don't pass :skip_action_cable to PASSTHROUGH_OPTIONS , these files will be always generated because skip_action_cable is false by default https://github.com/rails/rails/blob/master/railties/lib/rails/generators/app_base.rb#L55

@@ -1,3 +1,7 @@
* Add `--skip-action-cable` option to the plugin generator.
Copy link
Member

Choose a reason for hiding this comment

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

Could you squash this commit with the commit that added the option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

Improve app generator tests.

Ensure that generation `config/application.rb` is correct.

Ensure that generation `config/application.rb` is correct.
@bogdanvlviv bogdanvlviv force-pushed the improve-railties-generators-tests branch from 86d2e50 to 8cc5a6e Compare August 10, 2017 06:30
@rafaelfranca rafaelfranca merged commit 3ffc77a into rails:master Aug 10, 2017
@rafaelfranca
Copy link
Member

Really good PR! 👏

@bogdanvlviv bogdanvlviv deleted the improve-railties-generators-tests branch August 11, 2017 07:46
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