#655 FIX by TikhomirovSergey · Pull Request #682

#655 FIX by TikhomirovSergey · Pull Request #682 · appium/java-client

Change list

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

io.appium.java_client.remote.AppiumCommandExecutor was rolled back.

It is breaking change because it is compatible with appium server > v1.6.5

		public AppiumCommandExecutor(Map<String, CommandInfo> additionalCommands,
		URL addressOfRemoteServer, HttpClient.Factory httpClientFactory) {
		super(additionalCommands, addressOfRemoteServer, httpClientFactory);
		service = null;

Choose a reason for hiding this comment

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

can this be a reason of unexpected NPE?

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

then it might be useful to mark the property as Nullable

Choose a reason for hiding this comment

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

or make it optional (I'd prefer this option)

Choose a reason for hiding this comment

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

How about having some unit test to verify resulting capabilities structure?

@mykola-mokhnach

How about having some unit test to verify resulting capabilities structure?

I think we don't need for these tests:

as you can see public class AppiumCommandExecutor extends HttpCommandExecutor and

 @Override public Response execute(Command command) throws IOException, WebDriverException {
        ...
      return super.execute(command);
       ...
   }

The capability forming is fully inherited from Selenium
HttpCommandExecutor ProtocolHandshake

Our tests which run some AppiumDriver are successful.

Choose a reason for hiding this comment

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

LGTM

		response.setSessionId(command.getSessionId().toString());
		@Override public Response execute(Command command) throws WebDriverException {
		if (DriverCommand.NEW_SESSION.equals(command.getName())) {
		ofNullable(service).ifPresent(driverService -> {

Choose a reason for hiding this comment

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

I assume the code will be simpler if we make the service variable itself of type Optional

		}

		return new WebDriverException("The appium server has accidentally died!", rootCause);
		}).orElse(new WebDriverException(rootCause.getMessage(), rootCause));

Choose a reason for hiding this comment

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

probably, orElseGet ?