Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fully qualify classnames in specs #48

Closed
wants to merge 1 commit into from

Conversation

toddmohney
Copy link
Contributor

Fully qualified classnames do not suffer from auto-loading issues

@wfleming
Copy link
Contributor

In previous discussions, I think @pbrisbin felt strongly that the nested style had advantages that were preferable in general, so the fully-qualified style was one we would adopt for Rails projects (since it's Rails behavior that causes the autoloading issue), but not all Ruby projects.

@pbrisbin
Copy link
Contributor

I believe the nested style has the following benefit: it's syntactically easier to refer to the subject-under-test and any collaborators that share its namespace. I believe as we move into monorepo and form more rich and co-located sub-namespaces, this will become even more valuable.

The downside you mention only exists in auto-loading scenarios, which is Rails. Rails makes up less than 20% of our codebases (depending on what you count). I'm opposed to changing our overall style, and losing the benefit I mentioned, specifically to work around Rails' short-comings.

I propose we make the rule depend on Rails:

  • In Rails codebases, fully qualify the subject under test
  • In non-Rails codebases, used "nested describe" syntax

@toddmohney
Copy link
Contributor Author

As for referring to the system-under-test, I prefer described_class for a number of reasons: #49

As for the collaborators, I'd prefer to accentuate the pain of referring to a bunch of those in test. It's usually a smell 😉!

@pbrisbin
Copy link
Contributor

I think we're in an agree-to-disagree state on this one. I'm happy to take a vote and will abide by whatever the team thinks is best. Whatever we decide, it'd be great if we could bend fixme to enforce it (or maybe rubocop-rspec has stuff for this?).

@wfleming
Copy link
Contributor

My personal vote, if it comes down to a vote:

I am nay on the describe Foo::Bar over module Foo\ndescribe Bar, and tend to agree with Pat on that one.

I am yay on allowing described_class (#49). I agree with the idea that overuse of let/subject/described_class can hurt more than they help, but described_class in particular is 1) less surprising in behavior compared to the other two and 2) does really help with refactoring in some cases and 3) will make describe Foo::Bar specs in Rails apps less horrible to read.

or maybe rubocop-rspec has stuff for this?

From what I saw it didn't, but I found writing the custom rubocop rule for app to be straightforward: we could easily write an equivalent to flag the other style for non-Rails apps.

@toddmohney toddmohney closed this Dec 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants