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

RSpec/FilePath is broken with vscode-ruby after 1.39.0 (2020-05-01) #1091

Open
ahukkanen opened this issue Nov 13, 2020 · 16 comments · Fixed by #1099
Open

RSpec/FilePath is broken with vscode-ruby after 1.39.0 (2020-05-01) #1091

ahukkanen opened this issue Nov 13, 2020 · 16 comments · Fixed by #1099

Comments

@ahukkanen
Copy link
Contributor

Details:

  • rubocop-rspec version: 1.43.2 (all versions >= 1.39.0 affected)
  • Rubocop version: 0.92.0

Related issue with vscode-ruby:
rubyide/vscode-ruby#625


The RSpec/FilePath cop does not work with vscode-ruby.

It causes issues such as this one (originating from decidim/decidim#6857):
image

Why this happens is that vscode-ruby runs rubocop inside the folder where the file exists. So, in the example case shown in the screenshot it does the following:

$ cd /home/user/dev/decidim/decidim-assemblies/spec/presenters/decidim/assemblies/admin_log
$ bundle exec rubocop /home/user/dev/decidim/decidim-assemblies/spec/presenters/decidim/assemblies/admin_log/assemblies_type_presenter_spec.rb
Inspecting 1 file
C

Offenses:

assemblies_type_presenter_spec.rb:5:1: C: RSpec/FilePath: Spec path should end with decidim/assemblies/admin_log/assemblies_type_presenter*_spec.rb.
describe Decidim::Assemblies::AdminLog::AssembliesTypePresenter, type: :helper do
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected

I was debugging vscode-ruby to find the root cause but I figured it was this commit in rubocop-rspec that has broken the functionality:
811ae08#diff-5b709885b35566a0c699ad4a3200b9b7acbc28ef640fa56a8e66f6c732420a34

Is there some reasoning behind this change or should vscode-ruby change the way it runs the linter?

This could be fixed for vscode-ruby by changing the following lines in rubocop-rspec:
https://github.com/rubocop-hq/rubocop-rspec/blob/78f94bb3216fa2a6f27a43af47728762caabde89/lib/rubocop/cop/rspec/file_path.rb#L135-L137

To:

filename = File.expand_path(processed_source.buffer.name)

But maybe this would possibly reintroduce the problems that 811ae08 fixed?

It is hard to make the judgement where the actual bug is without knowing some insight about this change. I don't understand why it has been made.

@ahukkanen
Copy link
Contributor Author

@eitoball If you could provide some insight about 811ae08, it might help fixing this.

@pirj
Copy link
Member

pirj commented Nov 13, 2020

@ahukkanen Thanks for reporting, nice catch.

But maybe this would possibly reintroduce the problems

Let's check if specs fail. If they do not - a pull request is welcome.

@ahukkanen
Copy link
Contributor Author

@pirj Actually it breaks that test which was added in that commit linked above.

It tests that this fails:

  • Filepath: "home/foo/spec/models/bar_spec.rb"
  • Contents of the file: describe Foo do; end
  • Expected: Should be in a file named "foo_spec.rb"
  • Matched against glob: "foo*_spec.rb"

This matches that file path because:

home/foo/spec/models/bar_spec.rb
     foo****************_spec.rb

This is incorrect match because the file name should be "foo_spec.rb".

@pirj
Copy link
Member

pirj commented Nov 13, 2020

The spec shouldn't break, as it expected to be detected that it's an incorrect match (expect_offense is used).
I suggest extending the filename_ends_with? so that it is able to detect both cases.
Pull request is welcome!

@ahukkanen
Copy link
Contributor Author

@pirj Yes, it's not trivial. The glob pattern and matching flags need to be changed.

Would do PR if I knew how. Right now I don't know how.

@pirj
Copy link
Member

pirj commented Nov 13, 2020

For sure, I don't have a solution ready off the top of my head. But you can do it. Start with a spec that will pass for your case.
I'll be happy to help you out if you get stuck with something and discuss different approaches in that pull request.

You're in the best position to fix this, as nobody is going to fix it for you.

@ahukkanen
Copy link
Contributor Author

Yeah, I can personally live with it for now, just forking that one line locally - works for me.

I tried to fix it but it's quite complicated, would need to find the proper time for it.

@pirj
Copy link
Member

pirj commented Nov 13, 2020

Do you mind closing this if this is a non-issue for you?

@ahukkanen
Copy link
Contributor Author

No, I don't mind if you feel it's not a bug. At least it's kept here fore the records as other people have reported it for VSCode.

@pirj
Copy link
Member

pirj commented Nov 13, 2020

I feel it's a bug, but I also feel it will never be fixed unless someone wants to scratch their own itch.

To make sure it's eventually fixed I suggest you send a pull request with a pending failing spec.

We are volunteers, and, unfortunately, we can't guarantee feature completeness, correctness or freedom from bugs considering a rich choice of use cases.

Let's take a look at just a few recent tickets:

  • rubocop --auto-correct goes into infinite loop with it { expect(:a).to eq(:b) }
  • RSpec/Enabled: false causes a warning and doesn't turn off RSpec cops
  • 10.times { create(:user, age: rand(10..0)) } makes an incorrect (from spec point of view) correction to create_list(:user, 10, age: rand(10..0))

As you may have already guessed, those are nastier bugs, and they will be addressed earlier. And still we motivate reporters to dive into our codebase and contribute fixes.

You may find tickets dating back to 2016, and that means that nobody cared enough to apply the effort to address them. Given some work/life balance that is tilted already for most open-source software maintainers, and time constraints, this bug will very doubtfully be a priority anytime soon, and very unlikely to be fixed by maintainers themselves.

I really insist on sending a pull request with a pending spec. We have just one pending spec so far, and I personally hate them. Chances are I'll be in the mood to fix minor offences in a bulk, but again, no guarantees, as some cops, e.g. RSpec/PredicateMatcher and FactoryBot/CreateList are way closer to the top of my list.

Thanks for understanding.

@pirj pirj closed this as completed Nov 13, 2020
@ahukkanen
Copy link
Contributor Author

Yeah of course, I totally understand. It's just important to have it documented somewhere, as figuring out the root cause is already half the work.

I might also fix it at some point, I just don't have the time right now to investigate further. Also for me, it's not that big of a deal right now as I can live with the forked version.

If I will send a PR, I'd like to have incorporated a fix as well.

Thanks for your great work!

bquorning added a commit that referenced this issue Dec 13, 2020
Fix false positive for relative file path runs with long namespaces (fixes #1091)
@boris-petrov
Copy link

I've been trying to figure out what's the current status of this issue for a while now by reading issues here and there. The same problem happens to me right now with the latest version of everything. RSpec/FilePath gives a warning in VSCode and no other rubocop-rspec warnings are issued in that file. Running rubocop separately works totally fine. Am I doing something wrong or is this just not fixed yet?

@Darhazer Darhazer reopened this Jul 30, 2022
@4ndv
Copy link

4ndv commented Apr 3, 2023

Looks like vscode-ruby was just deprecated in favor of Ruby LSP extension from Shopify, which does not have the same issue

rubyide/vscode-ruby#815 (comment)

@difernandez
Copy link

I just ran into this using Ruby LSP, so I think it might still be an issue?

@4ndv
Copy link

4ndv commented Oct 25, 2023

I just ran into this using Ruby LSP, so I think it might still be an issue?

Have you uninstalled vscode-ruby?

@difernandez
Copy link

Yep, I have, thanks for the suggestion. I'm only seeing it in one file, and copying the same file in another project doesn't show the same issue, so it might just be a weird thing with my setup? Unless more people say they're also still experiencing this, might not be worth checking out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants