Add RSpec style guide rule for code that tests our test tools.

[Bodacious]

I've added a new suggestion to our RSpec style guide.

Recently, when adding custom matchers to the test suite, I felt that the behaviour of these was complex enough that it warranted its own tests.

Since these files were in spec/support, they were being loaded automatically in rails_helper.rb each time the specs are run—even when only running an isolated example.

This is, of course, caused by the boilerplate line:

Dir[Rails.root.join("spec", "support", "**", "*.rb")].each { |f| require f }

My solution to dealing with this was to use the spec extension. I think it would be sensible if we use this extension type for all tests that specifically test code that's part of the test suite.

Feedback welcome!

[tejanium]

I like the proposal, it's like how some JS devs test their code by adding foo.test.js next to the implementation of foo.js

Since these files were in spec/support

But, what do we think to leave spec/support for helpers implementation and test these helpers somewhere else? for example spec/spec_helpers / spec/spec_matchers

[Bodacious]

@tejanium I considered something like that too. I'm not sure where the best place to put these is though? 🤷‍♂️

Another idea is to put the test classes into lib, and then the corresponding tests in spec/lib.

But I would be happy with spec/matchers/... or similar too.

Another possible benefit of this suggestion, is that the we don't run need to test our test code in CI. (We can exclude by matching spec/**/*_spec.rb

[tejanium]

Another possible benefit of this suggestion, is that the we don't run need to test our test code in CI. (We can exclude by matching spec/*/_spec.rb

I'm not sure about this being a benefit. We expect our application specs to be correct, and if any application specs depend on the matchers being correct, I think it sensible to also make sure the matchers are correct by running the matchers specs.

I'm not sure where the best place to put these is though?

I don't think there are going to be any objective metrics/properties we can look at to what is the best place/name for this, I think we need to just pick a sensible one and stick to it.

From where I'm standing, spec/matchers seems like a sensible pick (until we have matchers directory under app 😅 )

[Bodacious]

I'm not sure about this being a benefit. We expect our application specs to be correct, and if any application specs depend on the matchers being correct, I think it sensible to also make sure the matchers are correct by running the matchers specs.

@tejanium yes, that's a good point. I agree.

From where I'm standing, spec/matchers seems like a sensible pick

For the avoidance of doubt, is this instead of or as well as using the .spec extension for test suite tests.

until we have matchers directory under app 😅

😂

[tejanium]

For the avoidance of doubt, is this instead of or as well as using the .spec extension for test suite tests.

Ah yes, forgot to mention, yes, the main point I was trying to avoid is to introduce one more custom Thing to Remember™ in our codebase. Rspec's convention is to have a prefix test file using _spec with the normal .rb extension. Having a new .spec extension under Rspec can lead to another Thing to Remember™ like _"Wait.. was it _spec.spec or .spec?"_

So let's use our standard Rspec's conventions, so for /spec/support/matchers/tinder_matcher.rb the spec file would be spec/<whatever the name>/tinder_matcher_spec.rb

[Bodacious]

@tejanium Yep, I'm happy with that.

Will close this PR and refactor my changes elsewhere.

Thanks for the discussion! 🙇‍♂️