#875 FIX by TikhomirovSergey · Pull Request #894 · appium/java-client

#875 FIX by TikhomirovSergey · Pull Request #894 · appium/java-client

Change list

update to selenium 3.12
java-client's NewSessionPayload.class conflict with selenium's NewSessionPayload.class #875 FIX

Types of changes

No changes in production code.
Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Details

It seems it is needed to propose some changes to Selenium. Make some methods protected, for instance.

		int threshold = (int) Math.min(Runtime.getRuntime().freeMemory() / 10, Integer.MAX_VALUE);
		FileBackedOutputStream os = new FileBackedOutputStream(threshold);
		try (
		CountingOutputStream counter = new CountingOutputStream(os);

Choose a reason for hiding this comment

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

the formatting here is a bit confusing

		return getPrivateFieldValue("client", HttpClient.class);
		}

		private Response createSession(Command command) throws IOException {

Choose a reason for hiding this comment

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

it would be handy to add some documentation on this method

		Response response;
		try {
		response = super.execute(command);
		if (!NEW_SESSION.equals(command.getName())) {

Choose a reason for hiding this comment

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

I'd rather use ternary operator:

response = NEW_SESSION.equals(command.getName()) ? createSession(command) : super.execute(command)



		public class NewSessionPayload implements Closeable {
		class NewAppiumSessionPayload implements Closeable {

Choose a reason for hiding this comment

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

Codacy does not appreciate default access modifiers

		@@ -54,11 +60,17 @@ public class DesktopBrowserCompatibilityTest {
		* The starting.
		*/
		@BeforeClass public static void beforeClass() {

Choose a reason for hiding this comment

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

we could keep both tests for Firefox and Chrome drivers

		Optional<Result> result = (Optional<Result>) createSessionMethod
		.invoke(this, client, contentStream, counter.getCount());

		if (result.isPresent()) {

Choose a reason for hiding this comment

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

return result.map(x -> { .... }).orElseThrow(() -> new SessionNotCreatedException(...))

		Capabilities desired = (Capabilities) command.getParameters().get("desiredCapabilities");
		desired = desired == null ? new ImmutableCapabilities() : desired;

		int threshold = (int) Math.min(Runtime.getRuntime().freeMemory() / 10, Integer.MAX_VALUE);

Choose a reason for hiding this comment

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

some magic numbers explanation might be handy here

		.createSession(getClient(), command);
		Dialect dialect = result.getDialect();
		setCommandCodec(dialect.getCommandCodec());
		for (Map.Entry<String, CommandInfo> entry : getAdditionalCommands().entrySet()) {

Choose a reason for hiding this comment

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

can this be done with forEach?


		return result.map(result1 -> {
		Result toReturn = result.get();
		System.out.print(format("Detected dialect: %s", toReturn.getDialect()));

Choose a reason for hiding this comment

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

cannot we use logger object instead of System.print?

		.createSession(getClient(), command);
		Dialect dialect = result.getDialect();
		setCommandCodec(dialect.getCommandCodec());
		getAdditionalCommands().forEach(this::defineCommand);

Choose a reason for hiding this comment

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

👍


		@Override
		public Response execute(Command command) throws WebDriverException {
		public Response execute(Command command) throws WebDriverException, IOException {

Choose a reason for hiding this comment

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

Is it a good idea to make it throwing the checked exception? Id rather wrap IOException into WebDriverException and keep the previous method signature

Choose a reason for hiding this comment

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

LGTM, there is only one small thing left about missing docstring

Choose a reason for hiding this comment

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

Looks good on iOS too