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

Fix helper specs with internal classes #1499

Merged
merged 2 commits into from Dec 10, 2015

Conversation

fables-tales
Copy link
Member

A rebase of #1340. Got a few more things to fix up.

@JonRowe
Copy link
Member

JonRowe commented Dec 6, 2015

LGTM, whats still outstanding?

@fables-tales
Copy link
Member Author

There's this from @cupakromer #1340 (comment) but maybe we already fixed it? @cupakromer can you weigh in here?

end
else
def determine_default_helper_class(_ignore)
return unless Module === described_class && !(Class === described_class)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to rewrite this boolean expression to read with less complexity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not groking why theres two different names for these methods

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these have to be rails hooks

@cupakromer
Copy link
Member

Ok, I remember what I was thinking before. It was more a question of does this make conceptual / semantic sense, not that I had a problem with the implementation.

Essentially, the nested group will have :type => :helper - with all of the helper stuff mixed into it - even though it is not specing a helper. I think before this concerned me as it will bloat and slow the nested group unnecessarily. It also may be misleading for tools / plugins that look for :type => :helper.

I did some basic sanity checking tonight and I don't have any real problems with this. My initial concern about the metadata and what goes along with it still stands but it's not something I feel is a blocker. There is one corner case that may be confusing to people:

# I ran this using Rails 4.2.5 and rspec-rails 3.4.0
require "rails_helper"

module FooHelper
  def foo_helper
    :foo
  end

  class InnerClass
  end
end

RSpec.describe FooHelper, type: :helper do
  def self.determine_default_helper_class(_ignore)
    return unless Module === described_class && !(Class === described_class)
    described_class
  end

  specify "#foo_helper returns :foo" do
    expect(helper.foo_helper).to be(:foo)
  end

  describe FooHelper::InnerClass do
    it "it has a helper" do
      expect(helper).to be
    end

    it "the helper does not have behavior like the parent group's" do
      expect {
        helper.foo_helper
      }.to raise_error(NoMethodError)
    end
  end
end

@fables-tales
Copy link
Member Author

@cupakromer I think that is a an edge case that we're unlikely to encounter, given that this is already a fix for an edge case. @cupakromer @JonRowe is this good to merge?

@cupakromer
Copy link
Member

Yeah good to merge. My concern isn't a blocker. Do you think there's a way to leverage Configuration#define_derived_metadata? Or would that potentially change metadata semantics too much to be even more confusing?

fables-tales pushed a commit that referenced this pull request Dec 10, 2015
…asses

Fix helper specs with internal classes
@fables-tales fables-tales merged commit b2d6f8e into master Dec 10, 2015
@fables-tales fables-tales deleted the fix-helper-specs-with-internal-classes branch December 10, 2015 14:11
@fables-tales
Copy link
Member Author

@cupakromer I think changing metadata semantics in a minor version is a bad idea. I think we should revisit when we start work on 4.0 proper.

fables-tales pushed a commit that referenced this pull request Dec 10, 2015
sebjacobs pushed a commit to futurelearn/rspec-rails that referenced this pull request Mar 15, 2019
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.

None yet

4 participants