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

Allow missing elements to be inspected #187

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tlconnor
Copy link
Contributor

@tlconnor tlconnor commented Sep 14, 2016

When assert_predicate assertions fail Minitest will inspect the target object as part of the error message generation.

Currently AePageObjects will pass the inspect call on to the Element which will attempt to load the DOM element and will result in AePageObjects::LoadingElementFailed being thrown.

This PR allow inspect to be called on ElementProxy objects where the DOM element is absent without throwing an error.

@@ -103,6 +103,14 @@ def method_missing(name, *args, &block)
end

implicit_element.__send__(name, *args, &block)
rescue AePageObjects::LoadingElementFailed
raise unless %w(to_s inspect).include? name.to_sym
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I included both to_s and inspect here because I figured that both methods would be better off returning a string rather than raising an error. I could be convinced otherwise.

@rmacklin
Copy link
Contributor

Can you add tests? I tried to reproduce this in an existing selenium test for a simple project and wasn't been able to, so I think there is more to it than what was described above.

@tlconnor tlconnor force-pushed the allowMissingElementsToBeInspected branch from 89a55e4 to eb7072f Compare September 14, 2016 17:14
@tlconnor
Copy link
Contributor Author

tlconnor commented Sep 14, 2016

@rmacklin you are right, the root cause of the flakiness was something else. The fix I've made is still valid as the failing inspect call would hide the real error.

I've added unit tests for the fix.

@rmacklin
Copy link
Contributor

rmacklin commented Sep 14, 2016

I'm still not totally clear on the root issue here. But if this is limited to Minitest::Assertions#assert_predicate assertions, did you see this comment in the file you linked in the original PR description (before you edited it)? Seems like overriding that method in a minitest extensions module would be smaller in scope than globally changing how inspect and to_s behave, which is technically a breaking change.

@dtognazzini
Copy link
Contributor

Seems like a pretty good improvement. I think things would become even cleaner if we collapsed ElementProxy into Element altogether: #127 (comment)

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