Add cop for Capybara expectations set on current_path

[timrogers]

Setting expectations on current_path in our Capybara feature specs leads to flaky tests. Switching to using Capybara's have_current_path matcher helps since it uses Capybara's waiting functionality that ensures that preceding actions (like click_link) have completed.

This cop tells you not to use expect(current_path).to match(/lol/), encouraging you to use the have_current_path matcher instead.

[Darhazer]

You have to:

• add entry to the config/default.yml (run bin/build_config)
• add entry to the changelog
• add bad and good examples as comments (see the other cops)
• add test that shows you can have current_path outside of the expectation block (like just assigning to a variable)
• I see often expect(page.current_path).to. Please cover this scenario

[timrogers]

Will do! Thanks @Darhazer - really helpful list.

[timrogers]

@Darhazer I've managed to do everything except no. 1 - I get the following:

$ bin/build_config
/Users/timrogers/Projects/rubocop-rspec/lib/rubocop/rspec/config_formatter.rb:23:in `fetch': key not found: "Capybara/CurrentPathExpectation" (KeyError)
	from /Users/timrogers/Projects/rubocop-rspec/lib/rubocop/rspec/config_formatter.rb:23:in `block in unified_config'
	from /Users/timrogers/Projects/rubocop-rspec/lib/rubocop/rspec/config_formatter.rb:22:in `each'
	from /Users/timrogers/Projects/rubocop-rspec/lib/rubocop/rspec/config_formatter.rb:22:in `each_with_object'
	from /Users/timrogers/Projects/rubocop-rspec/lib/rubocop/rspec/config_formatter.rb:22:in `unified_config'
	from /Users/timrogers/Projects/rubocop-rspec/lib/rubocop/rspec/config_formatter.rb:16:in `dump'
	from bin/build_config:20:in `<main>'

Any ideas? I'm sure I'm missing something obvious!

[Darhazer]

just make sure bin/rake passes

[Darhazer]

I believe this could be written as (send nil :expect (send {(send nil :page) nil} :current_path)})

[Darhazer]

@timrogers you need to add the key in config/default.yml. build_config will take care to sync description between the cop and the config

[timrogers]

@Darhazer Aha - thanks! All done and I've pushed a finished, squashed version.

[Darhazer]

Great job, @timrogers

Would you be willing to add another one, to check for not_to have_ calls, that should be avoided for the very same reason (see #378)?

[timrogers]

Yes, sure!

[Darhazer]

Note: generally only eq / match kind of matchers can be replaced with have_current_path. I didn't see other usages though; and even if someone uses current_path with some fancy matcher, refactoring to have_current_path is the right thing to do. So I'm going to merge this as is, unless there are any objections; the note should be respected however if someone adds an autocorrect to this.

@bquorning any thoughts?

[timrogers]

That makes sense to me. I was aware that people might be using other matchers, but where they are they’re likely to see non-deterministic test results such that they’d be well-advised to change their implementation. I’ll look at the other cop you suggested tomorrow, and may look at implementing an autocorrect too - but let’s merge this as-is for now if that’s okay with @bquorning as this feels like a positive addition.

…

On Tue, 26 Sep 2017 at 20:13, Maxim Krizhanovsky ***@***.***> wrote: Note: generally only eq / match kind of matchers can be replaced with have_current_path. I didn't see other usages though; and even if someone uses current_path with some fancy matcher, refactoring to have_current_path is the right thing to do. So I'm going to merge this as is, unless there are any objections; the note should be respected however if someone adds an autocorrect to this. @bquorning <https://github.com/bquorning> any thoughts? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#470 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHFpio8Uz4L0G9-6_YFbdGH3DVirUBTks5smUy9gaJpZM4PkV9E> .

[bquorning]

This is a semi-alphabetical list (RSpec/ is before Capybara/) – could we move this entry to line 328?

[bquorning]

There are no captures in the node matcher, so no block arguments are passed: You can remove |_match|.

https://github.com/bbatsov/rubocop/blob/e7cfdc47c0a7522c4de64af82f0ea0e04b22f1c8/lib/rubocop/node_pattern.rb#L18-L19

[bquorning]

Nitpick: If you use double quotes around the string, you don’t need to escape the ' in the middle. (also in line 18).

[timrogers]

@bquorning Done!