Make Rails app detection based on Rails::Application superclass

[louim]

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]

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]

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]

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

[vinistock]

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

[louim]

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]

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]

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]

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]

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]

@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]

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

[Earlopain]

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

[andyw8]

@Earlopain I've taken your suggestion but made a slight change so that we use a file fixture instead of needing to stub the private method (which may be brittle).

[andyw8]

Any further thoughts @vinistock @st0012 ?

[andyw8]

Hmm, as per @vinistock's comment here, should we remove the bin/rails check and use this instead?

[Earlopain]

I think that is fine to leave as is, the rails plugin uses bin/rails to run tests. If you switch that over you loose that for engines which don't contain the application config.

[louim]

Hey! This looks like it's ready to merge. It would be nice to have this in.

[andyw8]

Seems there is an issue when the file contains non-ASCII characters: #2463