Skip to content

Add a Selenium migration module#77

Open
troglodyne wants to merge 2 commits intoteodesian:mainfrom
troglodyne:selenium_shim
Open

Add a Selenium migration module#77
troglodyne wants to merge 2 commits intoteodesian:mainfrom
troglodyne:selenium_shim

Conversation

@troglodyne
Copy link
Copy Markdown

case #76: It occurred to me that it would be helpful to have something which can replace Selenium::Remote::Driver usage without having to rewrite a bunch of existing tests. The added code here seeks to acccomplish this.

case teodesian#76: It occurred to me that it would be helpful to have
something which can replace Selenium::Remote::Driver usage
without having to rewrite a bunch of existing tests. The added code
here seeks to acccomplish this.
Comment thread lib/Playwright/SeleniumMigrator/Driver.pm Outdated
Comment thread lib/Playwright/SeleniumMigrator/Driver.pm Outdated
Comment thread lib/Playwright/SeleniumMigrator/Driver.pm Outdated
sub new ( $class, %opts ) {
_ensure_playwright();

my $browser_name = delete $opts{browser_name} // 'firefox';
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

seems silly to do all this assignment when we just bless underscored versions of the opt keys later

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yea, kind of went off the cob here

firefox_profile pageLoadStrategy error_handler
webelement_class ua proxy debug}) {
if (exists $opts{$ignored}) {
carp "Playwright::SeleniumMigrator: ignoring Selenium option '$ignored'"
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Probably should print $class here

Comment thread lib/Playwright/SeleniumMigrator/Driver.pm Outdated
Comment thread lib/Playwright/SeleniumMigrator/Driver.pm
Comment thread lib/Playwright/SeleniumMigrator/Driver.pm Outdated
Comment thread lib/Playwright/SeleniumMigrator/Driver.pm Outdated
Comment thread lib/Playwright/SeleniumMigrator/Driver.pm
Comment thread lib/Playwright/SeleniumMigrator/Driver.pm Outdated
Comment thread lib/Playwright/SeleniumMigrator/Driver.pm Outdated
Comment thread lib/Playwright/SeleniumMigrator/Driver.pm Outdated
return bless {
_driver => $opts{_driver},
_element => $opts{_element},
_id => $opts{_id} // _generate_id(),
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Anything creating WEs should already have a PW element with a GUID that we can pass as the _id

As such we can just die rather than providing a useless guess

# avoids ElementHandle disposal issues with type().
$eh->click();

for my $key_string (@keys) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Seems like a lot of work when you could map the keys to their unicode counterparts, join and fill()

Copy link
Copy Markdown
Owner

@teodesian teodesian left a comment

Choose a reason for hiding this comment

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

Approach seems generally sound, most of the nits to pick here are QoL


sub get_element_location ($self) {
my $box = $self->{_element}->boundingBox();
return {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

just return $box

Comment thread lib/Playwright/SeleniumMigrator/WebElement.pm Outdated
@troglodyne
Copy link
Copy Markdown
Author

Pushed commit with a bit of cleanup based on commentary; comrade toboritsky seemed to pick up on the PR comments mostly like I'd hoped.

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.

2 participants