rubocop-rspec icon indicating copy to clipboard operation
rubocop-rspec copied to clipboard

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

Open ahukkanen opened this issue 4 years ago • 16 comments

Details:

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

Related issue with vscode-ruby: https://github.com/rubyide/vscode-ruby/issues/625


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

It causes issues such as this one (originating from https://github.com/decidim/decidim/pull/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: https://github.com/rubocop-hq/rubocop-rspec/commit/811ae08ea3a60453e18a8bcb9dfaefafd848f779#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 https://github.com/rubocop-hq/rubocop-rspec/commit/811ae08ea3a60453e18a8bcb9dfaefafd848f779 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 avatar Nov 13 '20 10:11 ahukkanen

@eitoball If you could provide some insight about https://github.com/rubocop-hq/rubocop-rspec/commit/811ae08ea3a60453e18a8bcb9dfaefafd848f779, it might help fixing this.

ahukkanen avatar Nov 13 '20 10:11 ahukkanen

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

pirj avatar Nov 13 '20 11:11 pirj

@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".

ahukkanen avatar Nov 13 '20 13:11 ahukkanen

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!

pirj avatar Nov 13 '20 14:11 pirj

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

ahukkanen avatar Nov 13 '20 14:11 ahukkanen

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.

pirj avatar Nov 13 '20 14:11 pirj

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.

ahukkanen avatar Nov 13 '20 14:11 ahukkanen

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

pirj avatar Nov 13 '20 14:11 pirj

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.

ahukkanen avatar Nov 13 '20 15:11 ahukkanen

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 avatar Nov 13 '20 19:11 pirj

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!

ahukkanen avatar Nov 13 '20 19:11 ahukkanen

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?

boris-petrov avatar Jul 30 '22 09:07 boris-petrov

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

https://github.com/rubyide/vscode-ruby/issues/815#issuecomment-1493321750

4ndv avatar Apr 03 '23 09:04 4ndv

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

difernandez avatar Oct 25 '23 12:10 difernandez

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

Have you uninstalled vscode-ruby?

4ndv avatar Oct 25 '23 16:10 4ndv

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

difernandez avatar Oct 26 '23 12:10 difernandez