Make Rails app detection based on Rails::Application superclass #2218

louim · 2024-06-21T06:35:55Z

Motivation

closes #2096

This change makes the Rails app detection based on the superclass of the main class being Rails::Application. This is a more reliable way to detect Rails apps than looking for the presence of the rails gem in the Gemfile. Application can require some of the Rails components without requiring the rails gem itself.

Implementation

The implementation is strongly inspired from #2161, hat tip to @Earlopain. I've added you as a co-author; let me know if that's ok.

Automated Tests

The test all pass individually. However, when the full file is executed, one of the test doesn't pass. It's most certainly related to something I have changed. I wasn't able to pinpoint if context is leaking between tests or if something else is happening. Since this is also my first time using minitest, an experienced eye would be appreciated.

The problem can be reproduced with:

SPEC_REPORTER=1 bundle exec rake TEST="test/setup_bundler_test.rb" TESTOPTS="--seed=55457"

Relevant snip in case it doesn't fail:

  test_ruby_lsp_rails_is_automatically_included_in_rails_apps     PASS (1.39s)
  test_creates_custom_bundle_for_a_rails_app                      FAIL (0.00s)
        Expected path '.ruby-lsp/Gemfile.lock' to exist.
        /Users/louim/repositories/ruby-lsp/test/setup_bundler_test.rb:97:in `block (2 levels) in test_creates_custom_bundle_for_a_rails_app'
        /Users/louim/repositories/ruby-lsp/test/setup_bundler_test.rb:79:in `chdir'
        /Users/louim/repositories/ruby-lsp/test/setup_bundler_test.rb:79:in `block in test_creates_custom_bundle_for_a_rails_app'
        /Users/louim/.local/share/mise/installs/ruby/3.3.1/lib/ruby/3.3.0/tmpdir.rb:99:in `mktmpdir'
        /Users/louim/repositories/ruby-lsp/test/setup_bundler_test.rb:78:in `test_creates_custom_bundle_for_a_rails_app'

Manual Tests

I haven't yet tested in our app, will do so as soon as possible.

Earlopain · 2024-06-21T08:36:49Z

This looks nice, thanks for picking this up! I've closed my PR since it missed the mark somewhat.

I tried giving this a shot but testing this out proved a bit more complicated. One issue I'm certain of is that a require "prism" is missing:

/home/user/.rbenv/versions/3.3.1/lib/ruby/gems/3.3.0/bundler/gems/ruby-lsp-17a19de139cd/lib/ruby_lsp/setup_bundler.rb:292:in `rails_app?': uninitialized constant RubyLsp::SetupBundler::Prism (NameError)

result = Prism.parse(config)

Can't use rubyLsp.branch since this is a fork and when adding this to the gemfile with gem "ruby-lsp", github: "https://github.com/Shopify/ruby-lsp/pull/2218" this logic isn't executed. I patched this out locally so it at least gives it a try and it does report my repo as a rails app but it fails a bit later since my workaroudn breaks some assumptions later on.

louim · 2024-06-21T13:17:55Z

One issue I'm certain of is that a require "prism" is missing:

Interesting, I'm surprised that no tests fail because of this (at least locally). Maybe because the tests already require Prism somewhere?

andyw8 · 2024-06-21T13:35:29Z

The test helper has require "ruby_lsp/internal" which brings in prism.

lib/ruby_lsp/listeners/rails_app.rb

vinistock · 2024-07-11T14:46:05Z

The changes look good, but there's a test failing.

Morriar · 2024-07-11T14:59:07Z

lib/ruby_lsp/setup_bundler.rb

+      application_contents = config.read if config.exist?
+      return false unless application_contents
+
+      /class .* < Rails::Application/.match?(application_contents)


Should we proof this a bit more? I know the file is generated but I wonder if someone would have some weird cop doing this kind of change?

Suggested change

/class .* < Rails::Application/.match?(application_contents)

/class .* < (::)?Rails::Application/.match?(application_contents)

Happy to apply the fix if we think it makes it more bulletproof. Here's a Github search https://github.com/search?q=%22%3A%3ARails%3A%3AApplication%22&type=code

louim · 2024-07-11T16:03:17Z

The changes look good, but there's a test failing.

@vinistock As I said in the original description, the tests all pass in isolation, but fail when ran together. I was not able to pinpoint the context leak between the tests. I will take another look, but if you have an idea on top of your head, as I'm not very familiar with Minitest so I might have overlooked something obvious.

andyw8 · 2024-07-11T16:20:43Z

I'm looking into. It can be reproduced consistently with this seed:

bundle exec ruby -Itest /Users/andyw8/src/github.com/Shopify/ruby-lsp/test/setup_bundler_test.rb --seed 18973

andyw8 · 2024-07-11T17:18:56Z

The minimal set of tests that causes this are:

- test_uses_absolute_bundle_path_for_bundle_install
- test_creates_custom_bundle_for_a_rails_app

andyw8 · 2024-07-11T17:24:56Z

If I check stderr in run_script I see it contains:

Minitest::Assertion: Expected "Ruby LSP> Skipping lockfile copies because there's no top level bundle"

🤔

Earlopain · 2024-07-11T17:44:41Z

This looks like a bundler caching thing, I ran into something similar over at RuboCop a bit ago. I added this helper to fully isolate bundler (at least as far as was relevant): https://github.com/rubocop/rubocop/blob/66435058ed2310b509febc88f689f4a7e04e9e9e/lib/rubocop/rspec/shared_contexts.rb#L55-L72

If you wrap test_uses_absolute_bundle_path_for_bundle_install in that, then the currently failing spec passes though test_uses_absolute_bundle_path_for_bundle_install would need some changes. Minitest seems to shell out to gdiff for me which then makes assert_equal failing fail because the test asserts against what is being passed to system.

andyw8 · 2024-07-11T18:37:32Z

@Earlopain thanks for the pointer. I've pushed a commit on a different branch, and it's passing:

d9a3072

Will need to discuss with @vinistock if there is a better way to handle.

andyw8 · 2024-07-11T18:40:08Z

Spoke too soon, there might be something else that needs wrapped in with_isolated_bundler.

paracycle · 2024-07-11T22:22:27Z

lib/ruby_lsp/setup_bundler.rb

+    # Detects if the project is a Rails app by looking if the superclass of the main class is `Rails::Application`
+    sig { returns(T::Boolean) }
+    def rails_app?
+      config = Pathname.new("config/application.rb").expand_path(Dir.pwd)


Suggested change

config = Pathname.new("config/application.rb").expand_path(Dir.pwd)

config = Pathname.new("config/application.rb").expand_path

The default value for expand_path is already Dir.pwd: https://ruby-doc.org/3.3.0/File.html#method-c-expand_path

Relative paths are referenced from the current working directory of the process unless dir_string is given, in which case it will be used as the starting point

Thanks, updated!

This change makes the Rails app detection based on the superclass of the main class being `Rails::Application`. This is a more reliable way to detect Rails apps than looking for the presence of the `rails` gem in the Gemfile. Application can require some of the Rails components without requiring the `rails` gem itself. co-authored-by: Earlopain <14981592+Earlopain@users.noreply.github.com>

Earlopain · 2024-07-24T09:54:39Z

I tried investigating the test failure again but didn't make any progress. How about just changing the code a bit to not run into that problem? I tested out the following patch locally and it doesn't appear to fail:

diff --git a/lib/ruby_lsp/setup_bundler.rb b/lib/ruby_lsp/setup_bundler.rb
index 903f43a3..b3ef164b 100644
--- a/lib/ruby_lsp/setup_bundler.rb
+++ b/lib/ruby_lsp/setup_bundler.rb
@@ -285,11 +285,15 @@ module RubyLsp
     # Detects if the project is a Rails app by looking if the superclass of the main class is `Rails::Application`
     sig { returns(T::Boolean) }
     def rails_app?
-      config = Pathname.new("config/application.rb").expand_path
-      application_contents = config.read if config.exist?
-      return false unless application_contents
+      return false unless (application_contents = rails_application_content)
 
       /class .* < Rails::Application/.match?(application_contents)
     end
+
+    sig { returns(T.nilable(String)) }
+    def rails_application_content
+      config = Pathname.new("config/application.rb").expand_path
+      config.read if config.exist?
+    end
   end
 end
diff --git a/test/setup_bundler_test.rb b/test/setup_bundler_test.rb
index 77f3b605..a65d7b3c 100644
--- a/test/setup_bundler_test.rb
+++ b/test/setup_bundler_test.rb
@@ -75,33 +75,33 @@ class SetupBundlerTest < Minitest::Test
   end
 
   def test_creates_custom_bundle_for_a_rails_app
-    Dir.mktmpdir do |dir|
-      Dir.chdir(dir) do
-        FileUtils.mkdir(File.join(dir, "config"))
-        File.write(File.join(dir, "config", "application.rb"), <<~RUBY)
-          module MyApp
-            class Application < Rails::Application
-            end
-          end
-        RUBY
-
-        Object.any_instance.expects(:system).with(
-          bundle_env(".ruby-lsp/Gemfile"),
-          "(bundle check || bundle install) 1>&2",
-        ).returns(true)
-        Bundler::LockfileParser.any_instance.expects(:dependencies).returns({ "rails" => true }).at_least_once
-        run_script
+    Object.any_instance.expects(:system).with(
+      bundle_env(".ruby-lsp/Gemfile"),
+      "(bundle check || bundle install) 1>&2",
+    ).returns(true)
 
-        assert_path_exists(".ruby-lsp")
-        assert_path_exists(".ruby-lsp/Gemfile")
-        assert_path_exists(".ruby-lsp/Gemfile.lock")
-        assert_path_exists(".ruby-lsp/main_lockfile_hash")
-        gemfile_content = File.read(".ruby-lsp/Gemfile")
-        assert_match("ruby-lsp", gemfile_content)
-        assert_match("debug", gemfile_content)
-        assert_match("ruby-lsp-rails", gemfile_content)
+    # There is some unknown state leak with Bundler which prevents us from creating the
+    # folder structure ourselves lest we encounter flaky tests
+    RubyLsp::SetupBundler.any_instance.stubs(:rails_application_content).returns(<<~RUBY)
+      module MyApp
+        class Application < Rails::Application
+        end
       end
-    end
+    RUBY
+
+    Bundler::LockfileParser.any_instance.expects(:dependencies).returns({ "rails" => true }).at_least_once
+
+    run_script
+
+    assert_path_exists(".ruby-lsp")
+    assert_path_exists(".ruby-lsp/Gemfile")
+    assert_path_exists(".ruby-lsp/Gemfile.lock")
+    assert_path_exists(".ruby-lsp/main_lockfile_hash")
+    assert_match("ruby-lsp", File.read(".ruby-lsp/Gemfile"))
+    assert_match("debug", File.read(".ruby-lsp/Gemfile"))
+    assert_match("ruby-lsp-rails", File.read(".ruby-lsp/Gemfile"))
+  ensure
+    FileUtils.rm_r(".ruby-lsp") if Dir.exist?(".ruby-lsp")
   end
 
   def test_changing_lockfile_causes_custom_bundle_to_be_rebuilt

louim requested a review from a team as a code owner June 21, 2024 06:35

louim requested review from andyw8 and st0012 June 21, 2024 06:35

Earlopain mentioned this pull request Jun 21, 2024

Improve detection for rails as the testing framework #2161

Closed

louim force-pushed the improvement/rails-detection branch from 17a19de to 2d5c93f Compare June 21, 2024 14:28

vinistock reviewed Jun 21, 2024

View reviewed changes

lib/ruby_lsp/listeners/rails_app.rb Outdated Show resolved Hide resolved

vinistock added bugfix This PR will fix an existing bug server This pull request should be included in the server gem's release notes labels Jun 21, 2024

louim force-pushed the improvement/rails-detection branch 2 times, most recently from 732be3a to ccdb0a6 Compare June 21, 2024 21:17

Morriar reviewed Jul 11, 2024

View reviewed changes

paracycle reviewed Jul 11, 2024

View reviewed changes

louim force-pushed the improvement/rails-detection branch from ccdb0a6 to 41d69b5 Compare July 15, 2024 13:01

vinistock assigned andyw8 Jul 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Rails app detection based on Rails::Application superclass #2218

Make Rails app detection based on Rails::Application superclass #2218

louim commented Jun 21, 2024 •

edited

Loading

Earlopain commented Jun 21, 2024 •

edited

Loading

louim commented Jun 21, 2024

andyw8 commented Jun 21, 2024

vinistock commented Jul 11, 2024

Morriar Jul 11, 2024

louim Jul 15, 2024

louim commented Jul 11, 2024

andyw8 commented Jul 11, 2024 •

edited

Loading

andyw8 commented Jul 11, 2024

andyw8 commented Jul 11, 2024 •

edited

Loading

Earlopain commented Jul 11, 2024

andyw8 commented Jul 11, 2024 •

edited

Loading

andyw8 commented Jul 11, 2024

paracycle Jul 11, 2024

louim Jul 15, 2024

Earlopain commented Jul 24, 2024

	/class .* < Rails::Application/.match?(application_contents)
	/class .* < (::)?Rails::Application/.match?(application_contents)

	config = Pathname.new("config/application.rb").expand_path(Dir.pwd)
	config = Pathname.new("config/application.rb").expand_path

Make Rails app detection based on Rails::Application superclass #2218

Are you sure you want to change the base?

Make Rails app detection based on Rails::Application superclass #2218

Conversation

louim commented Jun 21, 2024 • edited Loading

Motivation

Implementation

Automated Tests

Manual Tests

Earlopain commented Jun 21, 2024 • edited Loading

louim commented Jun 21, 2024

andyw8 commented Jun 21, 2024

vinistock commented Jul 11, 2024

Morriar Jul 11, 2024

Choose a reason for hiding this comment

louim Jul 15, 2024

Choose a reason for hiding this comment

louim commented Jul 11, 2024

andyw8 commented Jul 11, 2024 • edited Loading

andyw8 commented Jul 11, 2024

andyw8 commented Jul 11, 2024 • edited Loading

Earlopain commented Jul 11, 2024

andyw8 commented Jul 11, 2024 • edited Loading

andyw8 commented Jul 11, 2024

paracycle Jul 11, 2024

Choose a reason for hiding this comment

louim Jul 15, 2024

Choose a reason for hiding this comment

Earlopain commented Jul 24, 2024

louim commented Jun 21, 2024 •

edited

Loading

Earlopain commented Jun 21, 2024 •

edited

Loading

andyw8 commented Jul 11, 2024 •

edited

Loading

andyw8 commented Jul 11, 2024 •

edited

Loading

andyw8 commented Jul 11, 2024 •

edited

Loading