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

Detect rails as the testing framework if bin/rails exists #1896

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

Earlopain
Copy link
Contributor

@Earlopain Earlopain commented Apr 6, 2024

Motivation

I have an application that depends on Rails in the following way:

# Gemfile
rails_version = "~> 7.1.0"
gem "actionpack",    rails_version
gem "actionview",    rails_version
gem "activemodel",   rails_version
gem "activerecord",  rails_version
gem "activesupport", rails_version
gem "railties",      rails_version

The intent is to not pull in the other framework parts, like activestorage for example. Since the testing framework detection currently does a check for rails I am not getting code lenses for tests with ruby-lsp-rails.

See Shopify/ruby-lsp-rails#47 for similar reasoning.

Implementation

Also check for railties. A rails app doesn't boot if railties isn't present (require "rails/commands" in bin/rails is the first failure point).

Automated Tests

Yup.

Manual Tests

Create a rails app with the above gemfile, create a test, observe code lense.

@Earlopain Earlopain requested a review from a team as a code owner April 6, 2024 16:33
@Earlopain Earlopain requested review from andyw8 and KaanOzkan April 6, 2024 16:33
@andyw8
Copy link
Contributor

andyw8 commented Apr 8, 2024

Thanks for the PR. I have a slight concern that there could be some gems that depend on railties but use plain minitest. 🤔

@Earlopain
Copy link
Contributor Author

Hm, right. What about also checking for bin/rails instead?

Something like

File.exist?(File.join(workspace_path, "bin/rails"))

@andyw8 andyw8 added the enhancement New feature or request label Apr 8, 2024
@andyw8
Copy link
Contributor

andyw8 commented Apr 8, 2024

Yeah, that would have less chance of a false positive. Any thoughts @st0012, @vinistock ?

@andyw8 andyw8 added the server This pull request should be included in the server gem's release notes label Apr 8, 2024
@Earlopain Earlopain force-pushed the test-framework-rails-subset branch from 7b62b9a to 463c4eb Compare April 8, 2024 15:55
@Earlopain Earlopain changed the title Detect rails as the testing framework if direct dependency on railties Detect rails as the testing framework if bin/rails exists Apr 8, 2024
lib/ruby_lsp/global_state.rb Outdated Show resolved Hide resolved
@st0012 st0012 added bugfix This PR will fix an existing bug and removed enhancement New feature or request labels Apr 8, 2024
@st0012
Copy link
Member

st0012 commented Apr 8, 2024

I changed the label to bugfix because IMO we should have used bin/rails instead. Thanks for the PR 🙏

@Earlopain Earlopain force-pushed the test-framework-rails-subset branch from 463c4eb to 908f276 Compare April 8, 2024 17:55
@Earlopain
Copy link
Contributor Author

Seems somewhat relevant to #1466, #1467. While not the same I find it close enough to at least link to.

# by ruby-lsp-rails. A Rails app doesn't need to depend on the rails gem itself, individual components like
# activestorage may be added to the gemfile so that other components aren't downloaded. Check for the presence
# of bin/rails to support these cases.
elsif File.exist?(File.join(workspace_path, "bin/rails"))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
elsif File.exist?(File.join(workspace_path, "bin/rails"))
elsif File.exist?(File.join(workspace_path, "bin", "rails"))

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry I clicked merge at the same time 😅

Copy link
Member

Choose a reason for hiding this comment

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

Let's apply this change. We can't use forward slashes for Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, is that really the fix then? https://ruby-doc.org/3.3.0/File.html#method-c-join doesn't mention this being platform-aware and seems to always use / anyways.

Copy link
Member

Choose a reason for hiding this comment

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

Then maybe we should be using Pathname instead and need to switch this to

Suggested change
elsif File.exist?(File.join(workspace_path, "bin/rails"))
elsif Pathname.new(workspace_path).join("bin").join("rails").exist?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, maybe Ruby already takes care of that under the hood https://github.com/ruby/ruby/blob/b09604e1fd5768daf31aaa4f8130fa8cd9b8d240/file.c#L252.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is fine, yeah. I wanted to properly check in my VM but I don't think I'm going through the bother of properly setting ruby up, so a boring irb session will have to do:

image

@st0012 st0012 merged commit aa46c58 into Shopify:main Apr 8, 2024
21 checks passed
@vinistock
Copy link
Member

This PR will close #1466, so let's add to the description.

@Earlopain
Copy link
Contributor Author

I don't think it will close this? There are still a few other places that check against a rails dependecy here.

@vinistock
Copy link
Member

Do you mean in the custom bundle logic? If so, yeah that's true.

@Earlopain Earlopain deleted the test-framework-rails-subset branch April 8, 2024 18:27
Earlopain added a commit to Earlopain/ruby-lsp-rails that referenced this pull request Apr 24, 2024
Since Shopify/ruby-lsp#1896 the testing framework for rails
is dependant on the presence on `bin/rails`
andyw8 pushed a commit to Shopify/ruby-lsp-rails that referenced this pull request Apr 24, 2024
Update tests for ruby-lsp changes

Since Shopify/ruby-lsp#1896 the testing framework for rails
is dependant on the presence on `bin/rails`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants