Dev tool profiler #7249
Dev tool profiler #7249
Conversation
Major : + Adding to DevTools Profiler Minor : + Adding extractLong and extractInstant to JsonInputConverter
+ Remove JsonInputConverter with JsonInput Methods + Add nextInstant at JsonInput + Fix some Merge issues + Fix Test + Test imports
There was a problem hiding this comment.
This is a great start. Rather than adding loads of comments, I thought it best to outline some general points:
- Prefer immutable classes --- fields should be final, and there should be no setters as a rule of thumb.
- Check class invariants in the constructor --- generally this means "make sure all non-optional args are not null"
- It's best to implement
hashCodeandequalsrather than just one or the other.
Once those changes are made, I think this is ready to land in the tree. :)
java/client/src/org/openqa/selenium/devtools/profiler/Profiler.java
Outdated
Show resolved
Hide resolved
java/client/src/org/openqa/selenium/devtools/profiler/Profiler.java
Outdated
Show resolved
Hide resolved
java/client/src/org/openqa/selenium/devtools/profiler/model/ConsoleProfileFinished.java
Outdated
Show resolved
Hide resolved
java/client/src/org/openqa/selenium/devtools/profiler/model/ConsoleProfileFinished.java
Outdated
Show resolved
Hide resolved
java/client/src/org/openqa/selenium/devtools/profiler/model/ConsoleProfileFinished.java
Outdated
Show resolved
Hide resolved
java/client/src/org/openqa/selenium/devtools/profiler/model/ConsoleProfileFinished.java
Outdated
Show resolved
Hide resolved
java/client/src/org/openqa/selenium/devtools/profiler/model/PositionTickInfo.java
Show resolved
Hide resolved
| while (input.hasNext()) { | ||
| switch (input.nextName()) { | ||
| case "location": | ||
| location = Location.fromJson(input); |
There was a problem hiding this comment.
input.read(Location.class) will work. Please add a TODO, since we can tidy this up once the code has landed.
There was a problem hiding this comment.
One of the nice features of selenium's json library is that if a class has any fromJson method, it'll try and do the Right Thing.
There was a problem hiding this comment.
I will do it for next time
| /** | ||
| * Whether coverage data for this function has block granularity. | ||
| */ | ||
| private final Boolean isBlockCoverage; |
There was a problem hiding this comment.
boolean perhaps? JS would default this to undefined which is a falsey value.
| functionName = input.nextString(); | ||
| break; | ||
| case "ranges": | ||
| ranges = new ArrayList<>(); |
There was a problem hiding this comment.
input.read(new TypeToken<List<CoverageRange>>(){}.getType())
| public boolean equals(Object obj) { | ||
| Objects.requireNonNull(obj, "obj is mandatory for equals method"); | ||
|
|
||
| return this.getLine() == ((PositionTickInfo) obj).getLine() |
There was a problem hiding this comment.
This will throw a ClassCastException if obj is not a PositionTickInfo. Prefer:
if (!(obj instanceof PositionTickInfo)) {
return false;
}This handles the null case (and it's always legal to pass in null to equals), as well as the case where obj isn't the right type.
|
|
||
| @Override | ||
| public boolean equals(Object obj) { | ||
| Objects.requireNonNull(obj, "obj is mandatory for equals method"); |
There was a problem hiding this comment.
The contract for equals allows null to be passed in.
| endTime = input.nextInstant(); | ||
| break; | ||
| case "samples": | ||
| samples = new ArrayList<>(); |
There was a problem hiding this comment.
samples = input.read(new TypeToken<List<Integer>>(){}.getType()) is more idiomatic.
| } | ||
|
|
||
| private void validateProfile(Profile profiler) { | ||
| Assert.assertNotNull(profiler); |
There was a problem hiding this comment.
Typically junit tests use static imports for Assert (and the assertion methods from assertj)
Hi I have Added PR for DevTools Profiler .
this tools can add better info on script behavior and Test coverage,
Xin the preceding checkbox, I verify that I have signed the Contributor License Agreement