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

Added test cases for Action Text install generator #39317

Conversation

abhaynikam
Copy link
Contributor

@abhaynikam abhaynikam commented May 17, 2020

Previously I encountered some issues with the Action Text installer. Reference: #39128 and #37823. So I thought we should have a test cases for the generator.

@rails-bot rails-bot bot added the actiontext label May 17, 2020
destination File.expand_path("../../tmp", __dir__)
setup :prepare_destination
tests ActionText::Generators::InstallGenerator

def destination_root
tmp_path "foo_bar"
end

def tmp_path(*args)
@tmp_path ||= File.expand_path("../../tmp", __dir__)
File.join(@tmp_path, *args)
end

def sample_app_path
tmp_path("foo_bar")
end

def rails(cmd)
`#{Gem.ruby} #{RAILS_FRAMEWORK_ROOT}/railties/exe/rails #{cmd}`
end

def bundled_rails(cmd)
`bundle exec rails #{cmd}`
end

def generate_sample_app
FileUtils.rm_rf(sample_app_path)
FileUtils.mkdir_p(sample_app_path)

quietly {
rails("new #{sample_app_path} --skip-action-mailer --skip-action-cable --skip-sprockets --skip_bootsnap --skip_webpack_install --quiet")
}
end
Copy link
Contributor

@kaspth kaspth May 17, 2020

Choose a reason for hiding this comment

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

How much of this is copied from other generator tests? Seems like there's quite a bit of setup here.

Copy link
Contributor Author

@abhaynikam abhaynikam May 17, 2020

Choose a reason for hiding this comment

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

Almost everything in the setup from this generator: https://github.com/rails/rails/blob/master/railties/test/railties/generators_test.rb

Tried to require the isolations/abstract_unit and use generators_test_helper but faced a lot of issues. That was 4 days ago so can't recall exactly what the errors was but I can try it again and send it here.

Copy link
Contributor

@kaspth kaspth May 17, 2020

Choose a reason for hiding this comment

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

Yeah, we can’t duplicate the entire setup 👍

Copy link
Contributor Author

@abhaynikam abhaynikam Aug 9, 2020

Choose a reason for hiding this comment

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

@kaspth Removed the duplication and moved the test cases to railties. Could you please review the changes?

@abhaynikam abhaynikam force-pushed the add-test-case-for-action-text-install branch from acf280b to ae60230 Compare May 17, 2020
@abhaynikam abhaynikam force-pushed the add-test-case-for-action-text-install branch from ae60230 to 7e4740e Compare Aug 9, 2020
@rails-bot rails-bot bot added the railties label Aug 9, 2020
@abhaynikam abhaynikam force-pushed the add-test-case-for-action-text-install branch from 7e4740e to e851cc3 Compare Aug 9, 2020
@abhaynikam
Copy link
Contributor Author

abhaynikam commented Sep 4, 2020

Ping. If this test adds value also review: #40069. Thanks in adv. 😊

Copy link
Member

@jonathanhefner jonathanhefner left a comment

Thanks for looking into this! I will make another pass after the switch to run_generator. It will probably alter the structure a bit.

js_dependencies_stub = { "trix"=>"latest", "@rails/actiontext"=>"latest" }
generator.stub(:js_dependencies, js_dependencies_stub) do
generator.stub(:rails_command, nil, ["app:binstub:yarn"]) do
quietly { generator.install_javascript_dependencies }
Copy link
Member

@jonathanhefner jonathanhefner Sep 7, 2020

Choose a reason for hiding this comment

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

I think we normally tests generators as a whole via run_generator, rather than individual generator actions / methods.

Copy link
Contributor Author

@abhaynikam abhaynikam Sep 8, 2020

Choose a reason for hiding this comment

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

@jonathanhefner When I use run_generator here I run into following error:

Traceback (most recent call last):
	3: from /Users/abhaynikam/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0/gems/minitest-5.14.0/lib/minitest/parallel.rb:33:in `block (2 levels) in start'
	2: from /Users/abhaynikam/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0/gems/minitest-5.14.0/lib/minitest.rb:1026:in `run_one_method'
	1: from /Users/abhaynikam/personal/rails/activesupport/lib/active_support/testing/isolation.rb:23:in `run'
/Users/abhaynikam/personal/rails/activesupport/lib/active_support/testing/isolation.rb:23:in `load': marshal data too short (ArgumentError)
Traceback (most recent call last):
	3: from /Users/abhaynikam/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0/gems/minitest-5.14.0/lib/minitest/parallel.rb:33:in `block (2 levels) in start'
	2: from /Users/abhaynikam/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0/gems/minitest-5.14.0/lib/minitest.rb:1026:in `run_one_method'
	1: from /Users/abhaynikam/personal/rails/activesupport/lib/active_support/testing/isolation.rb:23:in `run'
/Users/abhaynikam/personal/rails/activesupport/lib/active_support/testing/isolation.rb:23:in `load': marshal data too short (ArgumentError)

I have seen at a lot of places(app_generator_test) in the codebase using either

  1. generator.invoke_all
  2. generator.method_name
  3. run_generator

I preferred to use generator.method_name to avoid running yarn add multiple times(stubbed) and slowing the test suite. Will it be okay if we go with generator.method_name? If not, I'll try to invest more time in it. 😊

Copy link
Contributor Author

@abhaynikam abhaynikam Nov 20, 2020

Choose a reason for hiding this comment

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

@jonathanhefner Using run_generator throws:

lib/active_support/testing/isolation.rb:23:in `load': marshal data too short (ArgumentError)

Any thoughts on how we could resolve it? I spent too much time on it but not able to fix it and use run_generator instead of generator.method_name.

assert_file "package.json" do |content|
assert_match(/@rails\/actiontext/, content)
assert_match(/trix/, content)
end
Copy link
Member

@jonathanhefner jonathanhefner Sep 7, 2020

Choose a reason for hiding this comment

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

This essentially tests that bin/yard add works. I think it would be better to test that bin/yarn add is properly called (and either assume it works, or add more targeted tests elsewhere).

FileUtils.rm_rf("app/javascript/packs/application.js")

expected = "WARNING: Action Text can't locate your JavaScript bundle to add its package dependencies."
generator.append_dependencies_to_package_file
$stdout.rewind
assert_match expected, $stdout.read
Copy link
Member

@jonathanhefner jonathanhefner Sep 7, 2020

Choose a reason for hiding this comment

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

This might be better as a separate test case.

@abhaynikam abhaynikam force-pushed the add-test-case-for-action-text-install branch from e851cc3 to 7973013 Compare Nov 19, 2020
@abhaynikam abhaynikam force-pushed the add-test-case-for-action-text-install branch from 7973013 to 091bf15 Compare Nov 20, 2020
@abhaynikam
Copy link
Contributor Author

abhaynikam commented Nov 20, 2020

Resolved the other two comments. Let me know if any feedback on this PR. 😊

jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Nov 23, 2020
This commit adds tests for the Action Text install generator.  It also
includes a few changes in and around `generators_test_helper.rb` to make
writing similar tests easier in the future.

Closes rails#39317.

Co-authored-by: Abhay Nikam <nikam.abhay1@gmail.com>
jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Nov 23, 2020
This commit adds tests for the Action Text install generator.  It also
includes a few changes in and around `generators_test_helper.rb` to make
writing similar tests easier in the future.

Closes rails#39317.

Co-authored-by: Abhay Nikam <nikam.abhay1@gmail.com>
@kaspth kaspth closed this in a6b503c Nov 27, 2020
@kaspth
Copy link
Contributor

kaspth commented Nov 27, 2020

Fixed in #40673

brendon pushed a commit to brendon/rails that referenced this pull request Jun 17, 2021
This commit adds tests for the Action Text install generator.  It also
includes a few changes in and around `generators_test_helper.rb` to make
writing similar tests easier in the future.

Closes rails#39317.

Co-authored-by: Abhay Nikam <nikam.abhay1@gmail.com>
wbotelhos pushed a commit to wbotelhos/rails that referenced this pull request Jul 18, 2021
This commit adds tests for the Action Text install generator.  It also
includes a few changes in and around `generators_test_helper.rb` to make
writing similar tests easier in the future.

Closes rails#39317.

Co-authored-by: Abhay Nikam <nikam.abhay1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants