Home Original page

A proposed fix for issue 276 (https://github.com/appium/java-client/i… by baechul · Pull Request #278 · appium/java-client

@baechul
Ok. I like this PR 👍 I think it is even better than the passing user's CommandExecutor through constructors because it could be incompatible with whole Appium ecosystem (with client or/and server).

But I have some remarks. My opinion is that proposed changes look bit incomplete. It would be better if you redesigned AppiumCommandExecutor and AppiumDriver the way below :

...
import org.openqa.selenium.remote.internal.ApacheHttpClient;
...

public class AppiumCommandExecutor extends HttpCommandExecutor{

    private final DriverService service;

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

    public AppiumCommandExecutor(Map<String, CommandInfo> additionalCommands, 
                                 DriverService service,
                                 HttpClient.Factory httpClientFactory) {
        super(additionalCommands, service.getUrl(), httpClientFactory);
        this.service = service;
    }

    public AppiumCommandExecutor(Map<String, CommandInfo> additionalCommands, 
                                 URL addressOfRemoteServer) {
        this(additionalCommands, addressOfRemoteServer, new ApacheHttpClient.Factory());
    }

    public AppiumCommandExecutor(Map<String, CommandInfo> additionalCommands, 
                                 DriverService service) {
        this(additionalCommands, service, new ApacheHttpClient.Factory());
    }
...

and

...
i@SuppressWarnings("unchecked")
public abstract class AppiumDriver<RequiredElementType extends WebElement> extends DefaultGenericMobileDriver<RequiredElementType> {
...

    private AppiumDriver(HttpCommandExecutor executor, Capabilities capabilities){
        super(executor, capabilities);
        this.executeMethod = new AppiumExecutionMethod(this);
        locationContext = new RemoteLocationContext(executeMethod);
        super.setErrorHandler(errorHandler);
        this.remoteAddress = executor.getAddressOfRemoteServer();
    }



    public AppiumDriver(URL remoteAddress, Capabilities desiredCapabilities) {
        this(new AppiumCommandExecutor(
                getMobileCommands(), remoteAddress), desiredCapabilities);
    }

    public AppiumDriver(URL remoteAddress, HttpClient.Factory httpClientFactory, Capabilities desiredCapabilities) {
        this(new AppiumCommandExecutor(
                        getMobileCommands(), remoteAddress, httpClientFactory), desiredCapabilities);
    }


    public AppiumDriver(AppiumDriverLocalService service, Capabilities desiredCapabilities) {
        this(new AppiumCommandExecutor(
                getMobileCommands(), service), desiredCapabilities);
    }


    public AppiumDriver(AppiumDriverLocalService service, HttpClient.Factory httpClientFactory, Capabilities desiredCapabilities) {
        this(new AppiumCommandExecutor(
                getMobileCommands(), service, httpClientFactory), desiredCapabilities);
    }


    public AppiumDriver(AppiumServiceBuilder builder, Capabilities desiredCapabilities) {
        this(builder.build(), desiredCapabilities);
    }

    public AppiumDriver(AppiumServiceBuilder builder, HttpClient.Factory httpClientFactory, Capabilities desiredCapabilities) {
        this(builder.build(), httpClientFactory, desiredCapabilities);
    }

    public AppiumDriver(Capabilities desiredCapabilities) {
        this(AppiumDriverLocalService.buildDefaultService(), desiredCapabilities);
    }

    public AppiumDriver(HttpClient.Factory httpClientFactory, Capabilities desiredCapabilities) {
        this(AppiumDriverLocalService.buildDefaultService(), httpClientFactory, desiredCapabilities);
    }

...

and add new public constructors to AndroidDriver/IOSDriver.

I just think that other users would like to use the new capability (you probably too) combined with the AppiumDriverLocalService.

I was managed to make this improvement locally. It works!

If you improve your PR the way above then there won't be necessity to cover the new capability with tests and the only way to shoot user's leg will be the implementation of a bad httpClientFactory (and only a user will be responsible) :)

@Jonahss @bootstraponline
Please look at this PR.