#875 FIX#894
Conversation
|
@Tenit2012 @Trucnt you can check out these changes too. |
| int threshold = (int) Math.min(Runtime.getRuntime().freeMemory() / 10, Integer.MAX_VALUE); | ||
| FileBackedOutputStream os = new FileBackedOutputStream(threshold); | ||
| try ( | ||
| CountingOutputStream counter = new CountingOutputStream(os); |
There was a problem hiding this comment.
the formatting here is a bit confusing
| return getPrivateFieldValue("client", HttpClient.class); | ||
| } | ||
|
|
||
| private Response createSession(Command command) throws IOException { |
There was a problem hiding this comment.
it would be handy to add some documentation on this method
| Response response; | ||
| try { | ||
| response = super.execute(command); | ||
| if (!NEW_SESSION.equals(command.getName())) { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Codacy does not appreciate default access modifiers
| @@ -54,11 +60,17 @@ public class DesktopBrowserCompatibilityTest { | ||
| * The starting. | ||
| */ | ||
| @BeforeClass public static void beforeClass() { |
There was a problem hiding this comment.
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()) { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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()) { |
There was a problem hiding this comment.
can this be done with forEach?
|
@mykola-mokhnach will make required improvements soon. |
|
@mykola-mokhnach There are updates |
|
|
||
| return result.map(result1 -> { | ||
| Result toReturn = result.get(); | ||
| System.out.print(format("Detected dialect: %s", toReturn.getDialect())); |
There was a problem hiding this comment.
cannot we use logger object instead of System.print?
| .createSession(getClient(), command); | ||
| Dialect dialect = result.getDialect(); | ||
| setCommandCodec(dialect.getCommandCodec()); | ||
| getAdditionalCommands().forEach(this::defineCommand); |
|
|
||
| @Override | ||
| public Response execute(Command command) throws WebDriverException { | ||
| public Response execute(Command command) throws WebDriverException, IOException { |
There was a problem hiding this comment.
Is it a good idea to make it throwing the checked exception? Id rather wrap IOException into WebDriverException and keep the previous method signature
|
@mykola-mokhnach There new updates. Sorry for the delaying. I was busy. |
mykola-mokhnach
left a comment
There was a problem hiding this comment.
LGTM, there is only one small thing left about missing docstring
SrinivasanTarget
left a comment
There was a problem hiding this comment.
Looks good on iOS too
Change list
Types of changes
Details
It seems it is needed to propose some changes to Selenium. Make some methods protected, for instance.