Add expect_issue and expect_no_issues spec helpers

[FnControlOption]

These are based on RuboCop's expect_offense and expect_no_offenses.

Todo:

• Convert some existing specs
• Add spec for AnnotatedSource
• Add documentation
• Further clean up RuboCop vestiges

Future PRs:

• Use diff shard or similar to generate diffs in expect_issue
• Add autocorrect Add autocorrect #248

Old:

it "reports multiple shared variables in spawn" do
  s = Source.new %(
    foo, bar, baz = 0, 0, 0
    while foo < 10
      baz += 1
      spawn do
        puts foo
        puts foo + bar + baz
      end
      foo += 1
    end
  )
  subject.catch(s).should_not be_valid
  s.issues.size.should eq 3
  s.issues[0].location.to_s.should eq ":5:10"
  s.issues[0].end_location.to_s.should eq ":5:12"
  s.issues[0].message.should eq "Shared variable `foo` is used in fiber"

  s.issues[1].location.to_s.should eq ":6:10"
  s.issues[1].end_location.to_s.should eq ":6:12"
  s.issues[1].message.should eq "Shared variable `foo` is used in fiber"

  s.issues[2].location.to_s.should eq ":6:22"
  s.issues[2].end_location.to_s.should eq ":6:24"
  s.issues[2].message.should eq "Shared variable `baz` is used in fiber"
end

New:

it "reports multiple shared variables in spawn" do
  expect_issue subject, %(
    foo, bar, baz = 0, 0, 0
    while foo < 10
      baz += 1
      spawn do
        puts foo
            # ^^^ error: Shared variable `foo` is used in fiber
        puts foo + bar + baz
            # ^^^ error: Shared variable `foo` is used in fiber
                        # ^^^ error: Shared variable `baz` is used in fiber
      end
      foo += 1
    end
  )
end

[veelenga]

Thanks for the suggestion.

1. I've seen the rubocop's dsl and was not sure if we should introduce it into Ameba. It looks much more concise, yes, however, it also looks a bit strange to me and I was curious if there are more idiomatic and declarative ideas/ways.

2. I would remove the diff from the scope and add it as a separate feature.

[FnControlOption]

however, it also looks a bit strange to me and I was curious if there are more idiomatic and declarative ideas/ways.

Perhaps Sorbet-style errors would be an improvement? For example:

# ameba:foo Lint/BadDirective # error: Bad action in comment directive: 'foo'. [...]

[1, 2, 3].reject { |e| e > 2 }.any?
        # ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: Use `any? {...}` instead of `reject {...}.any?`

it can happen that there are multiple issues reported by the same (or some other) rules. Would it be possible to catch that as well?

This is possible, yes.

[veelenga]

Yeah, sorbet-style errors look great to me.

[FnControlOption]

I might start working on some form of autocorrect now. How does that sound?

[veelenga]

That would be very helpful.

Would you mind to do the following as part of this PR:

1. cover AnnotatedSource by specs
2. convert those two files to new matchers, so the specs are consistent at least in the same file

• spec/ameba/rule/lint/shared_var_in_fiber_spec.cr
• spec/ameba/rule/performance/any_after_filter_spec.cr

I can convert rest of the specs on my own

[FnControlOption]

I already converted most uses of should be_valid so you can apply this patch when you start converting specs: https://gist.github.com/FnControlOption/b10358b166061d8ff3d6f3283bb3c02c

Or if it's easier, I've pushed this to my fork in the tmp-report_issue2 branch: FnControlOption@4ec325c

[FnControlOption]

Also, let me know if I should add more tests for AnnotatedSource because I kinda lost track of what I was doing while in the middle of adding them, haha 😅

[Sija]

I left just a couple of minor comments, but overall it's great! These helpers will make testing much more pleasant ❤️

[veelenga]

LGTM 💟

[Sija]

Looks good, I've added a few style-related comments.