Code review rejects

1. Fix typo's 2. use Selenium Session 3. Fix tests
SeleniumHQ · Sep 10, 2019 · f323d60 · f323d60
1 parent 53f7bf3
commit f323d60
Show file tree

Hide file tree

Showing 12 changed files with 102 additions and 87 deletions.
diff --git a/common/src/web/devToolsProfilerTest.html b/common/src/web/devToolsProfilerTest.html
@@ -0,0 +1,19 @@
+<!doctype html>
+
+<html lang="en">
+<head>
+  <meta charset="utf-8">
+
+  <title>The HTML5 Herald</title>
+  <meta content="The HTML5 Herald" name="description">
+  <meta content="SitePoint" name="author">
+
+  <link href="css/styles.css?v=1.0" rel="stylesheet">
+
+</head>
+
+<body>
+<h1>This is profiler page - test page</h1>
+<img src="map.png">
+</body>
+</html>
diff --git a/java/client/src/org/openqa/selenium/devtools/BUILD.bazel b/java/client/src/org/openqa/selenium/devtools/BUILD.bazel
@@ -12,7 +12,7 @@ java_export(
     deps = [
         "//java/client/src/org/openqa/selenium:core",
         "//java/client/src/org/openqa/selenium/json",
-        "//java/client/src/org/openqa/selenium/remote/http",
+        "//java/client/src/org/openqa/selenium/remote",
         "//third_party/java/guava",
     ],
 )
diff --git a/java/client/src/org/openqa/selenium/devtools/Connection.java b/java/client/src/org/openqa/selenium/devtools/Connection.java
@@ -25,9 +25,9 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Multimap;
 
-import org.openqa.selenium.devtools.target.model.SessionId;
 import org.openqa.selenium.json.Json;
 import org.openqa.selenium.json.JsonInput;
+import org.openqa.selenium.remote.SessionId;
 import org.openqa.selenium.remote.http.HttpClient;
 import org.openqa.selenium.remote.http.HttpRequest;
 import org.openqa.selenium.remote.http.WebSocket;

diff --git a/java/client/src/org/openqa/selenium/devtools/DevTools.java b/java/client/src/org/openqa/selenium/devtools/DevTools.java
@@ -20,15 +20,15 @@
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 
 import org.openqa.selenium.devtools.target.Target;
-import org.openqa.selenium.devtools.target.model.SessionId;
 import org.openqa.selenium.devtools.target.model.TargetId;
 import org.openqa.selenium.devtools.target.model.TargetInfo;
+import org.openqa.selenium.remote.SessionId;
 
 import java.io.Closeable;
 import java.time.Duration;
-import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.Set;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeoutException;
@@ -70,7 +70,7 @@ public void createSessionIfThereIsNotOne() {
 
   public void createSession() {
     // Figure out the targets.
-    List<TargetInfo> infos = connection.sendAndWait(cdpSession, Target.getTargets(), timeout);
+    Set<TargetInfo> infos = connection.sendAndWait(cdpSession, Target.getTargets(), timeout);
 
     // Grab the first "page" type, and glom on to that.
     // TODO: Find out which one might be the current one
@@ -106,4 +106,8 @@ public void createSession() {
       throw new org.openqa.selenium.TimeoutException(e);
     }
   }
+
+  public SessionId getCdpSession() {
+    return cdpSession;
+  }
 }
diff --git a/java/client/src/org/openqa/selenium/devtools/profiler/model/Profile.java b/java/client/src/org/openqa/selenium/devtools/profiler/model/Profile.java
@@ -82,7 +82,7 @@ private static Profile fromJson(JsonInput input) {
           nodes = new ArrayList<>();
           input.beginArray();
           while (input.hasNext()) {
-            nodes.add(ProfileNode.fromJson(input));
+            nodes.add(input.read(ProfileNode.class));
           }
           input.endArray();
           break;

diff --git a/java/client/src/org/openqa/selenium/devtools/profiler/model/ProfileNode.java b/java/client/src/org/openqa/selenium/devtools/profiler/model/ProfileNode.java
@@ -67,7 +67,7 @@ public ProfileNode(int id, CallFrame callFrame, Integer hitCount,
     this.positionTicks = positionTicks;
   }
 
-  public static ProfileNode fromJson(JsonInput input) {
+  private static ProfileNode fromJson(JsonInput input) {
     int id = -1;
     CallFrame callFrame = null;
     Integer hitCount = null;

diff --git a/java/client/src/org/openqa/selenium/devtools/target/Target.java b/java/client/src/org/openqa/selenium/devtools/target/Target.java
@@ -27,14 +27,15 @@
 import org.openqa.selenium.devtools.target.model.DetachedFromTarget;
 import org.openqa.selenium.devtools.target.model.ReceivedMessageFromTarget;
 import org.openqa.selenium.devtools.target.model.RemoteLocation;
-import org.openqa.selenium.devtools.target.model.SessionId;
 import org.openqa.selenium.devtools.target.model.TargetCrashed;
 import org.openqa.selenium.devtools.target.model.TargetId;
 import org.openqa.selenium.devtools.target.model.TargetInfo;
+import org.openqa.selenium.remote.SessionId;
 
 import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.Set;
 
 import static org.openqa.selenium.devtools.ConverterFunctions.map;
 
@@ -73,11 +74,11 @@ public static Command<Boolean> closeTarget(TargetId targetId) {
   }
 
   /**
-   * nject object to the target's main frame that provides a communication channel with browser
+   * Inject object to the target's main frame that provides a communication channel with browser
    * target. Injected object will be available as window[bindingName]. The object has the follwing
    * API: binding.send(json) - a method to send messages over the remote debugging protocol
-   * binding.onmessage = json =&gt; handleMessage(json) - a callback that will be called for the
-   * protocol notifications and command responses.
+   * binding.onmessage equals to json then bing to handleMessage(json) - a callback that will be called for the
+   * protocol notifications and command responses. EXPERIMENTAL
    */
   @Beta
   public static Command<Void> exposeDevToolsProtocol(
@@ -121,7 +122,7 @@ public static Command<TargetId> createTarget(
       String url,
       Optional<Integer> width,
       Optional<Integer> height,
-      Optional<BrowserContextID> browserContextID,
+      Optional<BrowserContextID> browserContextId,
       Optional<Boolean> enableBeginFrameControl,
       Optional<Boolean> newWindow,
       Optional<Boolean> background) {
@@ -130,8 +131,8 @@ public static Command<TargetId> createTarget(
     params.put("url", url);
     width.ifPresent(integer -> params.put("width", integer));
     height.ifPresent(integer -> params.put("height", integer));
-    browserContextID.ifPresent(
-        browserContextId -> params.put("browserContextId", browserContextId));
+    browserContextId.ifPresent(
+        bid -> params.put("browserContextId", bid));
     enableBeginFrameControl.ifPresent(aBoolean -> params.put("enableBeginFrameControl", aBoolean));
     newWindow.ifPresent(aBoolean -> params.put("newWindow", aBoolean));
     background.ifPresent(aBoolean -> params.put("background", aBoolean));
@@ -177,11 +178,11 @@ public static Command<TargetInfo> getTargetInfo(Optional<TargetId> targetId) {
   /**
    * Retrieves a list of available targets.
    */
-  public static Command<List<TargetInfo>> getTargets() {
+  public static Command<Set<TargetInfo>> getTargets() {
     return new Command<>(
         "Target.getTargets",
         ImmutableMap.of(),
-        ConverterFunctions.map("targetInfos", new TypeToken<List<TargetInfo>>() {
+        ConverterFunctions.map("targetInfos", new TypeToken<Set<TargetInfo>>() {
         }.getType()));
   }
 
@@ -209,7 +210,6 @@ public static Command<SessionId> attachToTarget(TargetId targetId, Optional<Bool
     final ImmutableMap.Builder<String, Object> params = ImmutableMap.builder();
     params.put("targetId", targetId);
     params.put("flatten", flatten.orElse(true));
-//    flatten.ifPresent(aBoolean -> params.put("flatten", aBoolean));
     return new Command<>(
         "Target.attachToTarget",
         params.build(),
@@ -221,6 +221,7 @@ public static Command<SessionId> attachToTarget(TargetId targetId, Optional<Bool
    * this one. When turned on, attaches to all existing related targets as well. When turned off,
    * automatically detaches from all currently attached targets.EXPERIMENTAL
    */
+  @Beta
   public static Command<Void> setAutoAttach(
       Boolean autoAttach, Boolean waitForDebuggerOnStart, Optional<Boolean> flatten) {
     Objects.requireNonNull(autoAttach, "autoAttach is required");

diff --git a/java/client/src/org/openqa/selenium/devtools/target/model/AttachToTarget.java b/java/client/src/org/openqa/selenium/devtools/target/model/AttachToTarget.java
@@ -17,44 +17,42 @@
 package org.openqa.selenium.devtools.target.model;
 
 import org.openqa.selenium.json.JsonInput;
+import org.openqa.selenium.remote.SessionId;
 
 import java.util.Objects;
 
 public class AttachToTarget {
 
   private final SessionId sessionId;
 
-  private final TargetId targetId;
+  private final TargetInfo targetInfo;
 
   private final boolean waitForDebugger;
 
   public AttachToTarget(SessionId sessionId,
-                        TargetId targetId, Boolean waitForDebugger) {
+                        TargetInfo targetInfo, Boolean waitForDebugger) {
     this.sessionId = Objects.requireNonNull(sessionId, "sessionId is required");
-    this.targetId = Objects.requireNonNull(targetId, "targetId is required");
+    this.targetInfo = Objects.requireNonNull(targetInfo, "targetInfo is required");
     this.waitForDebugger = Objects.requireNonNull(waitForDebugger, "waitForDebugger is require");
   }
 
   private static AttachToTarget fromJson(JsonInput input) {
-    SessionId sessionId = null;
-    TargetId targetId = null;
+    SessionId sessionId = input.read(SessionId.class);
+    TargetInfo targetInfo = null;
     Boolean waitForDebugger = null;
     while (input.hasNext()) {
       switch (input.nextName()) {
-        case "sessionId":
-          sessionId = input.read(SessionId.class);
+        case "targetInfo":
+          targetInfo = input.read(TargetInfo.class);
           break;
-        case "targetId":
-          targetId = input.read(TargetId.class);
-          break;
-        case "waitForDebugger":
+        case "waitingForDebugger":
           waitForDebugger = input.nextBoolean();
           break;
         default:
           input.skipValue();
           break;
       }
     }
-    return new AttachToTarget(sessionId, targetId, waitForDebugger);
+    return new AttachToTarget(sessionId, targetInfo, waitForDebugger);
   }
 }
diff --git a/java/client/src/org/openqa/selenium/remote/BUILD.bazel b/java/client/src/org/openqa/selenium/remote/BUILD.bazel
@@ -140,7 +140,6 @@ java_library(
         ":capabilities",
         ":http-session-id",
         "//java/client/src/org/openqa/selenium:core",
-        "//java/client/src/org/openqa/selenium/devtools",
         "//java/client/src/org/openqa/selenium/io",
         "//java/client/src/org/openqa/selenium/json",
         "//java/client/src/org/openqa/selenium/os",

diff --git a/java/client/test/org/openqa/selenium/devtools/ChromeDevToolsInspectorTest.java b/java/client/test/org/openqa/selenium/devtools/ChromeDevToolsInspectorTest.java
@@ -16,31 +16,29 @@
 // under the License.
 package org.openqa.selenium.devtools;
 
-import static org.openqa.selenium.devtools.inspector.Inspector.detached;
-import static org.openqa.selenium.devtools.inspector.Inspector.disable;
-import static org.openqa.selenium.devtools.inspector.Inspector.enable;
-import static org.openqa.selenium.devtools.target.Target.attachToTarget;
-
 import org.junit.Assert;
 import org.junit.Test;
 import org.openqa.selenium.devtools.target.Target;
-import org.openqa.selenium.devtools.target.model.SessionId;
 import org.openqa.selenium.devtools.target.model.TargetInfo;
+import org.openqa.selenium.remote.SessionId;
 
-import java.util.List;
 import java.util.Optional;
+import java.util.Set;
+
+import static org.openqa.selenium.devtools.inspector.Inspector.detached;
+import static org.openqa.selenium.devtools.inspector.Inspector.disable;
+import static org.openqa.selenium.devtools.inspector.Inspector.enable;
+import static org.openqa.selenium.devtools.target.Target.attachToTarget;
 
 public class ChromeDevToolsInspectorTest extends DevToolsTestBase {
   @Test
   public void inspectDetached() {
     devTools.addListener(detached(), Assert::assertNotNull);
     devTools.send(enable());
-    List<TargetInfo> targetInfos = devTools.send(Target.getTargets());
-    targetInfos.stream()
-        .forEach(
+    Set<TargetInfo> targetInfos = devTools.send(Target.getTargets());
+    targetInfos.forEach(
             targetInfo -> {
-              SessionId sessionId =
-                  devTools.send(attachToTarget(targetInfo.getTargetId(), Optional.of(false)));
+              SessionId sessionId = devTools.send(attachToTarget(targetInfo.getTargetId(), Optional.of(false)));
               devTools.send(
                   Target.sendMessageToTarget(
                       "{\"method\":\"Page.crash\"}",

diff --git a/java/client/test/org/openqa/selenium/devtools/ChromeDevToolsProfilerTest.java b/java/client/test/org/openqa/selenium/devtools/ChromeDevToolsProfilerTest.java
@@ -41,21 +41,8 @@
 import java.util.List;
 import java.util.Optional;
 
-
 public class ChromeDevToolsProfilerTest extends DevToolsTestBase {
 
-
-
-  @Test
-  public void aSimpleStartStopAndGetProfilerTest() {
-    devTools.send(enable());
-    devTools.send(start());
-    Profile profiler = devTools.send(stop());
-    validateProfile(profiler);
-    devTools.send(disable());
-
-  }
-
   private void validateProfile(Profile profiler) {
     assertNotNull(profiler);
     assertNotNull(profiler.getNodes());
@@ -74,7 +61,7 @@ private void validateProfile(Profile profiler) {
   @Test
   public void sampleGetBestEffortProfilerTest() {
     devTools.send(enable());
-    driver.get(appServer.whereIs("simpleTest.html"));
+    driver.get(appServer.whereIs("devToolsProfilerTest.html"));
     devTools.send(setSamplingInterval(30));
     List<ScriptCoverage> bestEffort = devTools.send(getBestEffortCoverage());
     assertNotNull(bestEffort);
@@ -85,7 +72,7 @@ public void sampleGetBestEffortProfilerTest() {
   @Test
   public void sampleSetStartPreciseCoverageTest() {
     devTools.send(enable());
-    driver.get(appServer.whereIs("simpleTest.html"));
+    driver.get(appServer.whereIs("devToolsProfilerTest.html"));
     devTools.send(startPreciseCoverage(Optional.of(true), Optional.of(true)));
     devTools.send(start());
     List<ScriptCoverage> pc = devTools.send(takePreciseCoverage());
@@ -95,20 +82,20 @@ public void sampleSetStartPreciseCoverageTest() {
     devTools.send(disable());
   }
 
-
   @Test
   public void sampleProfileEvents() {
+    devTools.addListener(consoleProfileFinished(), Assert::assertNotNull);
+    devTools.addListener(consoleProfileStarted(), Assert::assertNotNull);
     devTools.send(enable());
-    driver.get(appServer.whereIs("simpleTest.html"));
+    driver.get(appServer.whereIs("devToolsProfilerTest.html"));
     devTools.addListener(consoleProfileStarted(), Assert::assertNotNull);
     devTools.send(startTypeProfile());
     devTools.send(start());
     driver.navigate().refresh();
-    devTools.addListener(consoleProfileFinished(), Assert::assertNotNull);
+
     devTools.send(stopTypeProfile());
     Profile profiler = devTools.send(stop());
     validateProfile(profiler);
     devTools.send(disable());
   }
-
 }