[PRE-398] updated the existing add-on to work for the selenium 4.33.0 #200
Conversation
WalkthroughThis update introduces three new web action classes for extracting network data (payload, response time, status code), significantly refactors the request/response tracking logic to use updated Selenium DevTools and consolidated data storage, and overhauls utility methods to support granular retrieval of headers, payloads, status codes, and response times. The project dependencies are also upgraded. Changes
Sequence Diagram(s)sequenceDiagram
participant TestStep as Test Step
participant WebAction as WebAction (e.g., GetStatusCode)
participant Utilities as ResponseDataUtilities
participant Storage as Data Storage
TestStep->>WebAction: execute()
WebAction->>Utilities: getStatusCodeData(runId, logger)
Utilities->>Storage: Retrieve stored network data for runId
Utilities-->>WebAction: Return status code
WebAction->>TestStep: Store status code in runtime variable
sequenceDiagram
participant TestStep as Test Step
participant StartTracking as StartTracking Action
participant DevTools as Selenium DevTools
participant Utilities as ResponseDataUtilities
participant Storage as Data Storage
TestStep->>StartTracking: execute()
StartTracking->>DevTools: Start session, listen for requests/responses
DevTools-->>StartTracking: On request/response events
StartTracking->>Utilities: saveAllNetworkData(runId, allData, logger)
Utilities->>Storage: Store all network data as JSON
StartTracking->>TestStep: Return success/failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
store_network_logs_data/src/main/java/com/testsigma/addons/web/utilities/ResponseDataUtilities.java (1)
12-269: Potential thread safety issues with concurrent test executionThe utility class uses static methods that read/write to files without synchronization. If multiple tests run concurrently with the same
runId, this could lead to race conditions and data corruption.Consider adding synchronization or using thread-safe alternatives:
private static final Object fileLock = new Object(); private static void saveAllData(Long runId, JsonObject jsonObject, Logger logger) throws Exception { synchronized (fileLock) { // existing implementation } }Or use a thread-safe data structure like
ConcurrentHashMapto store data in memory instead of files.
🧹 Nitpick comments (9)
store_network_logs_data/src/main/java/com/testsigma/addons/web/GetDataFromRequestBody.java (2)
18-22: Consider renaming class to reflect actual functionality.The class name
GetDataFromRequestBodyis misleading since it now retrieves header values, not request body data. Consider renaming toGetDataFromRequestHeaderor similar to accurately reflect its purpose.
53-55: Improve exception handling specificity.The catch block for
IllegalStateExceptionprovides a helpful contextual error message, but consider whether other specific exceptions fromgetSpecificHeaderValueshould be handled differently.store_network_logs_data/src/main/java/com/testsigma/addons/web/GetResponseTime.java (2)
32-36: Add user feedback messages for better UX.Consider adding success and error messages using
setSuccessMessage()andsetErrorMessage()methods for better user feedback, similar to other action classes in the codebase.int responseTime = ResponseDataUtilities.getResponseTime(testCaseResult.getId(), logger); // store the response time in the runtime variable runTimeData.setKey(runtimeVariable.getValue().toString()); runTimeData.setValue(responseTime); +setSuccessMessage("Response time retrieved successfully: " + responseTime + "ms"); return Result.SUCCESS;
38-41: Improve error handling and logging level.Consider using
logger.warn()orlogger.error()instead oflogger.debug()for better error visibility, and addsetErrorMessage()for user feedback.} catch (Exception e) { - logger.debug("Error in getting response time: " + e.getMessage()); + logger.warn("Error in getting response time: " + e.getMessage()); + setErrorMessage("Failed to retrieve response time: " + e.getMessage()); return Result.FAILED; }store_network_logs_data/src/main/java/com/testsigma/addons/web/GetStatusCode.java (1)
31-31: Remove unnecessary variable initialization.The
Result result = Result.SUCCESS;initialization is unnecessary since you returnResult.SUCCESSdirectly later. Consider removing this line for cleaner code.-Result result = Result.SUCCESS; int statusCodeFromFile = getStatusCodeData(testCaseResult.getId(), logger);store_network_logs_data/src/main/java/com/testsigma/addons/web/GetPayloadDataForGivenUrl.java (1)
20-21: Consider adding custom screenshot configuration.The action is configured with
useCustomScreenshot = false. Verify if this setting is appropriate for payload data retrieval scenarios, as screenshots might be helpful for debugging network-related issues.store_network_logs_data/src/main/java/com/testsigma/addons/web/StartTracking.java (1)
173-177: Response time calculation could be affected by system clock changesThe response time calculation uses
System.currentTimeMillis()which is affected by system clock changes. While rare, this could result in negative or incorrect response times.Consider using
System.nanoTime()for more accurate elapsed time measurement:-final long[] requestStartTimes = new long[1]; +final long[] requestStartNanos = new long[1]; // In request listener: -requestStartTimes[0] = System.currentTimeMillis(); +requestStartNanos[0] = System.nanoTime(); // In response listener: -long responseTime = 0; -if (requestStartTimes[0] > 0) { - responseTime = System.currentTimeMillis() - requestStartTimes[0]; - logger.info("Response time: " + responseTime + "ms"); -} +long responseTimeMs = 0; +if (requestStartNanos[0] > 0) { + responseTimeMs = (System.nanoTime() - requestStartNanos[0]) / 1_000_000; + logger.info("Response time: " + responseTimeMs + "ms"); +}store_network_logs_data/src/main/java/com/testsigma/addons/web/utilities/ResponseDataUtilities.java (2)
23-23: Fix grammatical error in log message-logger.info("Did not saved data for the current testcase " + e.getMessage()); +logger.info("Did not save data for the current testcase " + e.getMessage());
106-178: Multiple overlapping methods for saving network dataThere are several methods that save partial network data (
addStatusCodeAndRequestHeadersData,addAllNetworkData,addStatusCodeData,addRequestHeadersData) which could lead to data inconsistency if used incorrectly. SinceStartTrackingusessaveAllNetworkDatato save all data atomically, consider whether these partial save methods are still needed.Consider either:
- Removing unused partial save methods to simplify the API
- Adding clear documentation about when to use each method
- Making partial save methods private if they're only for internal use
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
store_network_logs_data/pom.xml(3 hunks)store_network_logs_data/src/main/java/com/testsigma/addons/web/GetDataFromRequestBody.java(2 hunks)store_network_logs_data/src/main/java/com/testsigma/addons/web/GetPayloadDataForGivenUrl.java(1 hunks)store_network_logs_data/src/main/java/com/testsigma/addons/web/GetResponseTime.java(1 hunks)store_network_logs_data/src/main/java/com/testsigma/addons/web/GetStatusCode.java(1 hunks)store_network_logs_data/src/main/java/com/testsigma/addons/web/StartTracking.java(3 hunks)store_network_logs_data/src/main/java/com/testsigma/addons/web/utilities/ResponseDataUtilities.java(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
store_network_logs_data/src/main/java/com/testsigma/addons/web/GetDataFromRequestBody.java (1)
store_network_logs_data/src/main/java/com/testsigma/addons/web/utilities/ResponseDataUtilities.java (1)
ResponseDataUtilities(12-269)
store_network_logs_data/src/main/java/com/testsigma/addons/web/utilities/ResponseDataUtilities.java (1)
store_network_logs_data/src/main/java/com/testsigma/addons/web/utilities/FileUtilities.java (1)
FileUtilities(10-71)
store_network_logs_data/src/main/java/com/testsigma/addons/web/StartTracking.java (7)
store_network_logs_data/src/main/java/com/testsigma/addons/web/utilities/ResponseDataUtilities.java (1)
ResponseDataUtilities(12-269)store_network_logs_data/src/main/java/com/testsigma/addons/web/GetPayloadDataForGivenUrl.java (1)
Data(17-62)store_network_logs_data/src/main/java/com/testsigma/addons/web/GetResponseTime.java (1)
Data(14-44)store_network_logs_data/src/main/java/com/testsigma/addons/web/GetDataFromRequestBody.java (1)
Data(17-76)store_network_logs_data/src/main/java/com/testsigma/addons/web/GetStatusCode.java (1)
Data(14-49)store_network_logs_data/src/main/java/com/testsigma/addons/web/GetDataFromResponseBody.java (1)
Data(22-146)store_network_logs_data/src/main/java/com/testsigma/addons/web/NetworkTrafficCaptureActionLocal.java (1)
Data(26-211)
🔇 Additional comments (6)
store_network_logs_data/pom.xml (2)
9-9: LGTM! Version increment is appropriate.The project version bump from 1.0.2 to 1.0.5 properly reflects the significant functionality enhancements in this release.
51-51: Confirmed Selenium 4.33.0 compatibility with Chrome 137Selenium Java 4.33.0 and org.seleniumhq.selenium:selenium-devtools-v137:4.33.0 officially support Chrome v137, so the current dependency bump satisfies the PR objective.
• File: store_network_logs_data/pom.xml
– Lines 51, 71–72Optionally, you may upgrade to Selenium 4.34.0 (latest stable as of August 2025) for the newest fixes, but no compatibility changes are required.
store_network_logs_data/src/main/java/com/testsigma/addons/web/GetDataFromRequestBody.java (1)
48-65: LGTM! Simplified header retrieval implementation.The refactored implementation using
ResponseDataUtilities.getSpecificHeaderValueis much cleaner and more maintainable than the previous recursive JSON parsing approach. The error handling and logging are appropriate.store_network_logs_data/src/main/java/com/testsigma/addons/web/GetStatusCode.java (1)
32-42: LGTM! Well-implemented status code retrieval.The implementation properly handles the case where status code is not found (-1), provides good logging and user feedback messages, and follows the established pattern for action classes.
store_network_logs_data/src/main/java/com/testsigma/addons/web/GetPayloadDataForGivenUrl.java (1)
36-59: LGTM! Well-structured payload data retrieval.The implementation properly retrieves payload data using the utility method, converts it to string format, and provides comprehensive logging and user feedback. The error handling follows the established pattern.
store_network_logs_data/src/main/java/com/testsigma/addons/web/utilities/ResponseDataUtilities.java (1)
156-159: Header parsing may fail for values containing colonsThe current parsing logic uses
indexOf(": ")which would incorrectly parse headers with colons in their values (e.g., "Location: http://example.com:8080").Use
indexOf()with a limit to only split on the first colon:-int colonIndex = headerLine.indexOf(": "); -if (colonIndex > 0) { - String key = headerLine.substring(0, colonIndex).trim(); - String value = headerLine.substring(colonIndex + 2).trim(); +int colonIndex = headerLine.indexOf(":"); +if (colonIndex > 0 && colonIndex < headerLine.length() - 1) { + String key = headerLine.substring(0, colonIndex).trim(); + String value = headerLine.substring(colonIndex + 1).trim();Likely an incorrect or invalid review comment.
|
|
||
| public class GetPayloadDataForGivenUrl extends WebAction { | ||
|
|
||
| @TestData(reference = "runtim_variable", isRuntimeVariable = true) |
There was a problem hiding this comment.
Fix typo in TestData reference.
There's a typo in the @TestData annotation: "runtim_variable" should be "runtime_variable". This will cause runtime binding issues.
-@TestData(reference = "runtim_variable", isRuntimeVariable = true)
+@TestData(reference = "runtime_variable", isRuntimeVariable = true)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @TestData(reference = "runtim_variable", isRuntimeVariable = true) | |
| @TestData(reference = "runtime_variable", isRuntimeVariable = true) |
🤖 Prompt for AI Agents
In
store_network_logs_data/src/main/java/com/testsigma/addons/web/GetPayloadDataForGivenUrl.java
at line 26, correct the typo in the @TestData annotation by changing the
reference value from "runtim_variable" to "runtime_variable" to ensure proper
runtime binding.
| logger.info("Status code retrieved: " + statusCodeFromFile); | ||
| // Store the status code in the runtime variable | ||
| runTimeData.setKey(statusCodeVariable.getValue().toString()); | ||
| runTimeData.setValue(String.valueOf(statusCodeFromFile)); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify type consistency across similar actions.
The status code is converted to String before storing in runtime variable. Ensure this type handling is consistent with other similar actions like GetResponseTime which stores an int directly.
🏁 Script executed:
#!/bin/bash
# Check how other action classes handle numeric values in runtime variables
rg -A 3 -B 1 "runTimeData\.setValue" --type javaLength of output: 165833
🏁 Script executed:
#!/bin/bash
# Display the declaration and usage of responseTime in GetResponseTime.java
rg -n "responseTime" -C 3 store_network_logs_data/src/main/java/com/testsigma/addons/web/GetResponseTime.javaLength of output: 683
Align numeric runtime data storage in GetStatusCode
To keep numeric values consistent with GetResponseTime—which passes the int directly to runTimeData.setValue—update GetStatusCode to avoid converting the status code to a String.
Locations to update:
- store_network_logs_data/src/main/java/com/testsigma/addons/web/GetStatusCode.java, line 40
Suggested diff:
- runTimeData.setValue(String.valueOf(statusCodeFromFile));
+ runTimeData.setValue(statusCodeFromFile);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| runTimeData.setValue(String.valueOf(statusCodeFromFile)); | |
| runTimeData.setValue(statusCodeFromFile); |
🤖 Prompt for AI Agents
In
store_network_logs_data/src/main/java/com/testsigma/addons/web/GetStatusCode.java
at line 40, the status code is being converted to a String before passing to
runTimeData.setValue. To align with GetResponseTime and maintain numeric
consistency, pass the statusCodeFromFile integer directly to
runTimeData.setValue without converting it to a String.
| logger.warn("DevTools not supported by this driver. Using alternative network logging approach."); | ||
| // For drivers that don't support DevTools, we'll use browser logs | ||
| try { | ||
| // Enable browser logging | ||
| logger.info("Enabling browser performance logging..."); | ||
| // This is a fallback approach - the main DevTools approach should work | ||
| logger.info("DevTools approach failed, but action completed successfully."); | ||
| return Result.SUCCESS; | ||
| } catch (Exception e) { | ||
| logger.warn("Fallback approach also failed: " + e.getMessage()); | ||
| return Result.SUCCESS; | ||
| } | ||
| } |
There was a problem hiding this comment.
Misleading fallback logic that returns SUCCESS without implementing tracking
When DevTools is not supported, the code logs that it will use an "alternative network logging approach" but doesn't actually implement any alternative tracking. It then returns Result.SUCCESS, which could mislead users into thinking tracking is working when it's not.
Either implement actual fallback tracking or return Result.FAILED with a clear error message:
} else {
- logger.warn("DevTools not supported by this driver. Using alternative network logging approach.");
- // For drivers that don't support DevTools, we'll use browser logs
- try {
- // Enable browser logging
- logger.info("Enabling browser performance logging...");
- // This is a fallback approach - the main DevTools approach should work
- logger.info("DevTools approach failed, but action completed successfully.");
- return Result.SUCCESS;
- } catch (Exception e) {
- logger.warn("Fallback approach also failed: " + e.getMessage());
- return Result.SUCCESS;
- }
+ logger.error("DevTools not supported by this driver.");
+ setErrorMessage("DevTools is required for network tracking but not supported by this driver");
+ return Result.FAILED;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.warn("DevTools not supported by this driver. Using alternative network logging approach."); | |
| // For drivers that don't support DevTools, we'll use browser logs | |
| try { | |
| // Enable browser logging | |
| logger.info("Enabling browser performance logging..."); | |
| // This is a fallback approach - the main DevTools approach should work | |
| logger.info("DevTools approach failed, but action completed successfully."); | |
| return Result.SUCCESS; | |
| } catch (Exception e) { | |
| logger.warn("Fallback approach also failed: " + e.getMessage()); | |
| return Result.SUCCESS; | |
| } | |
| } | |
| } else { | |
| logger.error("DevTools not supported by this driver."); | |
| setErrorMessage("DevTools is required for network tracking but not supported by this driver"); | |
| return Result.FAILED; | |
| } |
🤖 Prompt for AI Agents
In
store_network_logs_data/src/main/java/com/testsigma/addons/web/StartTracking.java
between lines 59 and 71, the fallback logic for unsupported DevTools logs a
message about using an alternative network logging approach but does not
implement any actual tracking and returns Result.SUCCESS, which is misleading.
To fix this, either implement the alternative tracking logic properly or change
the return value to Result.FAILED and log a clear error message indicating that
tracking could not be started due to lack of DevTools support and no fallback
implementation.
| if (requestUrl.contains(urlVariable.getValue().toString()) && | ||
| requestMethod.equalsIgnoreCase(methodValue.getValue().toString())) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
URL matching may capture unintended requests
Using contains() for URL matching could capture unintended requests. For example, if urlVariable is "api", it would match both "api.example.com" and "example.com/api".
Consider using more precise matching or allowing regex patterns:
-if (requestUrl.contains(urlVariable.getValue().toString()) &&
- requestMethod.equalsIgnoreCase(methodValue.getValue().toString())) {
+String urlPattern = urlVariable.getValue().toString();
+if ((requestUrl.equals(urlPattern) || requestUrl.matches(urlPattern)) &&
+ requestMethod.equalsIgnoreCase(methodValue.getValue().toString())) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (requestUrl.contains(urlVariable.getValue().toString()) && | |
| requestMethod.equalsIgnoreCase(methodValue.getValue().toString())) { | |
| String urlPattern = urlVariable.getValue().toString(); | |
| if ((requestUrl.equals(urlPattern) || requestUrl.matches(urlPattern)) && | |
| requestMethod.equalsIgnoreCase(methodValue.getValue().toString())) { |
🤖 Prompt for AI Agents
In
store_network_logs_data/src/main/java/com/testsigma/addons/web/StartTracking.java
around lines 95 to 96, the use of contains() for URL matching is too broad and
may match unintended URLs. Replace contains() with a more precise matching
approach such as equals(), startsWith(), or implement regex pattern matching to
ensure only the intended URLs are matched. Adjust the logic to handle regex
patterns if you choose that route, validating the urlVariable value accordingly.
| public static int getResponseTime(Long runId, Logger logger) throws Exception { | ||
| logger.info("Getting response time for runId: " + runId); | ||
| String encodedData = FileUtilities.readFromFile(runId); | ||
| if (encodedData == null || encodedData.isEmpty()) { | ||
| logger.info("Response time data is not present for runId: " + runId); | ||
| throw new Exception("Failed to get any response time for runId: " + runId); | ||
|
|
||
| } | ||
|
|
||
| String json = new String(Base64.getDecoder().decode(encodedData)); | ||
| try { | ||
| JsonObject data = gson.fromJson(json, JsonObject.class); | ||
| if (data != null && data.has("responseTime")) { | ||
| int responseTime = data.get("responseTime").getAsInt(); | ||
| logger.info("Got response time for runId: " + runId + ", responseTime: " + responseTime); | ||
| return responseTime; |
There was a problem hiding this comment.
Response time data type mismatch could cause overflow
The method returns int but response times are stored as long in StartTracking. This could cause overflow for requests taking longer than ~24 days (Integer.MAX_VALUE milliseconds).
Change the return type to match the stored data type:
-public static int getResponseTime(Long runId, Logger logger) throws Exception {
+public static long getResponseTime(Long runId, Logger logger) throws Exception {
// ...
if (data != null && data.has("responseTime")) {
- int responseTime = data.get("responseTime").getAsInt();
+ long responseTime = data.get("responseTime").getAsLong();
logger.info("Got response time for runId: " + runId + ", responseTime: " + responseTime);
return responseTime;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static int getResponseTime(Long runId, Logger logger) throws Exception { | |
| logger.info("Getting response time for runId: " + runId); | |
| String encodedData = FileUtilities.readFromFile(runId); | |
| if (encodedData == null || encodedData.isEmpty()) { | |
| logger.info("Response time data is not present for runId: " + runId); | |
| throw new Exception("Failed to get any response time for runId: " + runId); | |
| } | |
| String json = new String(Base64.getDecoder().decode(encodedData)); | |
| try { | |
| JsonObject data = gson.fromJson(json, JsonObject.class); | |
| if (data != null && data.has("responseTime")) { | |
| int responseTime = data.get("responseTime").getAsInt(); | |
| logger.info("Got response time for runId: " + runId + ", responseTime: " + responseTime); | |
| return responseTime; | |
| public static long getResponseTime(Long runId, Logger logger) throws Exception { | |
| logger.info("Getting response time for runId: " + runId); | |
| String encodedData = FileUtilities.readFromFile(runId); | |
| if (encodedData == null || encodedData.isEmpty()) { | |
| logger.info("Response time data is not present for runId: " + runId); | |
| throw new Exception("Failed to get any response time for runId: " + runId); | |
| } | |
| String json = new String(Base64.getDecoder().decode(encodedData)); | |
| try { | |
| JsonObject data = gson.fromJson(json, JsonObject.class); | |
| if (data != null && data.has("responseTime")) { | |
| - int responseTime = data.get("responseTime").getAsInt(); | |
| + long responseTime = data.get("responseTime").getAsLong(); | |
| logger.info("Got response time for runId: " + runId + ", responseTime: " + responseTime); | |
| return responseTime; |
🤖 Prompt for AI Agents
In
store_network_logs_data/src/main/java/com/testsigma/addons/web/utilities/ResponseDataUtilities.java
around lines 242 to 257, the getResponseTime method returns an int but the
response time is stored as a long, risking overflow for large values. Change the
method's return type from int to long and update the variable holding the
response time to long accordingly to match the stored data type and prevent
overflow.
Jira
https://testsigma.atlassian.net/browse/TE-28689
Addon Name: Store Network Logs Data
Addon account: https://jarvis.testsigma.com/ui/tenants/2817/addons
fix
updated the devtools to chrome version 137
added nlp to retrieve payload data
added nlp to get the response time of endpoint
Summary by CodeRabbit
New Features
Improvements
Upgrades
Refactor