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
'find_element'-like functions in Test::Selenium::Remote::Driver should support a locator strategy as an argument #179
Conversation
Sure, this makes a good amount of sense. Other language bindings do things like Are you planning to support both the two and three argument versions of find_element_ok, or just break the previous usage? |
I guess I'll go for the simpler one 😊 |
Actually this could be combined with solving the croaking issues we have, if you're amenable. For example, sub find_element_by_id {
my ($self, $locator) = @_;
my $finder = 'id';
return try {
return $self->find_element($locator, $finder);
}
catch {
carp 'hope you notice we didn\'t find an element!'
return 0;
# or we could even try
return Selenium::Remote::Null->new;
};
} I think we should be able to generate appropriate functions based on our list of FINDERS and And then we could propagate them similarly into Test::Driver.pm ? |
So, your idea here is to introduce a new non-croaking family of functions ? |
Fair enough, don't worry about it then :) |
👍 |
So I have something working so far, but it might need some refactoring (https://github.com/gempesaw/Selenium-Remote-Driver/tree/fix-179-locator-strategy-in-T-S-R-D) :
|
Another thing, I come to think that using positional parameters everywhere is kind of a PITA, mainly when we want to add new parameters (like here, adding the finding strategy). |
OK I guess I am done, no need to update the mocks, the tests were put in t/Test-Selenium-Remote-Driver.t using the other mocking technique (adding a sub that returns an expected response). |
OK done this time ! Waiting for approval :) |
@@ -5,22 +5,84 @@ use Test::Selenium::Remote::Driver; | |||
use Selenium::Remote::WebElement; | |||
use Selenium::Remote::Mock::Commands; | |||
use Selenium::Remote::Mock::RemoteConnection; | |||
use DDP; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicking, pls remove :D
Done, thanks 👍 |
'find_child_element' => 3, | ||
'find_child_elements' => 3, | ||
'find_element' => 2, | ||
'find_elements' => 2, | ||
'compare_elements' => 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah wow I see what you mean about complicated. yeah the named parameter version seems more and more attractive. this is good for now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah :)
We could try and migrate T::S::R::D on a named parameter version, since this class seems to be used by few. Something to add on the roadmap at some point ?
Hmm is it me or are some Travis jobs that are failing with no good reasons at all ? |
if ($method eq 'find_no_element') { | ||
$real_method = $method; | ||
$method = 'find_element'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I can't see where we're inverting the return in the case that it's find_no_element. I understand that we need to actually be asking for Oh is 0 a successful return and 1 is error-ful? fair enoughfind_element
, since there's no find_no_element
on Driver.pm, but I must be missing where we say "find_element found nothing, that means we pass" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's it. I hacked this a bit quickly, I guess there's room for improvement for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah i'm not concerned about the "quickly" nature, I was just super confused as to how it was working :P
Yes the travisCI is not always helpful - basically it just checks that the Linux mocks work for the tests, but it doesn't actually run the tests against a live webdriver server. I've been meaning to switch that over, actually - download webdriver and do a |
…nctions, added some tests
…-R-D 'find_element'-like functions in Test::Selenium::Remote::Driver should support a locator strategy as an argument
[NEW FEATURES] - #178 Fix upload_file's return value & add test suite - #179 Add optional middle finder-strategy argument to T:S:R:D's find_element-like functions - #174 Add new endpoints: cache status, geolocation, log types, orientation, etc [BUG FIXES] - #175 Fix find_elements - #180 Stop overwriting body_text_unlike test descriptions - #141 Explicitly cast height and weight to integers for set_window_size
Thanks! Now on cpan as v0.23 |
I raise this issue mainly because I found myself writing a lot lately :
If we could pass the locator strategy as an argument, it would allow one to write :
There might be some issues doing this, but overall it would improve the readability of code written with T::S::R::D.