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

Proxy Object should inherit from BasicObject, to avoid clashes with methods on Object. eg present? defined by ActiveSupport #216

Closed
wants to merge 2 commits into from

Conversation

kirillrdy
Copy link

I hope issue title is descriptive enough.

methods on Object. eg present? defined by ActiveSupport
@coveralls
Copy link

Coverage Status

Coverage decreased (-0%) when pulling 54a4fd8 on kirillrdy:master into a4d6342 on watir:master.

@kirillrdy
Copy link
Author

After looking at output of failed build I see my naive mistake.
I'll try to fix and reissue pull request.

Kirill R

@kirillrdy
Copy link
Author

This should fix failing tests

@p0deje
Copy link
Member

p0deje commented Aug 8, 2013

Thanks, can you also write a spec for this?

@kirillrdy
Copy link
Author

Yeah, i will.
would you prefer me requiring activesupport or just mocking #present? on Object ?

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 41e533e on kirillrdy:master into a4d6342 on watir:master.

@p0deje
Copy link
Member

p0deje commented Aug 8, 2013

I think mocking would be enough

@kirillrdy
Copy link
Author

In case Google ever finds this, I should write a better description of this issue.
when using browser.elemen(something).wait_until.present? will always pass when you have activesupport, since active support defines #present? on Object. Which means WhenPresentDecorator#method_missing will never be called for #present?

Kirill R

@p0deje
Copy link
Member

p0deje commented Aug 8, 2013

I guess you mean wait_until_present.

I'm using watir-webdriver for rails testing and has never seen this problem, since Watir::Element redefines it, so not sure. That's why I asked for a spec :)

@kirillrdy
Copy link
Author

No, I dont mean wait_until_present, since that calls Watir::Wait.until.
then when_present, however returns WhenPresentDecorator, which uses method_missing to pass chain methods to @element.
let me know if you still dont see issue,
I am free to google talk @kirillrdy

@jarib
Copy link
Member

jarib commented Aug 8, 2013

@kirillrdy I can't really see the purpose of calling element.when_present.present? - it will always either raise a timeout error or return true.

@p0deje
Copy link
Member

p0deje commented Aug 8, 2013

Oh, I see now. I agree with @jarib, why would you want to call this sequence?

@kirillrdy
Copy link
Author

assuming #present? is missing on WhenPresentDecorator, it should only timeout after 30 seconds.
and yes, i dont call it in sequence.

@kirillrdy
Copy link
Author

just to give example of code we use in tests
element = browser.div(class: 'some-element').when_present
....
element.should be_present

that will always pass since present? was redefined by active support and always returns true for non nil objects.

I will write a test to show this, forgive my timing.

Kirill R

@p0deje
Copy link
Member

p0deje commented Aug 8, 2013

I'd say that this is a bad verification, since you already verify presence with when_present implicitly.

You should perform these assertions in the same fashion as watir-rspec does it

@kirillrdy
Copy link
Author

@p0deje, when_present is non blocking, so it doesnt verify for presence until we call a method on return value of when_present.

The reason we use when_present because it returns a promise object against which we can test other things, eg @when_present.text.should == "something"

Hope this helps
Kirill R

@p0deje
Copy link
Member

p0deje commented Aug 8, 2013

@kirillrdy Yeah, but when_present will raise error if element won't become present on any subsequent calls.

It works correct when using with #text or any other method except to #present?, doesn't it?

@kirillrdy
Copy link
Author

http://api.rubyonrails.org/classes/Object.html#method-i-present-3F

active_resource defines #present? on Object, so Watir::WhenPresentDecorator#method_missing will never be called for #present?

KirillR

@jarib
Copy link
Member

jarib commented Aug 8, 2013

That's understood. The question is why would you want to call present? here? The call will never return false, since WhenPresentDecorator purpose is to wait until Element#present? returns true.

@jarib
Copy link
Member

jarib commented Aug 8, 2013

Anyway, it's not a big deal. I don't think we lose anything (apart from support for Ruby 1.8, which I don't think we care about anyway) by inheriting from BasicObject.

@kirillrdy
Copy link
Author

Yes, thats true,
however it never calls Element#present?
because WhenPresentDecorator uses method_missing
but active_support defines present? on Object which always returns true for non nil objects.

I've stated that multiple times in this issue, perhaps a chat or a solid test would show this.
sorry for taking your time.

@kirillrdy
Copy link
Author

I've added
class Object
def present?
true
end
end

to your lib/watir-webdriver.rb

No failing tests,
that means I need to provided a test that will fail.

This should not work because WhenPresentDecorator should call present? on Element not on Object

@p0deje
Copy link
Member

p0deje commented Aug 8, 2013

@kirillrdy We're just saying that calling when_present.present? doesn't make much sense, but as @jarib said, it's not a big deal so we're ok to accept the patch :)

@jarmo
Copy link
Member

jarmo commented Aug 8, 2013

Why not just delegate #present? to the @element?

require "forwardable"

class WhenPresentDecorator
  extend Forwardable
  def_delegator :@element, :present?
end

This seems much cleaner solution to this problem in my opinion than extending BasicObject plus the support for Ruby 1.8 will still be there.

@p0deje p0deje closed this in 231002d Sep 12, 2013
@p0deje
Copy link
Member

p0deje commented Sep 12, 2013

@jarmo Thanks, applied that.

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

5 participants