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

selenium upgraded to 3.8.1, guava upgraded to 23.6-jre #209

Merged
merged 12 commits into from
Jan 30, 2018

Conversation

joannajasnowska
Copy link
Contributor

@joannajasnowska joannajasnowska commented Jan 23, 2018

Description

I've upgraded Selenium version to 3.8.1 and additionally Guava to 23.6-jre.

Motivation and Context

This upgrade is required for further AET's migration to newest versions of Chrome (and potentially Firefox) browsers.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

I hereby agree to the terms of the AET Contributor License Agreement.

@wiiitek
Copy link
Contributor

wiiitek commented Jan 23, 2018

This pull request require update for AET cookbook (folder for Firefox log)

@@ -35,6 +35,11 @@ public boolean apply(DBKey dbKey) {
return companyMatches(dbKey) && projectMatches(dbKey);
}

@Override
Copy link
Contributor

Choose a reason for hiding this comment

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

WE could use default implementation from the Predicate interface providing we change the JAVA level to 1.8 in root pom XML file: dbecf17

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, fixed

@@ -129,23 +135,17 @@ private FirefoxProfile getFirefoxProfile() throws IOException {
return firefoxProfile;
}

private AetFirefoxDriver getFirefoxDriver(FirefoxProfile fp, DesiredCapabilities capabilities)
private RemoteWebDriver getFirefoxDriver(DesiredCapabilities capabilities)
Copy link
Contributor

Choose a reason for hiding this comment

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

We are declaring RemoteWebDriver, but are returning FirefoxWebDriver.
Could we return WebDriver interface here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

requestExecutorFactory
.createInstance());
} catch (Exception e) {
throw new WorkerException(e.getMessage(), e);
}
}

private void setCommonCapabilities(DesiredCapabilities capabilities, FirefoxProfile fp) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we move this private method down after the two public methods that use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved

import org.openqa.selenium.firefox.FirefoxProfile;
import org.openqa.selenium.remote.CapabilityType;
import org.openqa.selenium.remote.DesiredCapabilities;
import org.openqa.selenium.remote.RemoteWebDriver;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like unused import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, removed.

@@ -67,7 +67,7 @@
<dependency>
<groupId>org.seleniumhq.selenium</groupId>
<artifactId>selenium-htmlunit-driver</artifactId>
<version>${selenium.version}</version>
<version>2.52.0</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

I've seen that selenium htmlunit package was separated from the selenium itself some time ago. And I believe this version has connection with all ignored unit tests?
Is there any way to make those ignored unit tests working? If not, maybe we should just remove them?
Or maybe we should add ignore comment like:
@Ignore("Not working because of selenium-htmlunit-driver broken compatibility. Consider to remove in the future.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are irrespectively working on fixing or even rewriting failing tests. Probably soon there will be additional pull request for junit changes.

@malaskowski
Copy link
Contributor

Could you please update PR description and/or remove unnecessary sections?
I believe it will help us in the future recognize decisions that were behind changes introduced in every PR.

dzuma
dzuma previously approved these changes Jan 25, 2018
@dzuma dzuma dismissed their stale review January 25, 2018 08:58

Not tested on vagrant

@@ -61,7 +61,7 @@
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
<version>15.0</version>
<version>23.6-jre</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's upgrade guava also in aet-features.xml file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/osgi-dependencies/aet-features.xml is recently added to gitignore. Is this expected? That's why my change is not in this PR.

@wiiitek wiiitek merged commit 421b70a into master Jan 30, 2018
@wiiitek wiiitek deleted the selenium-upgrade branch January 30, 2018 10:11
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.

4 participants