Old specs need love

[marcandre]

Many cop specs still use inspect_source or autocorrect_source. We'll like to deprecated this (see #8003), so they should be refactored to use expect_offense + expect_correction instead (I'd love to have an even nicer API for this).

[rrosenblum]

Some of these are much easier to convert than others. Most of the tests are as easy as merging an inspect test with and autocorrect test and rewriting a couple of lines.

The tests that make use of shared_examples will likely need to be rewritten to not use example groups. Since the code is dynamic, you likely won't be able to get the highlighting to work in a clean way.

[marcandre]

The tests that make use of shared_examples will likely need to be rewritten to not use example groups. Since the code is dynamic, you likely won't be able to get the highlighting to work in a clean way.

Like which one?

We introduced ^{label} and _{label} to ease variable length matchers

[rrosenblum]

Interesting, I didn't know ^{label} and _{label} had been added. It looks like those would solve the issue of having different length highlights!

[tejasbubane]

@marcandre I am willing to take some of this refactoring work. What would you prefer of the following?

1. Separate PRs each with 2-3 cops' specs refactored - easy to review - don't worry about git history.

2. Club all Style cops into one PR, Lint in another, so on.. - pain to review - will keep git history clean.

Or anything else?

[marcandre]

Either work, I'd say group them as you like.

[koic]

I'm not sure if it could be a hint, the following is an examples that PRs were split into A-Z and opened in the past.
#5134, #5132, #5131, #5130, and #5129.

[biinari]

Some new syntax of use here:

• ^{} to mark an offense on an empty line (added in Add expect_offense mark for offense on empty line #8255)
• [...] to abbreviate offense messages (added in [Fixes #8286] Allow abbreviated offense messages with expect_offense #8297)

[biinari]

I'm planning to pick up some more of these this weekend, to pick up the ones I missed in the Lint namespace and make a start on the Style namespace (probably A-M split over 2 or 3 PRs).

[mechos3d]

Hi all!
I'm planning to refactor all the Layout namespace specs if it's OK :)

[biinari]

I'm picking up the rest of the Style namespace cops this week as I have a bit of free time.

[AndreiEres]

I ran into a problem while I was working on the task. I can't understand how to implement expect_offense for missing final newline.

To throw an error I've written a wrong spec

expect_no_offenses(<<~RUBY.chomp)
  puts 'testing rubocop when final new line is missing
                            after block comments'

  =begin
  first line
  second line
  third line
  =end
RUBY

The error was

-=end
+=end    ^{} Final newline missing.

So if I'll write =end ^{} Final newline missing. in spec it won't caught. If I move error message to next line error will disappeared because we will have the newline.

Have anybody already met the problem? Is there a solution or do we have to enhance parser?

[marcandre]

@AndreiEres Good point. You are right, I don't think there's a way around this one. We could add some functionality to expect_offense but since this case is very rare, probably best to leave it as is.

[mechos3d]

I have a solution for it, but life happened and didn't create a PR with it yet.
I created a separate matcher expect_newline_offense that works like this:

expect_newline_offense("#{str}=end", <<~RUBY)
  #{str}=end
      ^{} Final newline missing.
RUBY

But @marcandre is right in that it's a very rare case - so I was hesitant wether this matcher is worth adding or not.

[marcandre]

If you have something that works @mechos3d, we can definitely include it.
Another "sneaky way" might be to check if the string we're being passed is missing a newline at the end, and if so reproduce that when we extract the failure messages. That might even be simpler.

[AndreiEres]

So I've used error raising in #9263 because that is built-in and needn't additional helpers. Thus we've completed with using inspect_source (if you accept PR of course).

[AndreiEres]

Could you tell me, please? What can we do with old syntax in that files?
spec/support/alignment_examples.rb
spec/support/multiline_literal_brace_layout_trailing_comma_examples.rb

[AndreiEres]

Could you tell me, please? What can we do with old syntax in that files?
spec/support/alignment_examples.rb
spec/support/multiline_literal_brace_layout_trailing_comma_examples.rb

@marcandre

[marcandre]

spec/support/multiline_literal_brace_layout_trailing_comma_examples.rb

I don't see an issue. If the failure message is different, ^{foo} [...] should match any failure with indent with the length of foo.

spec/support/alignment_examples.rb

I don't see an issue, the two cases should be combined and expect_correction used instead of autocorrect_source.

Or am I missing something?

[AndreiEres]

Or am I missing something?

I mean if there are no issues could we close the issue?

[marcandre]

There are still 4 uses of autocorrect_source and 1 of inspect_source in the specs that would ideally be removed

[biinari]

I'll tackle these last ones if no objections, I think they should pretty reasonable to finish off. Sorry I've been absent from this for a long while, life happened.

spec/support/alignment_examples.rb
spec/support/multiline_literal_brace_layout_trailing_comma_examples.rb

[rrosenblum]

This is amazing that we finally get to close this out! This spec conversion has been years in the making. Great job everyone that helped out!