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

Ensure spec names are correct #248

Closed
wants to merge 1 commit into from
Closed

Ensure spec names are correct #248

wants to merge 1 commit into from

Conversation

blowmage
Copy link

@blowmage blowmage commented Mar 8, 2013

Without this change I'm seeing spec names as "#Class:0x007ff82406a170", which blocks minitest-rails from being able to resolve the model/controller/mailer/etc from the spec name.

@zenspider
Copy link
Collaborator

I'm having to do (super rescue "dunno") on 1.8 because it freaks out that there is no to_s to super to. I don't get it.

@blowmage
Copy link
Author

blowmage commented Mar 8, 2013

Ugh, I stopped running minitest-rails tests against 1.8 and so I didn't catch that.

Yeah... I can't think of a better way to do that than that.

def to_s # :nodoc:
  defined?(@name) ? @name : (super rescue "")
  # defined?(@name) ? @name : (super rescue "[Spec name missing]")
  # defined?(@name) ? @name : (super rescue "[unknown name]")
end

@zenspider
Copy link
Collaborator

Worse yet, I think it is an oddity in 1.8 at parse time only... I don't think it is even possible to hit the rescue (in this case) because everything all the way up has a to_s.

@metaskills
Copy link

This illustrates the issue.

module DSL
  def to_s
    super
  end
  alias :name :to_s
end

class Bar
  extend DSL
end

puts Bar.to_s
puts Bar.name # dsl_fail.rb:3:in `name': super: no superclass method `to_s' for Bar:Class (NoMethodError)
              # from dsl_fail.rb:13

This fixed the problem with a test. I can do a pull request?

https://gist.github.com/metaskills/5125357

@blowmage
Copy link
Author

@metaskills Is the change in 5577af8 sufficient? If so, I think this pull request could be closed.

@metaskills
Copy link

No it is not, that was the point of the whole conversation I had here

06b9ce3#commitcomment-2772287

I.8 is failing because class names come back as "dunno"

@metaskills
Copy link

Did you see the tests I added in the gist? Should I just open up a new pull request/issue?

@blowmage
Copy link
Author

Oh, because name has no super... I gotcha. I'll work up a failing test for that and update this pull request with a fix.

@metaskills
Copy link

But I have a failing test!!!

https://gist.github.com/metaskills/5125357

@metaskills
Copy link

And a fix :) am I taking crazy pills?

@metaskills
Copy link

OK, I just created pull request #251 just getting flustered as I said I had this fixed and was trying to communicate with y'all before doing a pull request because Ryan asked first for a "repro" in the discussion here. So rather than going on a 3rd time how I have a working patch, let the PR in #251 speak for itself.

@ghost ghost assigned zenspider Mar 18, 2013
@zenspider zenspider mentioned this pull request Mar 18, 2013
@zenspider
Copy link
Collaborator

Unbunch the panties. I've been nose down working on my talk for MWRC and haven't come up for air until now.

I went with something in-between both your patches. Kept the code up in the module so anyone benefits but went with blowmage's implementation to avoid needless duplication. I also liked his tests more as they were slightly more complex (3 layers deep).

I'll get this out shortly.

@zenspider zenspider closed this Mar 18, 2013
zenspider added a commit that referenced this pull request Mar 18, 2013
…bug. (blowmage and metaskills respectively)

[git-p4: depot-paths = "//src/minitest/dev/": change = 8299]
@metaskills
Copy link

Unbunch the panties.

Thanks, I need that sometimes :) cheers and brews to you both 🍻

zenspider added a commit that referenced this pull request Apr 17, 2013
…bug. (blowmage and metaskills respectively)

[git-p4: depot-paths = "//src/minitest/dev/": change = 8299]
@minitest minitest locked and limited conversation to collaborators May 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants