Introduce microseconds and get rid of javax annotations#299
Conversation
# Conflicts: # gradle.properties # src/main/java/com/epam/reportportal/service/LoggingContext.java
…avax.annotation to jakarta.annotation in LoggingSubscriber and keep version 5.4.0-SNAPSHOT
# Conflicts: # gradle.properties
# Conflicts: # src/main/java/com/epam/reportportal/service/LaunchImpl.java
# Conflicts: # gradle.properties
WalkthroughUpgrade Java toolchain and dependencies, migrate javax.annotation to jakarta.annotation, add microsecond-aware timestamp support (serializers/deserializers and propagation), introduce many WS DTOs/models and ApiInfo, adjust Launch/step/log flows to use Comparable timestamps, improve resource handling and refactor tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Caller / Tests
participant RP as ReportPortal facade
participant Launch as LaunchImpl
participant API as ReportPortalClient
participant Server as ReportPortal Server
Client->>RP: startLaunch(StartLaunchRQ)
RP->>Launch: prepare start params (include startTime)
alt server capability unknown
Launch->>API: GET /info
API-->>Launch: ApiInfo
Launch->>Launch: useMicroseconds() (semantic version compare)
end
Launch->>Launch: convertIfNecessary(startTime)
Launch->>Server: POST /launch (startTime formatted by TimeSerializer)
Client->>RP: startTestItem(StartTestItemRQ)
RP->>Launch: decide useMicroseconds(), assign dateTime
Launch->>Server: POST /item (startTime formatted accordingly)
Client->>RP: log(SaveLogRQ)
RP->>Launch: decide useMicroseconds(), set logTime
Launch->>Server: POST /log (logTime formatted accordingly)
Client->>RP: finish(FinishTestItemRQ / FinishExecutionRQ)
RP->>Launch: decide useMicroseconds(), convertIfNecessary(endTime)
Launch->>Server: PUT /.../finish (endTime formatted accordingly)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (14)
src/main/java/com/epam/reportportal/service/tree/TestItemTree.java (2)
259-262: Update annotation usage to match chosen libraryIf switching to org.jetbrains, rename @nonnull to @NotNull here (and elsewhere).
- @Nonnull + @NotNull public Map<ItemTreeKey, TestItemLeaf> getChildItems() { return childItems; }
231-235: Constructor chaining calls wrong overload
this(itemId, parentId, attributes);doesn’t match any constructor; argument order should be(parentId, itemId, attributes).- this(itemId, parentId, attributes); + this(parentId, itemId, attributes); this.childItems.putAll(childItems);src/main/java/com/epam/reportportal/service/launch/lock/LaunchIdLockSocket.java (2)
95-101: Socket read may return partial data; use readFully.Single
InputStream.read(byte[])is not guaranteed to fill the buffer; this can corrupt protocol parsing under load.Apply:
- InputStream is = s.getInputStream(); - //noinspection ResultOfMethodCallIgnored - is.read(updateUuid); + DataInputStream is = new DataInputStream(s.getInputStream()); + is.readFully(updateUuid);And add the missing import (see separate import diff).
153-171: Client-side reads also assume full buffers; add readFully and a read timeout.Protect the client against partial reads and hung peers; also improve the warning to show expected vs actual.
- try (Socket socket = new Socket(InetAddress.getLocalHost(), portNumber)) { - byte[] launchAnswerBuffer = new byte[instanceUuid.getBytes(TRANSFER_CHARSET).length]; - InputStream is = socket.getInputStream(); - //noinspection ResultOfMethodCallIgnored - is.read(launchAnswerBuffer); + try (Socket socket = new Socket(InetAddress.getLocalHost(), portNumber)) { + socket.setSoTimeout((int) Math.min(instanceWaitTimeout, Integer.MAX_VALUE)); + byte[] launchAnswerBuffer = new byte[instanceUuid.getBytes(TRANSFER_CHARSET).length]; + DataInputStream is = new DataInputStream(socket.getInputStream()); + is.readFully(launchAnswerBuffer); String launchUuid = new String(launchAnswerBuffer, TRANSFER_CHARSET); byte[] saveBuffer = (command.name() + COMMAND_DELIMITER + instanceUuid).getBytes(TRANSFER_CHARSET); OutputStream os = socket.getOutputStream(); os.write(saveBuffer); os.flush(); String expectedAnswer = instanceUuid + OK_SUFFIX; byte[] answerBuffer = new byte[expectedAnswer.getBytes(TRANSFER_CHARSET).length]; - //noinspection ResultOfMethodCallIgnored - is.read(answerBuffer); + is.readFully(answerBuffer); String answer = new String(answerBuffer, TRANSFER_CHARSET); if (!expectedAnswer.equals(answer)) { - LOGGER.warn("Invalid server instance UUID '{}' answer", command.name()); + LOGGER.warn("Invalid server answer. Expected '{}', got '{}'", expectedAnswer, answer); return null; }src/test/java/com/epam/reportportal/service/ItemLoggingContextMultiThreadTest.java (1)
59-61: Mockito @mock is never initialized — tests will NPE.Add Mockito JUnit 5 extension (preferred) or manually open mocks.
@@ -import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; @@ -import org.mockito.Mock; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; @@ -@SuppressWarnings("ReactiveStreamsUnusedPublisher") -public class ItemLoggingContextMultiThreadTest { +@SuppressWarnings("ReactiveStreamsUnusedPublisher") +@ExtendWith(MockitoExtension.class) +public class ItemLoggingContextMultiThreadTest {dep.txt (1)
1-446: Do not commit generated dependency reports; remove dep.txt from VCS.This is a transient, environment-specific Gradle output that will quickly drift and create noise in diffs. Please delete the file and, if needed, attach a dependencies report as a CI artifact or document the Gradle command to reproduce locally. Consider adding a .gitignore entry to prevent re‑adds.
src/main/java/com/epam/reportportal/utils/files/Utils.java (3)
140-142: Close theInputStreamopened fromFiles.newInputStream(...).
readFileToBytesleaks the stream; wrap it in try-with-resources.- public static byte[] readFileToBytes(@Nonnull File file) throws IOException { - return readInputStreamToBytes(Files.newInputStream(file.toPath())); - } + public static byte[] readFileToBytes(@Nonnull File file) throws IOException { + try (InputStream is = Files.newInputStream(file.toPath())) { + return readInputStreamToBytes(is); + } + }
189-197: Close resource stream when reading from classpath fallback.
getFileAsByteSourceopens a resource stream but never closes it.- } else { - data = readInputStreamToBytes(getResourceAsStream(file.getPath())); - } + } else { + try (InputStream is = getResourceAsStream(file.getPath())) { + data = readInputStreamToBytes(is); + } + }
219-229: Close classpath resource stream ingetFile(URI).Same leak pattern; close the stream before wrapping.
- ByteSource byteSource = ByteSource.wrap(readInputStreamToBytes(getResourceAsStream(resourcePath))); + byte[] bytes; + try (InputStream is = getResourceAsStream(resourcePath)) { + bytes = readInputStreamToBytes(is); + } + ByteSource byteSource = ByteSource.wrap(bytes);src/main/java/com/epam/reportportal/service/logs/BufferSubscriber.java (2)
75-94: Always unlock in finally to prevent deadlocks on unexpected exceptions.Current pattern unlocks manually and can deadlock if future changes throw between lock and unlock. Use try/finally.
Apply:
- lock.lock(); - if (buffer == null) { - lock.unlock(); - return; - } - if (payloadSize + size > payloadLimit) { - if (!buffer.isEmpty()) { - toSend.add(buffer); - reset(); - } - } - buffer.add(t); - long newSize = payloadSize + size; - payloadSize = newSize; - if (buffer.size() >= maxSize) { - toSend.add(buffer); - reset(); - } - lock.unlock(); + lock.lock(); + try { + if (buffer == null) { + return; + } + if (payloadSize + size > payloadLimit) { + if (!buffer.isEmpty()) { + toSend.add(buffer); + reset(); + } + } + buffer.add(t); + payloadSize += size; + if (buffer.size() >= maxSize) { + toSend.add(buffer); + reset(); + } + } finally { + lock.unlock(); + }
115-121: Same here: protect unlock with try/finally in onComplete.Avoid manual unlock; future edits could introduce exceptions and deadlock the subscriber.
Apply:
- lock.lock(); - if (buffer != null && !buffer.isEmpty()) { - toSend.add(buffer); - reset(); - } - lock.unlock(); + lock.lock(); + try { + if (buffer != null && !buffer.isEmpty()) { + toSend.add(buffer); + reset(); + } + } finally { + lock.unlock(); + }src/main/java/com/epam/reportportal/service/launch/lock/LaunchIdLockFile.java (2)
351-367: Bug: incorrect “is last record” detection and unsafe truncate logic when removing UUID.
Uses current time to estimate removed line length; can misclassify last-record case and truncates inconsistently. Rewrite the file from scratch and derive “is last” from recordList emptiness.- if (!needUpdate) { - return false; - } - - String recordNl = System.currentTimeMillis() + TIME_SEPARATOR + instanceUuid + LINE_SEPARATOR; - long newLength = fileAccess.length() - recordNl.length(); - if (newLength > 0) { - fileAccess.setLength(newLength); - fileAccess.seek(0); - for (String record : recordList) { - writeLine(fileAccess, record); - } - return false; - } else { - fileIo.getKey().setLength(0); - return true; - } + if (!needUpdate) { + return false; + } + // Rewrite fully to avoid length/offset mistakes and return whether list is empty. + fileAccess.setLength(0); + fileAccess.seek(0); + for (String record : recordList) { + writeLine(fileAccess, record); + } + return recordList.isEmpty();
377-383: Race/NPE risk: lockUuid may be null between reads; use null‑safe equality.
Concurrent reset() can null out lockUuid after mainLock check, causing NPE.- if (mainLock != null && lockUuid.equals(instanceUuid)) { + if (mainLock != null && java.util.Objects.equals(lockUuid, instanceUuid)) { reset(); if (!lockFile.delete()) { LOGGER.warn("Unable to delete locking file: {}", lockFile.getPath()); } }src/main/java/com/epam/reportportal/service/step/DefaultStepReporter.java (1)
120-124: Bug: finishing previous step with its own startTime produces zero‑duration stepssendStep stores the finish request timestamp equal to the step’s start time, so finishPreviousStepInternal() later finishes the previous step at its start, yielding zero duration and risking non‑monotonic overlaps. Use “now” when creating the finish request.
- addStepEntry(stepId, status, rq.getStartTime()); + addStepEntry(stepId, status, launch.useMicroseconds() ? Instant.now() : Calendar.getInstance().getTime());
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
build.gradle (3)
43-55: Align all Jackson components via BOM to prevent version skew.Optional but safer: import jackson-bom and omit per-module versions to keep core/annotations/databind consistent transitively.
Apply:
dependencies { - api "com.fasterxml.jackson.core:jackson-databind:${project.jackson_version}" // Access is needed by HTTP loggers to format JSON + api platform("com.fasterxml.jackson:jackson-bom:${project.jackson_version}") + api "com.fasterxml.jackson.core:jackson-databind" // Access is needed by HTTP loggers to format JSON @@ - implementation ("com.squareup.retrofit2:converter-jackson:${project.retrofit_version}") { + implementation ("com.squareup.retrofit2:converter-jackson:${project.retrofit_version}") { exclude module: 'jackson-databind' } }
30-31: Coverage gate lowered: note rationale.Dropping class coverage from 90 → 85 weakens the quality bar; consider adding a brief note in the PR description.
101-103: Consider upgrading Gradle wrapper in a follow‑up.5.4.1 is quite old; newer plugins and dependency features (and JDKs) are better supported on Gradle 7.x/8.x. Not blocking this PR.
src/test/java/com/epam/reportportal/service/ReportPortalTest.java (1)
66-66: Disambiguate Mockito any vs Hamcrest any; avoid wildcard to reduce surprisesPrefer explicit Mockito static imports over wildcard to prevent collisions with Hamcrest’s
any(Class).Apply this diff to make the intent clear:
-import static org.mockito.Mockito.*; -import static org.mockito.Mockito.any; +import static org.mockito.Mockito.after; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.timeout; +import static org.mockito.Mockito.verify;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
build.gradle(4 hunks)src/test/java/com/epam/reportportal/service/ItemLoggingContextMultiThreadTest.java(3 hunks)src/test/java/com/epam/reportportal/service/LaunchMicrosecondsTest.java(1 hunks)src/test/java/com/epam/reportportal/service/LaunchTest.java(6 hunks)src/test/java/com/epam/reportportal/service/ReportPortalTest.java(10 hunks)src/test/java/com/epam/reportportal/service/StatisticsRunnable.java(1 hunks)src/test/java/com/epam/reportportal/service/launch/lock/LaunchIdLockFileTest.java(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/test/java/com/epam/reportportal/service/LaunchTest.java
- src/test/java/com/epam/reportportal/service/LaunchMicrosecondsTest.java
- src/test/java/com/epam/reportportal/service/StatisticsRunnable.java
- src/test/java/com/epam/reportportal/service/ItemLoggingContextMultiThreadTest.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/test/java/com/epam/reportportal/service/launch/lock/LaunchIdLockFileTest.java (1)
src/main/java/com/epam/reportportal/service/launch/lock/LaunchIdLockFile.java (1)
LaunchIdLockFile(51-406)
src/test/java/com/epam/reportportal/service/ReportPortalTest.java (1)
src/test/java/com/epam/reportportal/test/TestUtils.java (1)
TestUtils(49-356)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (19)
src/test/java/com/epam/reportportal/service/launch/lock/LaunchIdLockFileTest.java (6)
105-111: Excellent fix for thread leak issue!The refactoring to use try-with-resources with
CommonUtils.testExecutor()properly addresses the previous review comment about shutting down ExecutorService to prevent thread leaks. This pattern ensures executors are automatically closed after test execution.
117-134: LGTM! Consistent resource management pattern applied.The try-with-resources pattern is correctly applied here as well, ensuring proper cleanup of the executor.
139-146: Improved resource management in helper method.Good refactoring to use try-with-resources within the
executeParallelLaunchUuidSynchelper method as well. This ensures consistent executor lifecycle management throughout the test suite.
258-264: Simplified resource management with try-with-resources.The nested try-with-resources pattern for
RandomAccessFileandFileLockis clean and ensures proper cleanup.
268-275: Consistent try-with-resources pattern.Good consistency in applying the same resource management pattern for lock file testing.
299-306: More portable line separator usage.Using
System.lineSeparator()instead ofSystem.getProperty("line.separator")is cleaner and more idiomatic.build.gradle (4)
64-64: LGTM: commons‑lang3 addition is reasonable.Brings useful utils (e.g., StringUtils) without heavy footprint.
77-77: OK — slf4j_version is 2.0.7 (no action required).
gradle.properties declares slf4j_version=2.0.7, which satisfies Logback 1.5.x's SLF4J 2.x requirement.
33-34: Java 11 bump — confirm CI/tooling and document the breaking changebuild.gradle now sets sourceCompatibility/targetCompatibility = JavaVersion.VERSION_11 (lines 33–34). Repo search found no README/CHANGELOG mention of Java 11 and no JDK references under .github.
- Ensure CI/build agents actually run JDK 11+ and update workflows/agents/images (GitHub Actions, Dockerfiles, Jenkins, etc.).
- Add a “minimum Java 11” note to README/CHANGELOG/release notes for downstream users.
- Search for and update any non-.github CI configs (.travis.yml, .circleci/, etc.) if present.
46-46: Jakarta annotations: verify no lingering javax.annotation or transitive jsr305 remains.build.gradle (≈line 46) adds api 'jakarta.annotation:jakarta.annotation-api:3.0.0' — confirm there are no javax.annotation imports and no com.google.code.findbugs:jsr305 (or other jsr305) transitive deps on compile/runtime classpaths. Suggested checks:
- ./gradlew ::dependencies --configuration runtimeClasspath (run per module or root as appropriate)
- ./gradlew dependencyInsight --dependency jsr305 --configuration runtimeClasspath
- git grep -n 'javax.annotation' || true
- git grep -n 'jsr305' || true
If found, exclude the transitive jsr305 or force jakarta.annotation-api to avoid duplicate annotation classes.
src/test/java/com/epam/reportportal/service/ReportPortalTest.java (9)
174-175: Double-check proxy auth response path consistencyOther responses moved under
files/responses/; this one remainsfiles/proxy_auth_response.txt. Ensure the file wasn't relocated tofiles/responses/proxy_auth_response.txt.
273-277: LGTM: Centralized response path and clearer ServerCallable constructionThe switch to
files/responses/socket_response.txtand multiline ctor improves readability.
298-298: LGTM: Unified resource pathUsing
files/responses/socket_response.txtkeeps resources consistent.
325-326: LGTM: Centralized simple response pathOk to use
Collections.singletonList("files/responses/simple_response.txt").
342-346: LGTM: Logging scenario uses consistent socket response pathReassignment with
createCookieModel()and centralized resource path looks good.
388-388: Good addition: mock batch logging to stabilize testsStubbing
rpClient.log(...)avoids unwanted network/log side effects.
393-393: Nice: try-with-resources executorUsing
CommonUtils.testExecutor()with try-with-resources simplifies lifecycle management.
487-488: LGTM: HTTPS test resource path updated
files/responses/socket_response.txtaligns with other tests.
136-137: Resource present in test classpath — no action requiredFound src/test/resources/files/responses/simple_response.txt (also socket_response.txt and proxy_auth_response.txt), so the resource is available for tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
src/main/java/com/epam/ta/reportportal/ws/model/OperationCompletionRS.java (2)
19-22: Add @JsonAlias for backward compatibility with “resultMessage”.Prevents deserialization breakage with older servers while still serializing as “message”.
Apply:
import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.annotation.JsonInclude.Include; import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonAlias; @@ - @JsonProperty("message") + @JsonProperty("message") + @JsonAlias("resultMessage") private String message;Also applies to: 28-29
24-25: Fix Javadoc grammar.Use “operations that return” or “operation that returns”.
- * Common response for operation that return just message about completion + * Common response for operations that return just a completion messagesrc/main/java/com/epam/ta/reportportal/ws/model/StartRQ.java (4)
47-51: Make startTime strongly typed as Instant; drop duplicate aliasUsing Comparable> obscures the contract and invites invalid types. Also, @JsonAlias repeats "startTime". Prefer Instant and keep only "start_time" as alias for backwards compatibility.
- @JsonProperty(value = "startTime", required = true) - @JsonSerialize(using = TimeSerializer.class) - @JsonDeserialize(using = TimeDeserializer.class) - @JsonAlias({ "startTime", "start_time" }) - private Comparable<? extends Comparable<?>> startTime; + @JsonProperty(value = "startTime", required = true) + @JsonSerialize(using = TimeSerializer.class) + @JsonDeserialize(using = TimeDeserializer.class) + @JsonAlias("start_time") + private Instant startTime;- public Comparable<? extends Comparable<?>> getStartTime() { + public Instant getStartTime() { return startTime; } - public void setStartTime(Comparable<? extends Comparable<?>> startTime) { + public void setStartTime(Instant startTime) { this.startTime = startTime; }Add import:
+import java.time.Instant;Also applies to: 88-94
43-45: Remove redundant alias for attributes"attributes" is already the property name; no need to alias it to itself. Keep only "tags" for backward compatibility.
- @JsonProperty("attributes") - @JsonAlias({ "attributes", "tags" }) + @JsonProperty("attributes") + @JsonAlias("tags") private Set<ItemAttributesRQ> attributes;
76-78: Defensive copy/null-safety for attributes setterAvoid exposing internal state to external mutation.
- public void setAttributes(Set<ItemAttributesRQ> attributes) { - this.attributes = attributes; - } + public void setAttributes(Set<ItemAttributesRQ> attributes) { + this.attributes = (attributes == null) ? null : new java.util.LinkedHashSet<>(attributes); + }
37-41: Consider making name private for consistencyUnless subclasses require direct field access, prefer private + accessors like other fields.
- @JsonProperty(value = "name", required = true) - protected String name; + @JsonProperty(value = "name", required = true) + private String name;src/main/java/com/epam/ta/reportportal/ws/model/launch/LaunchResource.java (3)
180-182: Defensive copy for attributes setterAvoids external mutation of internal collection.
- public void setAttributes(Set<ItemAttributeResource> attributes) { - this.attributes = attributes; - } + public void setAttributes(Set<ItemAttributeResource> attributes) { + this.attributes = (attributes == null) ? null : new LinkedHashSet<>(attributes); + }
200-202: Prefer isHasRetries() for bean compatibilityJackson and bean introspection consistently recognize "is..." for boolean getters.
- public boolean hasRetries() { + public boolean isHasRetries() { return hasRetries; }
52-60: Numeric timestamps may lose microseconds in TimeDeserializerThe shared deserializer uses getLongValue() for numbers and treats them as milliseconds, truncating fractional seconds and any microsecond-resolution numerics.
If clients may receive numeric microsecond timestamps here, update TimeDeserializer to:
- parse decimals via BigDecimal to seconds + nanos,
- or detect microseconds in large integers and scale appropriately.
If only ISO-8601 strings are expected, document that to avoid precision loss.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/com/epam/reportportal/service/logs/LaunchLoggingCallback.java(1 hunks)src/main/java/com/epam/reportportal/utils/serialize/TimeSerializer.java(1 hunks)src/main/java/com/epam/ta/reportportal/ws/model/OperationCompletionRS.java(1 hunks)src/main/java/com/epam/ta/reportportal/ws/model/StartRQ.java(1 hunks)src/main/java/com/epam/ta/reportportal/ws/model/launch/LaunchResource.java(1 hunks)src/main/java/com/epam/ta/reportportal/ws/model/launch/MergeLaunchesRQ.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/epam/ta/reportportal/ws/model/launch/MergeLaunchesRQ.java
- src/main/java/com/epam/reportportal/utils/serialize/TimeSerializer.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/com/epam/ta/reportportal/ws/model/StartRQ.java (5)
src/main/java/com/epam/reportportal/utils/serialize/TimeDeserializer.java (1)
TimeDeserializer(34-72)src/main/java/com/epam/reportportal/utils/serialize/TimeSerializer.java (1)
TimeSerializer(33-59)src/main/java/com/epam/ta/reportportal/ws/model/attribute/ItemAttributesRQ.java (1)
ItemAttributesRQ(21-65)src/main/java/com/epam/ta/reportportal/ws/model/launch/StartLaunchRQ.java (1)
JsonInclude(24-59)src/main/java/com/epam/ta/reportportal/ws/model/StartTestItemRQ.java (1)
JsonInclude(26-127)
src/main/java/com/epam/ta/reportportal/ws/model/launch/LaunchResource.java (4)
src/main/java/com/epam/reportportal/utils/serialize/TimeDeserializer.java (1)
TimeDeserializer(34-72)src/main/java/com/epam/reportportal/utils/serialize/TimeSerializer.java (1)
TimeSerializer(33-59)src/main/java/com/epam/ta/reportportal/ws/model/attribute/ItemAttributeResource.java (1)
ItemAttributeResource(21-63)src/main/java/com/epam/ta/reportportal/ws/model/launch/StartLaunchRQ.java (1)
JsonInclude(24-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (6)
src/main/java/com/epam/ta/reportportal/ws/model/OperationCompletionRS.java (1)
26-45: DTO looks good and matches logging callsites.Constructor/accessors align with the JSON property and the logging change.
src/main/java/com/epam/reportportal/service/logs/LaunchLoggingCallback.java (1)
41-41: Correct: use rs.getMessage() after DTO rename.
Matches OperationCompletionRS API and fixes the previous mismatch. Re-run the ripgrep checks for getResultMessage / setResultMessage / resultMessage locally and attach output if any matches remain.src/main/java/com/epam/ta/reportportal/ws/model/StartRQ.java (1)
47-51: Verify microsecond handling for numeric timestampsCurrent TimeDeserializer treats numeric as milliseconds and uses getLongValue(), which truncates floats and cannot represent microseconds. If clients may send numeric microseconds or seconds-with-fraction, this will lose precision.
Confirm that clients send ISO-8601 strings for microsecond precision. If numerics are expected, update TimeDeserializer to:
- accept decimals as seconds with fractional part (BigDecimal),
- detect microseconds (e.g., 16+ digit integers) and convert via Instant.ofEpochSecond/Instant.ofEpochMilli with nanos.
I can prepare a safe patch if you confirm numeric formats to support.
src/main/java/com/epam/ta/reportportal/ws/model/launch/LaunchResource.java (3)
52-60: Change startTime/endTime to Instant; add snake_case aliasesAligns with microseconds goal, improves type safety, and preserves compatibility via aliases.
- @JsonProperty(value = "startTime", required = true) - @JsonSerialize(using = TimeSerializer.class) - @JsonDeserialize(using = TimeDeserializer.class) - private Comparable<? extends Comparable<?>> startTime; + @JsonProperty(value = "startTime", required = true) + @JsonSerialize(using = TimeSerializer.class) + @JsonDeserialize(using = TimeDeserializer.class) + @JsonAlias("start_time") + private Instant startTime; - @JsonProperty(value = "endTime") - @JsonSerialize(using = TimeSerializer.class) - @JsonDeserialize(using = TimeDeserializer.class) - private Comparable<? extends Comparable<?>> endTime; + @JsonProperty(value = "endTime") + @JsonSerialize(using = TimeSerializer.class) + @JsonDeserialize(using = TimeDeserializer.class) + @JsonAlias("end_time") + private Instant endTime;- public Comparable<? extends Comparable<?>> getStartTime() { + public Instant getStartTime() { return startTime; } - public void setStartTime(Comparable<? extends Comparable<?>> startTime) { + public void setStartTime(Instant startTime) { this.startTime = startTime; } - public Comparable<? extends Comparable<?>> getEndTime() { + public Instant getEndTime() { return endTime; } - public void setEndTime(Comparable<? extends Comparable<?>> endTime) { + public void setEndTime(Instant endTime) { this.endTime = endTime; }Add imports:
+import com.fasterxml.jackson.annotation.JsonAlias; +import java.time.Instant;Also applies to: 136-150
77-79: Emit "analyzers" and accept legacy "analysing"; make setter defensivePrevents property-name mismatch and shields internal state.
- @JsonProperty(value = "analysing") - private Set<String> analyzers = new LinkedHashSet<>(); + @JsonProperty(value = "analyzers") + @JsonAlias("analysing") + private Set<String> analyzers = new LinkedHashSet<>();- public void setAnalyzers(Set<String> analyzers) { - this.analyzers = analyzers; - } + public void setAnalyzers(Set<String> analyzers) { + this.analyzers = (analyzers == null) ? new LinkedHashSet<>() : new LinkedHashSet<>(analyzers); + }Also applies to: 192-198
37-48: Required=true is not enforced—ensure guarantees or add validationThese fields are annotated required=true, but Jackson doesn’t fail on missing POJO properties by default. Either guarantee they’re always present in server responses or enforce via creator-based deserialization or validation.
- Option A: introduce a @JsonCreator constructor with these props and enable FAIL_ON_MISSING_CREATOR_PROPERTIES.
- Option B: annotate with jakarta.validation @NotNull/@notblank where applicable and validate at boundaries.
Confirm which approach you want; I can draft patches accordingly.
Also applies to: 65-67
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/main/java/com/epam/reportportal/utils/http/ContentType.java (1)
95-106: Normalize KNOWN_TYPES to lowercase to enforce case‑insensitive semanticsMIME types are case‑insensitive. Normalize entries at build time to harden against future mixed‑case constants and to pair with a case‑insensitive lookup.
Apply:
KNOWN_TYPES = Arrays.stream(ContentType.class.getFields()) .filter(f -> String.class.equals(f.getType()) && Modifier.isStatic(f.getModifiers()) && Modifier.isPublic(f.getModifiers())) .map(f -> { try { return (String) f.get(null); } catch (IllegalAccessException e) { return null; } }) - .filter(Objects::nonNull) - .collect(Collectors.toUnmodifiableSet()); + .filter(Objects::nonNull) + .map(s -> s.toLowerCase(java.util.Locale.ROOT)) + .collect(Collectors.toUnmodifiableSet());src/main/java/com/epam/reportportal/utils/serialize/TimeSerializer.java (1)
54-59: Fail fast on unsupported time types; handle Number generically and allow pre-formatted CharSequence.Avoid serializing unknown objects via toString(), which can emit unexpected formats (e.g., nanoseconds or no zone). Treat any Number as epoch millis; accept pre-formatted strings; otherwise throw a mapping error.
Apply:
- } else if (value instanceof Long) { - gen.writeNumber((Long) value); - } else { - // Fallback for other Comparable types - convert to string - gen.writeString(value.toString()); - } + } else if (value instanceof Number) { + gen.writeNumber(((Number) value).longValue()); + } else if (value instanceof CharSequence) { + // Pre-formatted ISO-8601 string supplied by caller + gen.writeString(value.toString()); + } else { + throw com.fasterxml.jackson.databind.JsonMappingException.from( + gen, "Unsupported time value type: " + value.getClass().getName() + ); + }Please verify no callers rely on arbitrary toString() serialization for time fields.
src/main/java/com/epam/reportportal/service/step/DefaultStepReporter.java (1)
329-329: Review the suppression of rawtypes and unchecked warnings.While the suppression may be necessary due to the generic
Comparable<? extends Comparable<?>>handling, consider if this can be made more type-safe.The current implementation handles multiple timestamp types (
Date,Instant,Long) with raw types. Consider creating a sealed interface or enum-based approach for better type safety:public sealed interface TimestampValue extends Comparable<TimestampValue> permits DateTimestamp, InstantTimestamp, LongTimestamp { Comparable<?> getValue(); TimestampValue addMinimalIncrement(); }src/main/java/com/epam/reportportal/service/launch/lock/LaunchIdLockSocket.java (1)
26-26: Pair DataInputStream with DataOutputStream for symmetric framing.Given the move to
DataInputStream, also import/useDataOutputStreamfor length‑prefix writes as proposed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/java/com/epam/reportportal/service/launch/lock/LaunchIdLockSocket.java(3 hunks)src/main/java/com/epam/reportportal/service/step/DefaultStepReporter.java(7 hunks)src/main/java/com/epam/reportportal/service/tree/TestItemTree.java(2 hunks)src/main/java/com/epam/reportportal/utils/MemoizingSupplier.java(2 hunks)src/main/java/com/epam/reportportal/utils/files/ByteSource.java(2 hunks)src/main/java/com/epam/reportportal/utils/files/Utils.java(4 hunks)src/main/java/com/epam/reportportal/utils/http/ContentType.java(4 hunks)src/main/java/com/epam/reportportal/utils/serialize/TimeSerializer.java(1 hunks)src/main/java/com/epam/ta/reportportal/ws/model/launch/LaunchResource.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/java/com/epam/reportportal/service/tree/TestItemTree.java
- src/main/java/com/epam/reportportal/utils/files/ByteSource.java
- src/main/java/com/epam/ta/reportportal/ws/model/launch/LaunchResource.java
- src/main/java/com/epam/reportportal/utils/files/Utils.java
- src/main/java/com/epam/reportportal/utils/MemoizingSupplier.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (13)
src/main/java/com/epam/reportportal/utils/http/ContentType.java (2)
19-23: Jakarta migration imports — LGTMSwitch to
jakarta.annotation.Nullableand addingObjectsimport look good.
141-146: isKnownType: still case‑sensitive; lower‑case before lookupYou fixed the double
stripMediaTypecall, but the comparison remains case‑sensitive. Lower‑case the stripped value for correct behavior with mixed‑case inputs (HTTP/MIME are case‑insensitive).Apply:
- String stripMediaType = stripMediaType(mediaType); - if (stripMediaType == null) { + String stripped = stripMediaType(mediaType); + if (stripped == null) { return false; } - return KNOWN_TYPES.contains(stripMediaType); + return KNOWN_TYPES.contains(stripped.toLowerCase(java.util.Locale.ROOT));src/main/java/com/epam/reportportal/utils/serialize/TimeSerializer.java (1)
23-27: Emit Instants with microseconds and a 'Z' suffix; fix formatter, naming, and comments.Current pattern prints “+0000” and the comments say “milliseconds”. Use a microsecond-precise ISO-8601 formatter that outputs a trailing ‘Z’ and exactly 6 fractional digits.
Apply:
@@ -import java.time.ZoneOffset; import java.time.format.DateTimeFormatter; +import java.time.format.DateTimeFormatterBuilder; @@ /** * Custom serializer for time values that handles both Date and Instant objects. * - Date objects are serialized to long values (milliseconds from epoch) - * - Instant objects are serialized to ISO time format with milliseconds + * - Instant objects are serialized to ISO-8601 UTC with microseconds (6 fractional digits, 'Z' suffix) */ @@ -public class TimeSerializer extends JsonSerializer<Object> { +public class TimeSerializer extends JsonSerializer<Object> { @@ - private static final DateTimeFormatter ISO_FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSSSSZ") - .withZone(ZoneOffset.UTC); + private static final DateTimeFormatter ISO_MICRO_INSTANT = + new DateTimeFormatterBuilder().appendInstant(6).toFormatter(); @@ - } else if (value instanceof Instant) { - // Serialize Instant to ISO time format with milliseconds - Instant instant = (Instant) value; - gen.writeString(ISO_FORMATTER.format(instant)); + } else if (value instanceof Instant) { + // Serialize Instant to ISO-8601 UTC with microseconds + Instant instant = (Instant) value; + gen.writeString(ISO_MICRO_INSTANT.format(instant));To confirm wire-compat expectations (‘Z’ vs ‘+0000’), run:
#!/bin/bash set -euo pipefail echo "Occurrences expecting Z-suffix microseconds:" rg -nP -C2 'T\d{2}:\d{2}:\d{2}\.\d{6}Z' --type=java --type=kt || true echo "Occurrences expecting +0000 offset:" rg -nP -C2 '\+0000' --type=java --type=kt || true echo "Usages of TimeSerializer:" rg -nP -C2 '\bTimeSerializer\b' --type=javaAlso applies to: 29-33, 36-37, 51-53
src/main/java/com/epam/reportportal/service/step/DefaultStepReporter.java (9)
32-33: Migration to Jakarta EE annotations looks correct.The migration from
javax.annotationtojakarta.annotationaligns with the Java EE to Jakarta EE rebranding, where package relocation was necessary to avoid trademark issues. The functionality remains the same with only package name changes.
39-40: Good addition of microsecond timing support.The imports for
InstantandChronoUnitare correctly added to support the new microsecond-precision timing functionality.
122-122: Conditional timestamp selection implementation is correct.The microseconds mode selection using
launch.useMicroseconds()properly chooses betweenInstant.now()for microsecond precision andCalendar.getInstance().getTime()for backward compatibility.
241-244: Microsecond-aware step entry creation looks good.The conditional timestamp logic in
StepEntrycreation maintains consistency with the microseconds mode selection throughout the class.
276-279: Consistent timestamp handling in finish request.The finish request properly uses the same conditional logic for timestamp generation, ensuring consistency between step start and finish times.
361-361: Consistent microsecond-aware timestamp in step requests.The conditional timestamp selection in
buildStartStepRequestmaintains consistency with the overall microseconds mode handling.
375-375: Log timestamps aligned with step timestamps.The log timestamp generation correctly uses the same microseconds mode conditional logic, ensuring consistency between step and log timing.
333-348: Verify monotonic timestamp adjustment and add tests
- Inconsistent increments: Date uses +1 millisecond, Instant uses +1 microsecond, Long uses +1 (unit unclear). Fallback retains the original timestamp (no increment) and can still break monotonicity.
- Add unit tests for: Instant vs Date precision, behavior when previous >= current (ensure startTime becomes strictly greater), edge cases near max/overflow, and unknown timestamp-type fallback handling.
Location: src/main/java/com/epam/reportportal/service/step/DefaultStepReporter.java (lines 333–348)
365-368: addStepEntry timestamp signature is compatible — no changes required. StepEntry (StepReporter.java), StepRequestUtils and time-related models use Comparable>; DefaultStepReporter callers pass Instant or Date (launch.useMicroseconds() ? Instant.now() : Calendar.getInstance().getTime()), and LaunchImpl/serializers handle conversions — no incompatible usages found.src/main/java/com/epam/reportportal/service/launch/lock/LaunchIdLockSocket.java (1)
22-22: Jakarta migration LGTM.Switch to
jakarta.annotation.Nonnullis consistent with the PR goal.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
src/test/java/com/epam/reportportal/service/LaunchMicrosecondsTest.java (4)
145-146: Relax ISO pattern to accept Z or +00:00 timezoneSerializer may emit ‘Z’ or ‘+00:00’. Current regex only matches “+0000”.
- Pattern p = Pattern.compile("\"" + key + "\"\\s*:\\s*\"\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d{6,9}\\+0000\""); + Pattern p = Pattern.compile( + "\"" + key + "\"\\s*:\\s*\"\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d{6,9}(Z|\\+00:?00)\"" + );
137-141: Make numeric assertion whitespace‑tolerantJSON emitters can insert spaces around “:”. Prefer regex over raw contains.
- String expectedField = "\"" + key + "\":" + START_TIME_MILLISECONDS_BASE; - assertThat("Expected numeric " + key + " in JSON: " + json, json, containsString(expectedField)); + Pattern p = Pattern.compile("\"" + key + "\"\\s*:\\s*" + START_TIME_MILLISECONDS_BASE); + assertThat("Expected numeric " + key + " in JSON: " + json, p.matcher(json).find(), is(true));
111-117: Reuse the START_TIME_NANO_ADJUSTMENT constantAvoid duplicating “123_456_000”.
- Instant testInstant = Instant.ofEpochSecond(START_TIME_SECONDS_BASE, 123_456_000); + Instant testInstant = Instant.ofEpochSecond(START_TIME_SECONDS_BASE, START_TIME_NANO_ADJUSTMENT);
74-81: Comment contradicts code; disable HTTP logging or update commentYou say “avoid OkHttp logger dependency” but enable httpLogging(true). It’s unnecessary for these assertions.
- parameters.setHttpLogging(true); + parameters.setHttpLogging(false);Or update the comment to reflect that logging is enabled but not relied upon.
src/main/java/com/epam/reportportal/utils/serialize/TimeSerializer.java (5)
30-33: Fix Javadoc: it's microseconds, not milliseconds.@@ - * - Instant objects are serialized to ISO time format with milliseconds + * - Instant objects are serialized to ISO-8601 with microseconds (6 fractional digits) and 'Z' suffix
54-55: Broaden numeric handling to any Number (not just Long).Prevents surprising toString fallback for Integer, AtomicLong, etc.
@@ - } else if (value instanceof Long) { - gen.writeNumber((Long) value); + } else if (value instanceof Number) { + gen.writeNumber(((Number) value).longValue());
57-58: Correct misleading comment ("Comparable").The branch isn’t limited to Comparable.
@@ - // Fallback for other Comparable types - convert to string + // Fallback for other types - convert to string
19-27: Import DateTimeFormatterBuilder explicitly if you keep it unqualified.If you don’t use the fully qualified name in the formatter change, add the import.
@@ -import java.time.format.DateTimeFormatter; +import java.time.format.DateTimeFormatter; +import java.time.format.DateTimeFormatterBuilder;
36-37: Rename ISO_FORMATTER to ISO_MICRO_FORMATTER and fix the fractional-second commentThe DateTimeFormatter pattern uses six fractional digits ("SSSSSS") — microseconds — but the inline serialize comment says milliseconds. Rename the constant and update all references and the comment.
Files to update:
- src/main/java/com/epam/reportportal/utils/serialize/TimeSerializer.java (def: lines 36–37; serialize/comment: ~51–53)
- src/main/java/com/epam/reportportal/utils/serialize/TimeDeserializer.java (uses TimeSerializer.ISO_FORMATTER: line 57)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/epam/reportportal/utils/serialize/TimeDeserializer.java(1 hunks)src/main/java/com/epam/reportportal/utils/serialize/TimeSerializer.java(1 hunks)src/test/java/com/epam/reportportal/service/LaunchMicrosecondsTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/epam/reportportal/utils/serialize/TimeDeserializer.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/test/java/com/epam/reportportal/service/LaunchMicrosecondsTest.java (2)
src/main/java/com/epam/reportportal/listeners/ListenerParameters.java (1)
ListenerParameters(43-839)src/main/java/com/epam/reportportal/service/ReportPortal.java (1)
ReportPortal(72-641)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (3)
src/test/java/com/epam/reportportal/service/LaunchMicrosecondsTest.java (1)
412-417: Ensure finish() completes before assertionsIf finish() is async, consider blocking (e.g., Completable.blockingAwait()) to avoid races; or verify that SocketUtils already synchronizes on expected requests.
Can you confirm the return type of Launch.finish(FinishExecutionRQ)? If it’s reactive, blocking here would make the test more deterministic.
src/main/java/com/epam/reportportal/utils/serialize/TimeSerializer.java (2)
51-53: Clarify comment and align with microsecond goal.@@ - // Serialize Instant to ISO time format with milliseconds + // Serialize Instant to ISO-8601 with microseconds and 'Z' suffix
36-37: Replace formatter with appendInstant(6) and update tests that assert '+0000'ofPattern("...Z") emits an RFC‑822 offset (+0000); use DateTimeFormatterBuilder.appendInstant(6) to produce fixed 6 fractional digits and a literal 'Z'.
@@ - public static final DateTimeFormatter ISO_FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSSSSZ") - .withZone(ZoneOffset.UTC); + public static final DateTimeFormatter ISO_FORMATTER = + new java.time.format.DateTimeFormatterBuilder() + .appendInstant(6) + .toFormatter() + .withZone(ZoneOffset.UTC);Also update the test expecting "+0000": src/test/java/com/epam/reportportal/service/LaunchMicrosecondsTest.java (line ~145) — change the regex to expect 'Z' (or accept both '+0000' and 'Z').
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/com/epam/reportportal/service/step/DefaultStepReporter.java (3)
241-244: Use the request’s startTime for StepEntry to keep type/time consistent.
Avoid tiny drifts and potential type mismatches by reusing the already computed startTime from the request, with a safe fallback.Apply this diff:
- steps.put( - itemId, - new StepEntry(itemId, launch.useMicroseconds() ? Instant.now() : Calendar.getInstance().getTime(), new FinishTestItemRQ()) - ); + steps.put( + itemId, + new StepEntry( + itemId, + ofNullable(startStepRequest.getStartTime()) + .orElse(launch.useMicroseconds() ? Instant.now() : Calendar.getInstance().getTime()), + new FinishTestItemRQ() + ) + );
260-269: Remove redundant status assignments in finishNestedStep.
One final setStatus is enough; preserve existing semantics.Apply this diff:
- FinishTestItemRQ actualRequest = ObjectUtils.clonePojo(finishStepRequest, FinishTestItemRQ.class); - if (manualStatus != null) { - actualRequest.setStatus(manualStatus); - if (ItemStatus.FAILED.name().equalsIgnoreCase(manualStatus)) { - failParents(); - } - } else { - actualRequest.setStatus(runStatus); - } - actualRequest.setStatus(ofNullable(manualStatus).orElse(runStatus)); + FinishTestItemRQ actualRequest = ObjectUtils.clonePojo(finishStepRequest, FinishTestItemRQ.class); + String finalStatus; + if (manualStatus != null) { + finalStatus = manualStatus; + if (ItemStatus.FAILED.name().equalsIgnoreCase(manualStatus)) { + failParents(); + } + } else { + finalStatus = runStatus; + } + actualRequest.setStatus(finalStatus);
329-348: Avoid cross-type Comparable.compareTo; compare in epoch micros and increment currentDate.
Mixing Date/Instant/Long can throw ClassCastException. Also prefer ordering against the previous endTime when available.Apply this diff:
- Comparable previousDate = e.getTimestamp(); - Comparable currentDate = startTestItemRQ.getStartTime(); - if (previousDate.compareTo(currentDate) >= 0) { - Comparable newDate; - if (previousDate instanceof Date) { - newDate = new Date(((Date) previousDate).getTime() + 1); - } else if (previousDate instanceof Instant) { - newDate = ((Instant) previousDate).plus(1, ChronoUnit.MICROS); - } else if (previousDate instanceof Long) { - newDate = ((Long) previousDate) + 1; - } else { - // Fallback to the original timestamp if it is not a Date or Instant to not fail reporting - newDate = previousDate; - } - startTestItemRQ.setStartTime(newDate); - } + Comparable previousDate = ofNullable(e.getFinishTestItemRQ().getEndTime()).orElse(e.getTimestamp()); + Comparable currentDate = startTestItemRQ.getStartTime(); + if (previousDate != null && currentDate != null) { + long prevMicros; + if (previousDate instanceof Date) { + prevMicros = ((Date) previousDate).getTime() * 1000L; + } else if (previousDate instanceof Instant) { + Instant p = (Instant) previousDate; + prevMicros = p.getEpochSecond() * 1_000_000L + p.getNano() / 1_000L; + } else if (previousDate instanceof Long) { + prevMicros = (Long) previousDate; + } else { + prevMicros = Long.MIN_VALUE; + } + long curMicros; + if (currentDate instanceof Date) { + curMicros = ((Date) currentDate).getTime() * 1000L; + } else if (currentDate instanceof Instant) { + Instant c = (Instant) currentDate; + curMicros = c.getEpochSecond() * 1_000_000L + c.getNano() / 1_000L; + } else if (currentDate instanceof Long) { + curMicros = (Long) currentDate; + } else { + curMicros = Long.MAX_VALUE; + } + if (prevMicros >= curMicros) { + if (currentDate instanceof Date) { + startTestItemRQ.setStartTime(new Date(((Date) currentDate).getTime() + 1)); + } else if (currentDate instanceof Instant) { + startTestItemRQ.setStartTime(((Instant) currentDate).plus(1, ChronoUnit.MICROS)); + } else if (currentDate instanceof Long) { + startTestItemRQ.setStartTime(((Long) currentDate) + 1); + } + } + }Please confirm FinishTestItemRQ exposes getEndTime(); if not, we can keep using the start timestamp fallback only.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/epam/reportportal/service/step/DefaultStepReporter.java(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (7)
src/main/java/com/epam/reportportal/service/step/DefaultStepReporter.java (7)
32-33: Jakarta annotations migration — LGTM.
39-40: Instant/ChronoUnit imports for microseconds — LGTM.
122-123: Finish timestamp source aligned with microseconds flag — LGTM.
276-279: Finish time builder updated for microseconds — LGTM.
361-362: Start time source aligned with microseconds flag — LGTM.
365-369: Generic Comparable timestamps in addStepEntry — LGTM.
376-377: Log time source aligned with microseconds flag — LGTM.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (13)
src/test/java/com/epam/reportportal/service/LaunchMicrosecondsTest.java (4)
76-82: Disable HTTP logging in tests unless asserted
setHttpLogging(true)adds overhead/noise without being used. Prefer default false.private static ListenerParameters baseParameters(String baseUrl) { ListenerParameters parameters = TestUtils.standardParameters(); parameters.setBaseUrl(baseUrl); - parameters.setHttpLogging(true); + parameters.setHttpLogging(false); return parameters; }
137-141: Make numeric time assertion whitespace‑tolerantJSON writers may add spaces around colon; use a regex instead of a raw substring.
private static void assertTimeNumeric(String json, String key) { assertThat("Request body should be present", json, notNullValue()); - String expectedField = "\"" + key + "\":" + START_TIME_MILLISECONDS_BASE; - assertThat("Expected numeric " + key + " in JSON: " + json, json, containsString(expectedField)); + Pattern p = Pattern.compile("\"" + key + "\"\\s*:\\s*" + START_TIME_MILLISECONDS_BASE); + assertThat("Expected numeric " + key + " in JSON: " + json, p.matcher(json).find(), is(true)); }
143-147: Broaden timezone match to accept Z and +00:00Serializer may emit “Z” or “+00:00” instead of “+0000”. Loosen regex to avoid false negatives.
private static void assertTimeIsoMicro(String json, String key) { assertThat("Request body should be present", json, notNullValue()); - Pattern p = Pattern.compile("\"" + key + "\"\\s*:\\s*\"\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d{6,9}\\+0000\""); + Pattern p = Pattern.compile( + "\"" + key + "\"\\s*:\\s*\"\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d{6,9}(?:Z|[+-]\\d{2}:?\\d{2})\"" + ); assertThat("Expected ISO instant with microseconds for " + key + " in JSON: " + json, p.matcher(json).find(), is(true)); }If the intended format is strictly “+0000”, keep it and ignore this change. Otherwise, this makes tests resilient to common ISO8601 variants.
183-426: Consider parameterized tests to reduce duplicationThe 16 near-identical cases vary by booleans and call type. JUnit 5 ParameterizedTest would cut boilerplate and speed maintenance.
src/main/java/com/epam/reportportal/utils/serialize/TimeDeserializer.java (3)
34-38: Align method return type with the declared deserializer generic for clarity.Use Comparable to exactly match JsonDeserializer>. Reduces cognitive load and avoids tool warnings.
Apply this diff:
- public Comparable<? extends Comparable<?>> deserialize(JsonParser p, DeserializationContext ctxt) throws IOException { + public Comparable<?> deserialize(JsonParser p, DeserializationContext ctxt) throws IOException {
51-55: Use String.isBlank() (Java 11) and drop redundant null check.p.getText() is non-null for VALUE_STRING.
Apply this diff:
- String value = p.getText(); - if (value == null || value.trim().isEmpty()) { + String value = p.getText(); + if (value.isBlank()) { return null; }
29-33: Update Javadoc to reflect all accepted formats and types.Clarify behavior for numeric-only strings and micro/nano support.
Apply this diff:
- * - Long/numeric values are deserialized to Date objects (milliseconds from epoch) - * - String values are deserialized to Instant objects using ISO time format + * - Integer JSON numbers -> Date (epoch milliseconds) + * - String values: + * - ISO-8601 with microseconds (serializer format) -> Instant + * - Other ISO-8601 accepted by Instant.parse -> Instant + * - Numeric-only strings: infer unit by digit count (s/ms/us/ns); ms -> Date, us/ns -> Instantsrc/main/java/com/epam/reportportal/service/step/DefaultStepReporter.java (6)
122-122: Align Instant to microsecond precision to avoid rounding collisions.When useMicroseconds() is true, some backends round to micros. Using Instant.now() with nanos can cause equal/unstable ordering after serialization. Truncate to MICROS at the source.
Apply this diff:
- addStepEntry(stepId, status, rq.getStartTime(), launch.useMicroseconds() ? Instant.now() : Calendar.getInstance().getTime()); + addStepEntry( + stepId, + status, + rq.getStartTime(), + launch.useMicroseconds() ? Instant.now().truncatedTo(ChronoUnit.MICROS) : Calendar.getInstance().getTime() + );
274-277: Also truncate finish time to microseconds.Apply this diff:
- FinishTestItemRQ finishStepRequest = buildFinishTestItemRequest( - status, - launch.useMicroseconds() ? Instant.now() : Calendar.getInstance().getTime() - ); + FinishTestItemRQ finishStepRequest = buildFinishTestItemRequest( + status, + launch.useMicroseconds() ? Instant.now().truncatedTo(ChronoUnit.MICROS) : Calendar.getInstance().getTime() + );
327-346: Make Instant bump micro-aligned (and keep it strictly increasing).If previousDate is Instant, ensure the bumped value is micro-aligned as well.
Apply this diff:
- } else if (previousDate instanceof Instant) { - newDate = ((Instant) previousDate).plus(1, ChronoUnit.MICROS); + } else if (previousDate instanceof Instant) { + newDate = ((Instant) previousDate).plus(1, ChronoUnit.MICROS).truncatedTo(ChronoUnit.MICROS);Optional: guard comparisons by type to avoid ClassCastException if types diverge unexpectedly:
- Only compare when previousDate.getClass() == currentDate.getClass().
- Otherwise skip adjustment or normalize both to a common representation.
359-359: Truncate step start time to microseconds.Apply this diff:
- startTestItemRQ.setStartTime(launch.useMicroseconds() ? Instant.now() : Calendar.getInstance().getTime()); + startTestItemRQ.setStartTime(launch.useMicroseconds() ? Instant.now().truncatedTo(ChronoUnit.MICROS) : Calendar.getInstance().getTime());
374-374: Truncate log time to microseconds.Apply this diff:
- rq.setLogTime(launch.useMicroseconds() ? Instant.now() : Calendar.getInstance().getTime()); + rq.setLogTime(launch.useMicroseconds() ? Instant.now().truncatedTo(ChronoUnit.MICROS) : Calendar.getInstance().getTime());
122-122: Reduce duplication: centralize “now” timestamp selection.Multiple sites branch on useMicroseconds(). Extract a small helper to keep behavior consistent and avoid drift.
For example, add:
private Comparable<? extends Comparable<?>> nowTimestamp() { return launch.useMicroseconds() ? Instant.now().truncatedTo(ChronoUnit.MICROS) : Calendar.getInstance().getTime(); }Then replace call sites with nowTimestamp().
Also applies to: 274-277, 359-359, 374-374
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/epam/reportportal/service/step/DefaultStepReporter.java(6 hunks)src/main/java/com/epam/reportportal/utils/serialize/TimeDeserializer.java(1 hunks)src/main/java/com/epam/reportportal/utils/serialize/TimeSerializer.java(1 hunks)src/test/java/com/epam/reportportal/service/LaunchMicrosecondsTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/epam/reportportal/utils/serialize/TimeSerializer.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/com/epam/reportportal/utils/serialize/TimeDeserializer.java (1)
src/main/java/com/epam/reportportal/utils/serialize/TimeSerializer.java (1)
TimeSerializer(34-61)
src/test/java/com/epam/reportportal/service/LaunchMicrosecondsTest.java (2)
src/main/java/com/epam/reportportal/listeners/ListenerParameters.java (1)
ListenerParameters(43-839)src/main/java/com/epam/reportportal/service/ReportPortal.java (1)
ReportPortal(72-641)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (6)
src/test/java/com/epam/reportportal/service/LaunchMicrosecondsTest.java (1)
133-135: Make HTTP body extraction robust; avoid AIOOBE on CRLF or missing separatorSplit on “\n\n” is brittle. Parse body via CRLF-first, fallback to LF. This was already flagged earlier.
private static String findLastJsonWithKey(Pair<List<String>, ?> result, String key) { - return ofNullable(findLastRequestWithKey(result.getKey(), key)).map(r -> r.split("\\n\\n")[1]).orElse(null); + String req = findLastRequestWithKey(result.getKey(), key); + if (req == null) return null; + return extractHttpBody(req); }Add this helper (outside the selected range):
private static String extractHttpBody(String raw) { int idx = raw.indexOf("\r\n\r\n"); int sepLen = 4; if (idx < 0) { idx = raw.indexOf("\n\n"); sepLen = 2; } return idx >= 0 ? raw.substring(idx + sepLen) : null; }src/main/java/com/epam/reportportal/utils/serialize/TimeDeserializer.java (1)
56-60: Confirm serializer/deserializer zone format compatibility.TimeSerializer uses pattern '...SSSSSSZ' (e.g., +0000) while Instant.parse expects 'Z' or '+00:00'. This deserializer first tries the custom formatter, then Instant.parse, which is fine. Please ensure tests cover both "+0000" and "Z"/"+00:00" inputs.
Would you like me to add unit tests covering:
- "+0000" (custom formatter) and "Z"/"+00:00" (Instant.parse) round-trips
- 10/13/16/19-digit numeric strings (s/ms/us/ns)
- Rejection of floating numbers?
src/main/java/com/epam/reportportal/service/step/DefaultStepReporter.java (4)
32-34: javax → jakarta migration looks correct here.
39-41: Time API imports align with microsecond support.
258-268: Status resolution logic reads well.
363-367: Method signature and finish request creation look consistent with Comparable timestamps.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests
Chores