[#184611345] Chore / Support for rails 6.1 #1
Conversation
masonlouchart
changed the title
|
@masonlouchart Sorry, do you get any errors when executing the above comments? I tried validating like suggested:
When trying to execute the tests it fails: |
|
Actually, I got some errors but different ones. The test suite can be run entirely without Rake failure. Please take a look at the file I've attached. These errors don't have to be fixed for our purpose but are a sign that we should either update our fork or stop using this gem. I will go ahead with the investigation. Hoping I can reproduce the error. |
|
It might be due to old ❯ gem list activesupport
*** LOCAL GEMS ***
activesupport (6.1.7.3, 6.1.0, 6.0.6.1, 6.0.5.1) |
|
I fixed some warnings and configurations. But 2 errors remaining. I have no idea why it is related to missing files not being generated. As far as I understand, running the test is supposed to ask Rails to generate them. I am not sure it is worth it to invest more time in this 2 errors. Please take a look and let me know what you think 🧐 Here is the output.txt Could you please reinstall this project and re-run the test suite? |
|
I could reproduce the output. Sorry, please let me consider a bit :) |
There was a problem hiding this comment.
Thank you for dealing with the old library :) Please take a look at the comments.
I fixed some warnings and configurations. But 2 errors remaining. I have no idea why it is related to missing files not being generated. As far as I understand, running the test is supposed to ask Rails to generate them. I am not sure it is worth it to invest more time in this 2 errors. Please take a look and let me know what you think 🧐
Here is the output.txt
The output file you shared has 4 errors, not 2 errors? And I can also reproduce the same 4 errors locally.
10 runs, 18 assertions, 4 failures, 0 errors, 0 skips
Out of those 4, wouldn't two be very important to the functionality of the library? These two:
AssetsTest#test_typescript.js_is_included_in_Sprockets_environment [/Users/mason/workspace/NEWROPE/typescript-rails/test/assets_test.rb:40]:
assert { assets["typescript"].source.include?('var ts;') }
rails test test/assets_test.rb:38
and
AssetsTest#test_assets_.js.ts_is_compiled_from_TypeScript_to_JavaScript [/Users/mason/workspace/NEWROPE/typescript-rails/test/assets_test.rb:44]:
assert { assets['javascripts/hello.js'].present? }
rails test test/assets_test.rb:43
We use the library to generate assets from TypeScript via Sprockets, so it would be important to ensure that part still works.
About the other errors related to "controller generator" and "scaffold generator": I agree they aren't so important, because they only affect developers.
| @@ -31,25 +31,25 @@ def source | |||
|
|
|||
| test 'typescript views are served as javascript' do | |||
| get '/site/index.js' | |||
| assert_match /var x = 5;\s*/, | |||
| assert_match (/var x = 5;\s*/), | |||
There was a problem hiding this comment.
What effect do the brackets have? I didn't see any difference after removing them when running tests.
There was a problem hiding this comment.
It removes warnings.
❯ bundle exec rake test --trace
** Invoke test (first_time)
** Execute test
/Users/mason/workspace/NEWROPE/typescript-rails/test/template_handler_test.rb:34: warning: ambiguous first argument; put parentheses or a space even after `/' operator
/Users/mason/workspace/NEWROPE/typescript-rails/test/template_handler_test.rb:40: warning: ambiguous first argument; put parentheses or a space even after `/' operator
/Users/mason/workspace/NEWROPE/typescript-rails/test/template_handler_test.rb:46: warning: ambiguous first argument; put parentheses or a space even after `/' operator
/Users/mason/workspace/NEWROPE/typescript-rails/test/template_handler_test.rb:52: warning: ambiguous first argument; put parentheses or a space even after `/' operator
Run options: --seed 34434
Gemfile
Outdated
| @@ -4,7 +4,7 @@ source 'https://rubygems.org' | |||
| gemspec | |||
|
|
|||
| group :test do | |||
| gem 'rails', '~> 4.0' | |||
| gem 'rails', '>= 4.0', '< 7.0' | |||
There was a problem hiding this comment.
Since we need to use Rails 7 don't we need to test this using Rails 7? https://newrope.slack.com/archives/C02S08WDQ/p1681097711254339?thread_ts=1680764615.995429&cid=C02S08WDQ
There was a problem hiding this comment.
Yes, I thought about it last night before bed. I wanted to make sure we don't use Rails 7 yet for debugging this old dependency but actually might not be a wise decision. I will remove the lock.
|
You will notice that running tests actually fail. ❯ bundle exec rake test --trace
** Invoke test (first_time)
** Execute test
Run options: --seed 22052
# Running:
/Users/mason/.rvm/gems/ruby-2.7.2/gems/typescript-src-1.6.2.0/lib/typescript-src.rb:12: warning: instance variable @typescript_path not initialized
.E
Error:
AssetsTest#test_typescript.js_is_included_in_Sprockets_environment:
FrozenError: can't modify frozen Array: []
/Users/mason/.rvm/gems/ruby-2.7.2/gems/railties-7.0.4.3/lib/rails/engine.rb:574:in `unshift'
/Users/mason/.rvm/gems/ruby-2.7.2/gems/railties-7.0.4.3/lib/rails/engine.rb:574:in `block in <class:Engine>'
/Users/mason/.rvm/gems/ruby-2.7.2/gems/railties-7.0.4.3/lib/rails/initializable.rb:32:in `instance_exec'
/Users/mason/.rvm/gems/ruby-2.7.2/gems/railties-7.0.4.3/lib/rails/initializable.rb:32:in `run'
/Users/mason/.rvm/gems/ruby-2.7.2/gems/railties-7.0.4.3/lib/rails/initializable.rb:61:in `block in run_initializers'
/Users/mason/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/tsort.rb:228:in `block in tsort_each'
/Users/mason/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/tsort.rb:350:in `block (2 levels) in each_strongly_connected_component'
/Users/mason/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/tsort.rb:422:in `block (2 levels) in each_strongly_connected_component_from'
/Users/mason/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/tsort.rb:422:in `block (2 levels) in each_strongly_connected_component_from'
/Users/mason/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/tsort.rb:431:in `each_strongly_connected_component_from'
/Users/mason/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/tsort.rb:421:in `block in each_strongly_connected_component_from'
/Users/mason/.rvm/gems/ruby-2.7.2/gems/railties-7.0.4.3/lib/rails/initializable.rb:50:in `each'
/Users/mason/.rvm/gems/ruby-2.7.2/gems/railties-7.0.4.3/lib/rails/initializable.rb:50:in `tsort_each_child'
/Users/mason/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/tsort.rb:415:in `call'
/Users/mason/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/tsort.rb:415:in `each_strongly_connected_component_from'
/Users/mason/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/tsort.rb:421:in `block in each_strongly_connected_component_from'
/Users/mason/.rvm/gems/ruby-2.7.2/gems/railties-7.0.4.3/lib/rails/initializable.rb:50:in `each'
/Users/mason/.rvm/gems/ruby-2.7.2/gems/railties-7.0.4.3/lib/rails/initializable.rb:50:in `tsort_each_child'
/Users/mason/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/tsort.rb:415:in `call'
/Users/mason/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/tsort.rb:415:in `each_strongly_connected_component_from'
/Users/mason/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/tsort.rb:349:in `block in each_strongly_connected_component'
/Users/mason/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/tsort.rb:347:in `each'
/Users/mason/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/tsort.rb:347:in `call'
/Users/mason/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/tsort.rb:347:in `each_strongly_connected_component'
/Users/mason/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/tsort.rb:226:in `tsort_each'
/Users/mason/.rvm/rubies/ruby-2.7.2/lib/ruby/2.7.0/tsort.rb:205:in `tsort_each'
/Users/mason/.rvm/gems/ruby-2.7.2/gems/railties-7.0.4.3/lib/rails/initializable.rb:60:in `run_initializers'
/Users/mason/.rvm/gems/ruby-2.7.2/gems/railties-7.0.4.3/lib/rails/application.rb:372:in `initialize!'
/Users/mason/.rvm/gems/ruby-2.7.2/gems/railties-7.0.4.3/lib/rails/railtie.rb:226:in `public_send'
/Users/mason/.rvm/gems/ruby-2.7.2/gems/railties-7.0.4.3/lib/rails/railtie.rb:226:in `method_missing'
/Users/mason/workspace/NEWROPE/typescript-rails/test/assets_test.rb:23:in `setup'However, if you run the for Sprocket commenting the other for TypeScript and do the opposite too, it works. We just cannot run all tests at once... I suspect Sprocket but could not find anything. |
|
This PR is ready to be reviewed. Please give it a try and let me know if you encounter different situation. |
I debugged this a bit. If you look at the stack trace:
You can see that it is raised by initializing Rails. I added print debugging to diff --git a/test/assets_test.rb b/test/assets_test.rb
index 95c56a5..e551230 100644
--- a/test/assets_test.rb
+++ b/test/assets_test.rb
@@ -35,12 +35,13 @@ class AssetsTest < ActiveSupport::TestCase
@app.assets
end
- test 'typescript.js is included in Sprockets environment' do
+ # Run all tests in one block to avoid error from calling @app.initialize! multiple times
+ test 'generates assets successfully' do
+ # typescript.js is included in Sprockets environment
assert { assets['typescript'].filename.to_s.end_with?('/lib/assets/javascripts/typescript.js.erb') }
assert { assets['typescript'].source.include?('var ts;') }
- end
- test 'assets .js.ts is compiled from TypeScript to JavaScript' do
+ # 'assets .js.ts is compiled from TypeScript to JavaScript'
assert { assets['javascripts/hello.js'].present? }
assert { assets['javascripts/hello.js'].source.include?('var log_to_console = function (x) {') }
assert { assets['javascripts/hello.js'].source.include?('var s = "Hello, world!";') }But after this the tests still failed... I suspect something about sprockets v4 breaking changes, because when I tried using sprockets v3 the tests passed: diff --git a/Gemfile b/Gemfile
index 12d4128..7506172 100644
--- a/Gemfile
+++ b/Gemfile
@@ -6,9 +6,9 @@ gemspec
group :test do
gem 'rails', '>= 4.0'
gem 'sprockets-rails', '> 2.0'
+ gem 'sprockets', '~> 3'
gem 'minitest-power_assert'
gem 'coveralls'
gem 'simplecov'
gem 'tzinfo-data', platforms: [:mingw, :mswin]
end
-CBK currently uses sprockets v3 though - if we don't need to upgrade to v4 we can keep v3 in this gem? By the way, should we commit |
I didn't expect an internal issue from Rails because of Sprocket...
I thought we would have to use v4 with Rails 7 but it does not look incompatible after checking again.
Definitely. I forgot about this point. |
TBH I didn't check this too deeply. But we can deal with it later if the need becomes evident. |
|
Interesting... Recently the best practice about whether to commit |
Thank you for sharing that. I think for development it's convenient (=> easy to reproduce results consistently when collaborating). If we want to ensure support for ranges of versions, just in my opinion: it's difficult to manually keep up -> CI should deal with that. |
@iusztin @NEWROPE/package
Pivotal Story
Changes
How to reproduce
masterbranchbundlebundle exec rake testHow to validate
chore/support-for-rails-6.1bundle exec rake testPlease review.