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

[WebDriver] seeLink compares relative links correctly #4182

Merged
merged 3 commits into from May 8, 2017

Conversation

Projects
None yet
2 participants
@Naktibalda
Member

Naktibalda commented Apr 30, 2017

Fixes #4107

@DavertMik

Thanks for the fix, but I'd move getAbsoluteUrlFor to Util.
WebDriver class is already overblown by various methods :(

Show outdated Hide outdated src/Codeception/Module/WebDriver.php
* @param string $uri the absolute or relative URI
* @return string the absolute URL
*/
private function getAbsoluteUrlFor($uri)

This comment has been minimized.

@DavertMik

DavertMik Apr 30, 2017

Member

Let's move it to Codeception\Util\Uri

@DavertMik

DavertMik Apr 30, 2017

Member

Let's move it to Codeception\Util\Uri

This comment has been minimized.

@Naktibalda

Naktibalda Apr 30, 2017

Member

The last line calls a method of Codeception\Util\Uri.
The first 2 lines are WebDriver specific. (It is funny that Selenium always returns an absolute url for href, but it can return a relative url for current uri).
The check for empty url should be moved to Uri::mergeUrls, but I have copied it from InnerBrowser and was too lazy to refactor/didn't want to break anything else.

There are similar methods in Guzzle and Guzzle6 connector too.

@Naktibalda

Naktibalda Apr 30, 2017

Member

The last line calls a method of Codeception\Util\Uri.
The first 2 lines are WebDriver specific. (It is funny that Selenium always returns an absolute url for href, but it can return a relative url for current uri).
The check for empty url should be moved to Uri::mergeUrls, but I have copied it from InnerBrowser and was too lazy to refactor/didn't want to break anything else.

There are similar methods in Guzzle and Guzzle6 connector too.

This comment has been minimized.

@Naktibalda

Naktibalda May 1, 2017

Member

I will restructure this code because I don't like that _getCurrentUri makes HTTP​ request to Selenium on every call to getAbsoluteUrlFor.

I will introduce a private method filterNodesByHref instead to reduce code duplication in seeLink and dontSeeLink methods.

@Naktibalda

Naktibalda May 1, 2017

Member

I will restructure this code because I don't like that _getCurrentUri makes HTTP​ request to Selenium on every call to getAbsoluteUrlFor.

I will introduce a private method filterNodesByHref instead to reduce code duplication in seeLink and dontSeeLink methods.

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik May 8, 2017

Member

Good. Thank you

Member

DavertMik commented May 8, 2017

Good. Thank you

@DavertMik DavertMik merged commit 7505f57 into Codeception:2.2 May 8, 2017

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details
wercker/build Wercker pipeline passed
Details

chris1312 added a commit to chris1312/Codeception that referenced this pull request Jun 16, 2017

[WebDriver] seeLink compares relative links correctly (#4182)
* [WebDriver] seeLink compares relative links correctly

Fixes #4107

* [WebDriver] removed code copied from InnerBrowser

* Extracted common code of seeLink and dontSeeLink to filterNodesByHref

@Naktibalda Naktibalda deleted the Naktibalda:webdriver-see-link-relative-link branch Oct 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment