Playwright Extended Support for Network Log ,Alert message and interp… #4010
Conversation
…olation added in Analyzer
WalkthroughThis pull request introduces several changes across multiple classes within the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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: 18
🧹 Outside diff range and nitpick comments (14)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/BrowserHelper.cs (1)
1-57: Consider architectural improvements for better maintainabilityThe current implementation could benefit from:
- Interface extraction for better testability
- Separation of URL monitoring and file operations
- Unit tests for the functionality
Consider creating:
public interface IBrowserUrlMonitor { bool ShouldMonitorAllUrls(); bool ShouldMonitorUrl(string requestUrl); } public interface INetworkLogWriter { string CreateNetworkLogFile(string filename, List<Tuple<string, object>> networkLogList); } // Then implement these interfaces separately public class BrowserUrlMonitor : IBrowserUrlMonitor { ... } public class NetworkLogWriter : INetworkLogWriter { ... }Would you like me to help create unit tests for this functionality?
Ginger/GingerCoreNET/AnalyzerLib/AnalyzeAction.cs (4)
Line range hint
443-455: Enhance error handling with structured loggingThe catch block only writes to console using
Console.WriteLine. Consider implementing structured logging for better error tracking and debugging in production.- Console.WriteLine("Analyzer" + ex.Message); + Log.Error(ex, "Error analyzing POM mapping for action {ActionDescription}", a.ActionDescription);
Line range hint
32-391: Consider breaking down the Analyze method for better maintainabilityThe
Analyzemethod is quite long (>350 lines) and handles multiple responsibilities. Consider breaking it down into smaller, focused methods such as:
AnalyzeFlowControlsAnalyzeReturnValuesAnalyzeVariables
Line range hint
278-307: Remove commented-out codeThe commented-out code block for LocateBy validation should be either removed if it's no longer needed or uncommented if it's still relevant. Keeping commented code creates maintenance overhead.
Line range hint
463-466: Optimize LINQ query for duplicate checkThe LINQ query for checking duplicate ActInputValues is executed multiple times in the loop. Consider materializing the results once to improve performance.
- if (a.InputValues.Where(aiv => AIV != null && aiv != null && aiv.Param == AIV.Param).ToList().Count > 1) + var duplicates = a.InputValues + .Where(aiv => aiv?.Param != null) + .GroupBy(aiv => aiv.Param) + .Where(g => g.Count() > 1); + foreach (var duplicate in duplicates)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActBrowserElementHandler.cs (4)
593-595: Improve error message inHandleDismissMessageBoxOperationAsync.The error message "Error when Dismiss MessageBox." should be rephrased for grammatical correctness.
Apply this diff:
- _act.Error = "Error when Dismiss MessageBox."; - Reporter.ToLog(eLogLevel.ERROR, "Error when Dismiss MessageBox - " + e.Message); + _act.Error = "Error when dismissing MessageBox."; + Reporter.ToLog(eLogLevel.ERROR, "Error when dismissing MessageBox - " + e.Message);
621-623: Improve error message inHandleSetMessageBoxTextOperationAsync.The error message "Error when Set MessageBox Text." should be rephrased for clarity and proper grammar.
Apply this diff:
- _act.Error = "Error when Set MessageBox Text."; - Reporter.ToLog(eLogLevel.ERROR, "Error when Set MessageBox Text - " + e.Message); + _act.Error = "Error when setting MessageBox text."; + Reporter.ToLog(eLogLevel.ERROR, "Error when setting MessageBox text - " + e.Message);
607-609: Improve error message inHandleGetMessageBoxTextOperation.The error message "Error when Get MessageBox Text." should be rephrased for grammatical correctness.
Apply this diff:
- _act.Error = "Error when Get MessageBox Text."; - Reporter.ToLog(eLogLevel.ERROR, "Error when Get MessageBox Text - " + e.Message); + _act.Error = "Error when getting MessageBox text."; + Reporter.ToLog(eLogLevel.ERROR, "Error when getting MessageBox text - " + e.Message);
158-161: Remove redundant code inGetMessageBoxTexthandling.The check for
Actualbeing null is unnecessary since the value was just added. This redundancy can be removed to simplify the code.Apply this diff:
_act.AddOrUpdateReturnParamActual("Actual", AlertBoxText); - if (_act.GetReturnParam("Actual") == null) - { - _act.AddOrUpdateReturnParamActual("Actual", AlertBoxText); - }Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (3)
625-646: Unnecessary Use ofTask.RuninStartCaptureNetworkLogWrapping synchronous initialization code inside
await Task.Runis unnecessary and adds overhead. Since the code does not perform any long-running computations, consider removingawait Task.Runand execute the code synchronously.Modify the method as follows:
public async Task StartCaptureNetworkLog(ActBrowserElement act) { try { _act = act; networkRequestLogList = new List<Tuple<string, object>>(); networkResponseLogList = new List<Tuple<string, object>>(); NetworkResponseList = new ObservableList<IResponse>(); _playwrightPage.Request += OnNetworkRequestSent; _playwrightPage.Response += OnNetworkResponseReceived; isNetworkLogMonitoringStarted = true; } catch(Exception ex) { Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex); } }
647-680: Unnecessary Use ofTask.RuninGetCaptureNetworkLogThe use of
await Task.Runto wrap synchronous code is unnecessary. The method can be simplified by removingTask.Run.Modify the method as follows:
public async Task GetCaptureNetworkLog(ActBrowserElement act) { try { _act = act; if (isNetworkLogMonitoringStarted) { act.AddOrUpdateReturnParamActual("Raw Request", JsonConvert.SerializeObject(networkRequestLogList.Select(x => x.Item2).ToList(), Formatting.Indented)); act.AddOrUpdateReturnParamActual("Raw Response", JsonConvert.SerializeObject(networkResponseLogList.Select(x => x.Item2).ToList(), Formatting.Indented)); foreach (var val in networkRequestLogList) { act.AddOrUpdateReturnParamActual($"{nameof(act.ControlAction)} {val.Item1}", Convert.ToString(val.Item2)); } foreach (var val in networkResponseLogList) { act.AddOrUpdateReturnParamActual($"{nameof(act.ControlAction)} {val.Item1}", Convert.ToString(val.Item2)); } } else { act.ExInfo = $"Action is skipped, {nameof(ActBrowserElement.eControlAction.StartMonitoringNetworkLog)} action is not started"; act.Status = Amdocs.Ginger.CoreNET.Execution.eRunStatus.Skipped; } } catch (Exception ex) { Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex); } }
682-721: Unnecessary Use ofTask.RuninStopCaptureNetworkLogThe synchronous code inside
StopCaptureNetworkLogdoes not require wrapping inawait Task.Run. RemoveTask.Runto simplify the method.Modify the method as follows:
public async Task StopCaptureNetworkLog(ActBrowserElement act) { try { _act = act; _playwrightPage.Request -= OnNetworkRequestSent; _playwrightPage.Response -= OnNetworkResponseReceived; isNetworkLogMonitoringStarted = false; act.AddOrUpdateReturnParamActual("Raw Request", JsonConvert.SerializeObject(networkRequestLogList.Select(x => x.Item2).ToList())); act.AddOrUpdateReturnParamActual("Raw Response", JsonConvert.SerializeObject(networkResponseLogList.Select(x => x.Item2).ToList())); foreach (var val in networkRequestLogList) { act.AddOrUpdateReturnParamActual($"{nameof(act.ControlAction)} {val.Item1}", Convert.ToString(val.Item2)); } foreach (var val in networkResponseLogList) { act.AddOrUpdateReturnParamActual($"{nameof(act.ControlAction)} {val.Item1}", Convert.ToString(val.Item2)); } string requestPath = _BrowserHelper.CreateNetworkLogFile("NetworkLogRequest", networkRequestLogList); act.ExInfo = $"RequestFile: {requestPath}\n"; string responsePath = _BrowserHelper.CreateNetworkLogFile("NetworkLogResponse", networkResponseLogList); act.ExInfo += $"ResponseFile: {responsePath}\n"; act.AddOrUpdateReturnParamActual("RequestFile", requestPath); act.AddOrUpdateReturnParamActual("ResponseFile", responsePath); Act.AddArtifactToAction(Path.GetFileName(requestPath), act, requestPath); Act.AddArtifactToAction(Path.GetFileName(responsePath), act, responsePath); } catch (Exception ex) { Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex); } }Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (2)
91-91: Consider making_BrowserHelperfieldprivateandreadonlyThe
_BrowserHelperfield can be marked asprivateandreadonlyto encapsulate it properly and prevent unintended modifications.Apply the following change:
- BrowserHelper _BrowserHelper = new BrowserHelper(); + private readonly BrowserHelper _BrowserHelper = new BrowserHelper();
11023-11024: Avoid exposing full file paths in logs to prevent security risksIncluding full file paths in
ExInfoand return parameters may expose sensitive information about the file system structure.[security]
Modify the code to log only the file names:
// In StopMonitoringNetworkLog method act.AddOrUpdateReturnParamActual("Raw Request", Newtonsoft.Json.JsonConvert.SerializeObject(networkRequestLogList.Select(x => x.Item2).ToList(), Formatting.Indented)); string requestPath = _BrowserHelper.CreateNetworkLogFile("NetworklogRequest", networkRequestLogList); - act.ExInfo = $"RequestFile : { requestPath }\n"; + act.ExInfo = $"RequestFile : { Path.GetFileName(requestPath) }\n"; string responsePath = _BrowserHelper.CreateNetworkLogFile("NetworklogResponse", networkResponseLogList); - act.ExInfo = $"{act.ExInfo} ResponseFile : {responsePath}\n"; + act.ExInfo = $"{act.ExInfo} ResponseFile : { Path.GetFileName(responsePath) }\n"; // Additionally, add the files as artifacts Act.AddArtifactToAction(Path.GetFileName(requestPath), act, requestPath); Act.AddArtifactToAction(Path.GetFileName(responsePath), act, responsePath);Also applies to: 11038-11041
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
Ginger/GingerCoreNET/AnalyzerLib/AnalyzeAction.cs(15 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActBrowserElementHandler.cs(3 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/BrowserHelper.cs(1 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs(4 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs(1 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs(5 hunks)
🔇 Additional comments (2)
Ginger/GingerCoreNET/AnalyzerLib/AnalyzeAction.cs (1)
73-74: LGTM: String interpolation improvements enhance readability
The conversion from string concatenation to string interpolation makes the code more readable and maintainable while maintaining the same functionality.
Also applies to: 104-105, 133-134, 153-154, 175-176, 195-196, 317-319, 344-345, 362-364, 382-384, 414-415, 431-432, 448-449, 467-468, 589-589
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs (1)
1813-1819: Ensure handling of newly added actions in ActBrowserElementHandler
The new browser actions added to ActBrowserElementSupportedOperations need to have corresponding implementations in ActBrowserElementHandler to function correctly during execution.
Run the following script to verify that each new action is properly handled in ActBrowserElementHandler:
✅ Verification successful
All new browser actions are properly handled in ActBrowserElementHandler
Based on the verification results, all the newly added actions in ActBrowserElement.eControlAction have corresponding case handlers in ActBrowserElementHandler.cs:
AcceptMessageBox→HandleAcceptMessageBoxOperationAsync()DismissMessageBox→HandleDismissMessageBoxOperationAsync()GetMessageBoxText→HandleGetMessageBoxTextOperation()SetAlertBoxText→HandleSetMessageBoxTextOperationAsync()StartMonitoringNetworkLog→HandleStartMonitoringNetworkLogOperationAsync()GetNetworkLog→HandleGetNetworkLogOperationAsync()StopMonitoringNetworkLog→HandleStopMonitoringNetworkLogOperationAsync()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the new ActBrowserElement.eControlAction actions are handled in ActBrowserElementHandler.
actions=(
"AcceptMessageBox"
"DismissMessageBox"
"GetMessageBoxText"
"SetAlertBoxText"
"StartMonitoringNetworkLog"
"GetNetworkLog"
"StopMonitoringNetworkLog"
)
# Search for each action in ActBrowserElementHandler.cs
for action in "${actions[@]}"; do
echo "Verifying handling of $action..."
rg "case ActBrowserElement.eControlAction.$action" -A 5 --glob "**/ActBrowserElementHandler.cs" || \
echo "Handling for $action not found in ActBrowserElementHandler."
done
Length of output: 8290
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActBrowserElementHandler.cs
Outdated
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs
Outdated
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs
Outdated
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs
Outdated
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightDriver.cs
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (3)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActBrowserElementHandler.cs (2)
586-587: Remove unnecessary return statements.The return statements after error handling in void async methods are unnecessary.
Remove these return statements as they don't serve any purpose in async void methods.
Also applies to: 604-605
571-574: Improve XML documentation formatting.The XML documentation blocks have inconsistent line breaks and some have trailing spaces. Consider standardizing the format across all methods.
Example format to follow:
/// <summary> /// Accepts a message box in the current browser tab. /// If an error occurs, it logs the error and updates the Error property. /// </summary>Also applies to: 589-592, 607-611, 626-631, 644-648, 662-665, 680-683
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (1)
Line range hint
73-891: Consider architectural improvements for better maintainability
- Split the class into smaller interfaces following Interface Segregation Principle:
- INetworkMonitor
- IDialogHandler
- IBrowserNavigator
- Consider using a thread-safe collection like ConcurrentBag for network logs
- Implement a consistent error handling strategy across all methods
- Add unit tests for the new functionality
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActBrowserElementHandler.cs(3 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs(4 hunks)
🔇 Additional comments (3)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (3)
773-801:
Fix incorrect response body check
The response body check in OnNetworkResponseReceived is incorrect:
BodyAsync()returns a Task which will never be null- The body should be awaited and checked for content
Modify the check:
- if (response.BodyAsync() != null)
+ var body = await response.BodyAsync();
+ if (body != null && body.Length > 0)812-824:
Remove unnecessary Task.Run and improve error handling
The OnPlaywrightDialog method unnecessarily wraps a simple assignment in Task.Run:
696-735:
Fix async void and improve error handling
The StopCaptureNetworkLog method has critical issues:
- Using async void is dangerous as it can't be awaited and exceptions can't be caught
- Unnecessary use of Task.Run
- No null checks
Change the method signature and improve error handling:
- public async void StopCaptureNetworkLog(ActBrowserElement act)
+ public async Task StopCaptureNetworkLog(ActBrowserElement act)
{
+ ArgumentNullException.ThrowIfNull(act);
try
{
- await Task.Run(() =>
- {
- _act = act;
- _playwrightPage.Request -= OnNetworkRequestSent;
- _playwrightPage.Response -= OnNetworkResponseReceived;
- isNetworkLogMonitoringStarted = false;
+ _act = act;
+ _playwrightPage.Request -= OnNetworkRequestSent;
+ _playwrightPage.Response -= OnNetworkResponseReceived;
+ _isNetworkLogMonitoringStarted = false;
// ... rest of the code ...
- });
}
catch (Exception ex)
{
Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex);
+ throw;
}
}Likely invalid or redundant comment.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActBrowserElementHandler.cs
Outdated
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActBrowserElementHandler.cs
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActBrowserElementHandler.cs
Outdated
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActBrowserElementHandler.cs
Outdated
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActBrowserElementHandler.cs
Outdated
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs
Outdated
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs
Outdated
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActBrowserElementHandler.cs (3)
571-625: Improve XML documentation for message box handling methods.The XML documentation is missing or incomplete for the following elements:
- Return value documentation (
<returns>tag) is empty or missing- Parameter documentation for
MessageBoxTextparameterApply this pattern to all message box handling methods:
/// <summary> /// This method retrieves the text of the message box from the current browser tab. /// If an error occurs, it logs the error and returns an empty string. /// </summary> +/// <returns>The text content of the message box, or empty string if an error occurs.</returns> private string HandleGetMessageBoxTextOperation()
576-643: Standardize error handling pattern across message box methods.Some methods have unnecessary
returnstatements in catch blocks while others don't. This creates inconsistency in the error handling pattern.Remove unnecessary
returnstatements from catch blocks inHandleAcceptMessageBoxOperationAsyncandHandleDismissMessageBoxOperationAsyncto match the pattern used inHandleSetMessageBoxTextOperationAsync.
644-697: Enhance network log monitoring documentation and error handling.
- The XML documentation should provide more details about:
- The format of captured network logs
- Where logs are stored
- Any size limitations
- The error handling pattern with
returnstatements in catch blocks is inconsistent with other methods.Consider adding more detailed documentation and standardizing error handling:
/// <summary> /// This asynchronous method starts monitoring and capturing network logs in the current browser tab. If an error occurs, /// it logs the error and updates the Error property. /// </summary> +/// <remarks> +/// Network logs are captured in HAR format and include: +/// - Request/response headers and bodies +/// - Timing information +/// - Resource types +/// Logs are stored in memory until retrieved via GetNetworkLog or cleared via StopMonitoringNetworkLog. +/// </remarks> /// <returns></returns> private async Task HandleStartMonitoringNetworkLogOperationAsync()Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (1)
769-807: Simplify network response handling logic.The OnNetworkResponseReceived method has complex nested conditions that make it hard to maintain.
Consider extracting the response type check and URL monitoring into separate methods:
private bool ShouldMonitorResponse(IResponse response) { if (!_isNetworkLogMonitoringStarted || _act == null) return false; var monitorType = _act.GetOrCreateInputParam(nameof(ActBrowserElement.eMonitorUrl)).Value; if (!(_browserHelper.IsToMonitorAllUrls(_act) || _browserHelper.IsToMonitorOnlySelectedUrls(_act, response.Url))) return false; var requestType = _act.GetOrCreateInputParam(nameof(ActBrowserElement.eRequestTypes)).Value; if (requestType == ActBrowserElement.eRequestTypes.FetchOrXHR.ToString()) { return response.Request.ResourceType.Equals("XHR", StringComparison.CurrentCultureIgnoreCase) || response.Request.ResourceType.Equals("FETCH", StringComparison.CurrentCultureIgnoreCase); } return true; }Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (2)
10938-10948: Reduce code duplication in log processingThe same log processing logic is duplicated in GetNetworkLogAsync and StopMonitoringNetworkLog methods. Consider extracting to a reusable method.
+ private void ProcessNetworkLogs(ActBrowserElement act) + { + act.AddOrUpdateReturnParamActual("Raw Request", JsonConvert.SerializeObject(networkRequestLogList.Select(x => x.Item2).ToList())); + act.AddOrUpdateReturnParamActual("Raw Response", JsonConvert.SerializeObject(networkResponseLogList.Select(x => x.Item2).ToList())); + // Process request logs + foreach (var val in networkRequestLogList) + { + act.AddOrUpdateReturnParamActual($"{act.ControlAction} {val.Item1}", Convert.ToString(val.Item2)); + } + // Process response logs + foreach (var val in networkResponseLogList) + { + act.AddOrUpdateReturnParamActual($"{act.ControlAction} {val.Item1}", Convert.ToString(val.Item2)); + } + }Also applies to: 11020-11029
91-91: Consider making BrowserHelper readonlyThe BrowserHelper instance should be marked as readonly since it's initialized only once.
- BrowserHelper _BrowserHelper = new BrowserHelper(); + private readonly BrowserHelper _BrowserHelper = new BrowserHelper();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActBrowserElementHandler.cs(3 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs(4 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs(5 hunks)
🧰 Additional context used
📓 Learnings (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (1)
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4010
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs:773-801
Timestamp: 2024-11-28T14:05:03.513Z
Learning: In `PlaywrightBrowserTab.OnNetworkResponseReceived`, it's not necessary to call `Response.BodyAsync()`.
🔇 Additional comments (5)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (3)
875-895: 🛠️ Refactor suggestion
Improve error handling in GetMessageBoxText.
The method swallows exceptions and returns a generic error message, which could hide important issues.
public string GetMessageBoxText()
{
try
{
- if (dialogs != null)
+ if (_dialogs == null)
{
- return dialogs.Message;
+ throw new InvalidOperationException("No active dialog available");
}
- else
- {
- Reporter.ToLog(eLogLevel.WARN, "No dialog to get message.");
- return string.Empty;
- }
+ return _dialogs.Message;
}
catch (Exception ex)
{
Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex);
- return "Error While Get Message Box Text";
+ throw;
}
}Likely invalid or redundant comment.
74-82: 🛠️ Refactor suggestion
Follow C# naming conventions and encapsulation principles.
The field declarations need improvements for better maintainability and thread safety:
Apply these changes:
-List<Tuple<string, object>> networkResponseLogList;
-List<Tuple<string, object>> networkRequestLogList;
-ObservableList<IResponse> NetworkResponseList;
-ActBrowserElement _act;
-public bool isNetworkLogMonitoringStarted = false;
-IDialog dialogs;
-BrowserHelper _BrowserHelper;
+private readonly List<Tuple<string, object>> _networkResponseLogList = new();
+private readonly List<Tuple<string, object>> _networkRequestLogList = new();
+private readonly ObservableList<IResponse> _networkResponseList = new();
+private ActBrowserElement? _act;
+private bool _isNetworkLogMonitoringStarted;
+private IDialog? _dialogs;
+private readonly BrowserHelper _browserHelper;Likely invalid or redundant comment.
631-651: 🛠️ Refactor suggestion
Remove unnecessary Task.Run and improve error handling.
The StartCaptureNetworkLog method has several issues:
- Using Task.Run for synchronous operations is unnecessary
- Missing null check for act parameter
- Exception handling swallows errors
Apply these changes:
-public async Task StartCaptureNetworkLog(ActBrowserElement act)
+public Task StartCaptureNetworkLog(ActBrowserElement act)
{
+ ArgumentNullException.ThrowIfNull(act);
try
{
- await Task.Run(() =>
- {
- _act = act;
- networkRequestLogList = new List<Tuple<string, object>>();
- networkResponseLogList = new List<Tuple<string, object>>();
- NetworkResponseList = new ObservableList<IResponse>();
- _playwrightPage.Request += OnNetworkRequestSent;
- _playwrightPage.Response += OnNetworkResponseReceived;
- isNetworkLogMonitoringStarted = true;
- });
+ _act = act;
+ _networkRequestLogList.Clear();
+ _networkResponseLogList.Clear();
+ _networkResponseList.Clear();
+ _playwrightPage.Request += OnNetworkRequestSent;
+ _playwrightPage.Response += OnNetworkResponseReceived;
+ _isNetworkLogMonitoringStarted = true;
+ return Task.CompletedTask;
}
catch(Exception ex)
{
Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex);
+ throw;
}
}Likely invalid or redundant comment.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (2)
10934-10960:
Consider adding PII data filtering
The network log capture may contain sensitive information like authentication tokens, personal data etc. Consider adding a mechanism to filter/mask sensitive data before storing logs.
[security]
Consider adding data filtering:
- act.AddOrUpdateReturnParamActual("Raw Request", Newtonsoft.Json.JsonConvert.SerializeObject(networkRequestLogList.Select(x => x.Item2).ToList()));
+ // Filter sensitive data before serializing
+ var filteredRequests = networkRequestLogList.Select(x => FilterSensitiveData(x.Item2)).ToList();
+ act.AddOrUpdateReturnParamActual("Raw Request", Newtonsoft.Json.JsonConvert.SerializeObject(filteredRequests));Likely invalid or redundant comment.
11020-11038: 🛠️ Refactor suggestion
Validate file paths before writing logs
The code writes network logs to files without proper path validation which could lead to security issues. Consider adding path validation and sanitization.
Add path validation:
+ // Validate and sanitize file paths
+ if (!_BrowserHelper.IsValidFilePath(requestPath) || !_BrowserHelper.IsValidFilePath(responsePath))
+ {
+ throw new SecurityException("Invalid file path detected");
+ }Likely invalid or redundant comment.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs
Outdated
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/BrowserHelper.cs(1 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs(4 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs(8 hunks)
🧰 Additional context used
📓 Learnings (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (1)
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4010
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs:773-801
Timestamp: 2024-11-28T14:05:03.513Z
Learning: In `PlaywrightBrowserTab.OnNetworkResponseReceived`, it's not necessary to call `Response.BodyAsync()`.
🔇 Additional comments (10)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/BrowserHelper.cs (3)
15-20:
Remove empty constructor in favor of dependency injection
The constructor should validate its input parameter to prevent null reference exceptions.
Apply this diff:
- internal class BrowserHelper
+ public class BrowserHelper
{
private readonly ActBrowserElement _act;
public BrowserHelper(ActBrowserElement act) {
- _act = act;
+ _act = act ?? throw new ArgumentNullException(nameof(act));
}
}22-32: 🛠️ Refactor suggestion
Refactor URL monitoring methods for better reliability
The current implementation has inefficient string comparisons and lacks proper input validation.
Apply this diff:
public bool ShouldMonitorUrl(string requestUrl)
{
+ if (string.IsNullOrEmpty(requestUrl))
+ return false;
+
return _act.GetOrCreateInputParam(nameof(ActBrowserElement.eMonitorUrl)).Value == nameof(ActBrowserElement.eMonitorUrl.SelectedUrl)
&& _act.UpdateOperationInputValues != null
- && _act.UpdateOperationInputValues.Any(x => !string.IsNullOrEmpty(x.ValueForDriver) && requestUrl.ToLower().Contains(x.ValueForDriver.ToLower()));
+ && _act.UpdateOperationInputValues.Any(x => !string.IsNullOrEmpty(x.ValueForDriver)
+ && requestUrl.Contains(x.ValueForDriver, StringComparison.OrdinalIgnoreCase));34-73:
Critical improvements needed in network log file creation
The current implementation has several critical issues:
- Input validation logs but doesn't prevent execution
- Inefficient and unsafe file path construction
- Unnecessary file operations
- Improper error handling
Apply this improved implementation:
public string CreateNetworkLogFile(string Filename, List<Tuple<string, object>> networkLogList)
{
- if (string.IsNullOrEmpty(Filename))
- {
- Reporter.ToLog(eLogLevel.INFO, $"Method - {MethodBase.GetCurrentMethod().Name}, Filename should not be empty");
- }
-
- if (networkLogList == null)
- {
- Reporter.ToLog(eLogLevel.INFO, $"Method - {MethodBase.GetCurrentMethod().Name}, networkLogList should not be empty");
- }
- string FullFilePath = string.Empty;
+ if (string.IsNullOrEmpty(Filename))
+ throw new ArgumentNullException(nameof(Filename));
+ if (networkLogList == null)
+ throw new ArgumentNullException(nameof(networkLogList));
try
{
- string FullDirectoryPath = System.IO.Path.Combine(WorkSpace.Instance.Solution.Folder, "Documents", "NetworkLog");
- if (!System.IO.Directory.Exists(FullDirectoryPath))
- {
- System.IO.Directory.CreateDirectory(FullDirectoryPath);
- }
-
- FullFilePath = FullDirectoryPath + @"\" + Filename + DateTime.Now.Day.ToString() + "_" + DateTime.Now.Month.ToString() + "_" + DateTime.Now.Year.ToString() + "_" + DateTime.Now.Millisecond.ToString() + ".har";
- if (!System.IO.File.Exists(FullFilePath))
- {
- string FileContent = JsonConvert.SerializeObject(networkLogList.Select(x => x.Item2).ToList());
-
- using (Stream fileStream = System.IO.File.Create(FullFilePath))
- {
- fileStream.Close();
- }
- System.IO.File.WriteAllText(FullFilePath, FileContent);
- }
+ string directoryPath = Path.Combine(WorkSpace.Instance.Solution.Folder, "Documents", "NetworkLog");
+ Directory.CreateDirectory(directoryPath);
+
+ string timestamp = DateTime.Now.ToString("yyyyMMdd_HHmmss");
+ string fullPath = Path.Combine(directoryPath, $"{Filename}_{timestamp}.har");
+
+ string fileContent = JsonConvert.SerializeObject(
+ networkLogList.Select(x => x.Item2).ToList(),
+ Formatting.Indented
+ );
+
+ File.WriteAllText(fullPath, fileContent);
+ return fullPath;
}
catch(Exception ex)
{
- Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name} Error: {ex.Message}", ex);
+ var message = $"Failed to create network log file: {ex.Message}";
+ Reporter.ToLog(eLogLevel.ERROR, message, ex);
+ throw new IOException(message, ex);
}
- return FullFilePath;
}Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (3)
815-825: 🛠️ Refactor suggestion
Fix async void event handler.
The dialog event handler has unnecessary async modifier and improper exception handling.
Apply these changes:
-private async void OnPlaywrightDialog(object? sender, IDialog e)
+private void OnPlaywrightDialog(object? sender, IDialog e)
{
+ ArgumentNullException.ThrowIfNull(e);
try
{
- dialogs = e;
+ _dialogs = e;
}
catch (Exception ex)
{
Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex);
+ throw;
}
}Likely invalid or redundant comment.
74-82: 🛠️ Refactor suggestion
Follow C# naming conventions and improve field encapsulation.
The field declarations need improvements:
- Fields lack proper access modifiers
- Public fields should be properties
- Collections should be initialized in constructor
- Field names don't follow C# naming conventions
Apply these changes:
-List<Tuple<string, object>> networkResponseLogList;
-List<Tuple<string, object>> networkRequestLogList;
-ObservableList<IResponse> NetworkResponseList;
-ActBrowserElement _act;
-public bool isNetworkLogMonitoringStarted = false;
-IDialog dialogs;
-BrowserHelper _BrowserHelper;
+private readonly List<Tuple<string, object>> _networkResponseLogList = new();
+private readonly List<Tuple<string, object>> _networkRequestLogList = new();
+private readonly ObservableList<IResponse> _networkResponseList = new();
+private ActBrowserElement? _act;
+private bool _isNetworkLogMonitoringStarted;
+private IDialog? _dialogs;
+private readonly BrowserHelper _browserHelper;Likely invalid or redundant comment.
830-922: 🛠️ Refactor suggestion
Improve dialog operation methods.
The dialog operation methods need improvements in:
- Consistent null checking
- Exception handling
- Return values
Consider refactoring the methods:
public async Task AcceptMessageBox()
{
+ if (_dialogs == null)
+ {
+ throw new InvalidOperationException("No active dialog available");
+ }
try
{
- if (dialogs != null)
- {
- await dialogs.AcceptAsync();
- }
- else
- {
- Reporter.ToLog(eLogLevel.WARN, "No dialog to accept.");
- }
+ await _dialogs.AcceptAsync();
}
catch (Exception ex)
{
Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex);
+ throw;
}
}
public string GetMessageBoxText()
{
+ if (_dialogs == null)
+ {
+ throw new InvalidOperationException("No active dialog available");
+ }
try
{
- if (dialogs != null)
- {
- return dialogs.Message;
- }
- else
- {
- Reporter.ToLog(eLogLevel.WARN, "No dialog to get message.");
- return string.Empty;
- }
+ return _dialogs.Message;
}
catch (Exception ex)
{
Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex);
- return "Error While Get Message Box Text";
+ throw;
}
}Similar changes should be applied to DismissMessageBox and SetMessageBoxText methods.
Likely invalid or redundant comment.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (4)
8394-8394: LGTM: Good refactoring to extract helper functionality
The initialization of BrowserHelper class helps improve code organization and maintainability by extracting helper methods.
11096-11100: 🛠️ Refactor suggestion
Improve error handling in network request monitoring
The error handling in OnNetworkRequestSent could be improved to provide more context and proper cleanup.
try
{
if (_BrowserHelper.ShouldMonitorAllUrls() || _BrowserHelper.ShouldMonitorUrl(e.RequestUrl))
{
networkRequestLogList.Add(new Tuple<string, object>($"RequestUrl: {e.RequestUrl}", JsonConvert.SerializeObject(e, Formatting.Indented)));
}
}
catch (Exception ex)
{
- Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex);
+ Reporter.ToLog(eLogLevel.ERROR, $"Failed to monitor network request for URL {e?.RequestUrl}. Error: {ex.Message}", ex);
+ // Consider cleanup/recovery steps
+ networkRequestLogList.Clear();
+ throw; // Re-throw to handle at higher level if needed
}Likely invalid or redundant comment.
10977-10981: 🛠️ Refactor suggestion
Add input validation for operation values
The code processes input values without validation. Consider adding validation to prevent potential issues.
foreach (ActInputValue item in mAct.UpdateOperationInputValues)
{
+ if (string.IsNullOrEmpty(item.Param))
+ {
+ throw new ArgumentException($"Invalid input parameter: {item.Name}");
+ }
item.Value = item.Param;
item.ValueForDriver = VE.Calculate(item.Param);
}Likely invalid or redundant comment.
10896-10898: Verify DevTools version upgrade compatibility
The DevTools version has been upgraded from V125 to V127. While this keeps the code up-to-date, ensure all dependent functionality remains compatible.
✅ Verification successful
Let me gather more information to verify if all DevTools version references have been properly updated.
Let me check for any potential version-specific imports or usages that might have been missed.
DevTools version upgrade is consistently implemented
All DevTools references have been properly updated to V127, and there are no remaining V125 references in the codebase except for a commented-out portion in the import statement. The upgrade appears to be complete and consistent across all usage points.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any V125 references that may have been missed
rg "V125" --type cs
Length of output: 201
Script:
#!/bin/bash
# Search for any DevTools related imports and usages
rg "DevTools" --type cs -A 2
Length of output: 5539
Script:
#!/bin/bash
# Search for any version-specific Selenium DevTools references
rg "OpenQA\.Selenium\.DevTools\.V\d+" --type cs
Length of output: 1145
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs
Outdated
Show resolved
Hide resolved
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActBrowserElementHandler.cs (2)
652-652: Fix typographical error in error messagesThere is an extra space between "log" and "operation" in the error messages, which should be corrected for clarity.
Apply this diff:
- _act.Error = $"Error when handling Network log operation."; + _act.Error = $"Error when handling Network log operation.";This applies to lines 652, 670, and 688.
Also applies to: 670-670, 688-688
626-626: Add parameter description in XML documentationThe
<param>tag forMessageBoxTextlacks a description in the XML documentation. Providing a brief description improves code readability and maintenance.Apply this diff:
/// <param name="MessageBoxText"></param> +/// <param name="MessageBoxText">The text to set in the message box.</param>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActBrowserElementHandler.cs(3 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs(4 hunks)
🧰 Additional context used
📓 Learnings (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (1)
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4010
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs:773-801
Timestamp: 2024-11-28T14:05:03.513Z
Learning: In `PlaywrightBrowserTab.OnNetworkResponseReceived`, it's not necessary to call `Response.BodyAsync()`.
🔇 Additional comments (4)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (3)
74-82: Verify field declarations implementation.
The field declarations still need to be updated according to C# conventions and best practices:
- Add access modifiers to all fields
- Use proper C# naming conventions (camelCase for private fields)
- Initialize collections in constructor
- Mark appropriate fields as readonly
#!/bin/bash
# Description: Check if the field declarations have been updated in the latest commit
git show HEAD:Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs | grep -A 10 "List<Tuple<string, object>> networkResponseLogList"812-822: 🛠️ Refactor suggestion
Fix async void event handler.
The OnPlaywrightDialog method has unnecessary async modifier and swallows exceptions.
Apply this diff:
-private async void OnPlaywrightDialog(object? sender, IDialog e)
+private void OnPlaywrightDialog(object? sender, IDialog e)
{
try
{
- dialogs = e;
+ _dialogs = e;
}
catch (Exception ex)
{
Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex);
+ throw;
}
}630-648: 🛠️ Refactor suggestion
Improve StartCaptureNetworkLog implementation.
The method has several issues:
- BrowserHelper is reinitialized on each call
- Exception is swallowed
- Method is marked async but doesn't use await
Apply this diff:
-public async Task StartCaptureNetworkLog(ActBrowserElement act)
+public Task StartCaptureNetworkLog(ActBrowserElement act)
{
+ ArgumentNullException.ThrowIfNull(act);
_act = act;
- _BrowserHelper = new BrowserHelper(act);
try
{
networkRequestLogList = new List<Tuple<string, object>>();
networkResponseLogList = new List<Tuple<string, object>>();
NetworkResponseList = new ObservableList<IResponse>();
_playwrightPage.Request += OnNetworkRequestSent;
_playwrightPage.Response += OnNetworkResponseReceived;
isNetworkLogMonitoringStarted = true;
+ return Task.CompletedTask;
}
catch(Exception ex)
{
Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex);
+ throw;
}
}Likely invalid or redundant comment.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/ActionHandlers/ActBrowserElementHandler.cs (1)
575-575:
Ensure safe type casting when accessing PlaywrightBrowserTab
The code performs unsafe casting of _browser.CurrentWindow.CurrentTab to PlaywrightBrowserTab without verifying the type, which can lead to runtime exceptions if the current tab is not a PlaywrightBrowserTab.
Consider adding type checking to prevent potential runtime errors. For example:
- await ((PlaywrightBrowserTab)_browser!.CurrentWindow.CurrentTab).AcceptMessageBox();
+ if (_browser?.CurrentWindow.CurrentTab is PlaywrightBrowserTab playwrightTab)
+ {
+ await playwrightTab.AcceptMessageBox();
+ }
+ else
+ {
+ throw new InvalidOperationException("Current tab is not a Playwright tab.");
+ }This update applies to all instances where the cast occurs without type checking.
Also applies to: 593-593, 612-612, 631-631, 648-648, 666-666, 684-684
| public async Task GetCaptureNetworkLog(ActBrowserElement act) | ||
| { | ||
| _act = act; | ||
| _BrowserHelper = new BrowserHelper(act); | ||
| try | ||
| { | ||
| await Task.Run(() => | ||
| { | ||
| if (isNetworkLogMonitoringStarted) | ||
| { | ||
| act.AddOrUpdateReturnParamActual("Raw Request", Newtonsoft.Json.JsonConvert.SerializeObject(networkRequestLogList.Select(x => x.Item2).ToList(), Formatting.Indented)); | ||
| act.AddOrUpdateReturnParamActual("Raw Response", Newtonsoft.Json.JsonConvert.SerializeObject(networkResponseLogList.Select(x => x.Item2).ToList(), Formatting.Indented)); | ||
| foreach (var val in networkRequestLogList.ToList()) | ||
| { | ||
| act.AddOrUpdateReturnParamActual(nameof(act.ControlAction) + " " + val.Item1.ToString(), Convert.ToString(val.Item2)); | ||
| } | ||
|
|
||
| foreach (var val in networkResponseLogList.ToList()) | ||
| { | ||
| act.AddOrUpdateReturnParamActual(nameof(act.ControlAction) + " " + val.Item1.ToString(), Convert.ToString(val.Item2)); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| act.ExInfo = $"Action is skipped,{nameof(ActBrowserElement.eControlAction.StartMonitoringNetworkLog)} Action is not started"; | ||
| act.Status = Amdocs.Ginger.CoreNET.Execution.eRunStatus.Skipped; | ||
| } | ||
| }); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract common log processing logic.
GetCaptureNetworkLog and StopCaptureNetworkLog share significant code duplication in processing network logs.
Create a private method to handle common log processing:
private void ProcessNetworkLogs(ActBrowserElement act, bool createFiles = false)
{
if (!isNetworkLogMonitoringStarted)
{
act.ExInfo = $"Action is skipped, {nameof(ActBrowserElement.eControlAction.StartMonitoringNetworkLog)} Action is not started";
act.Status = Amdocs.Ginger.CoreNET.Execution.eRunStatus.Skipped;
return;
}
act.AddOrUpdateReturnParamActual("Raw Request", JsonConvert.SerializeObject(networkRequestLogList.Select(x => x.Item2).ToList(), Formatting.Indented));
act.AddOrUpdateReturnParamActual("Raw Response", JsonConvert.SerializeObject(networkResponseLogList.Select(x => x.Item2).ToList(), Formatting.Indented));
foreach (var val in networkRequestLogList)
{
act.AddOrUpdateReturnParamActual($"{nameof(act.ControlAction)} {val.Item1}", Convert.ToString(val.Item2));
}
foreach (var val in networkResponseLogList)
{
act.AddOrUpdateReturnParamActual($"{nameof(act.ControlAction)} {val.Item1}", Convert.ToString(val.Item2));
}
if (createFiles)
{
string requestPath = _browserHelper.CreateNetworkLogFile("NetworklogRequest", networkRequestLogList);
string responsePath = _browserHelper.CreateNetworkLogFile("NetworklogResponse", networkResponseLogList);
act.ExInfo = $"RequestFile: {requestPath}\nResponseFile: {responsePath}";
act.AddOrUpdateReturnParamActual("RequestFile", requestPath);
act.AddOrUpdateReturnParamActual("ResponseFile", responsePath);
Act.AddArtifactToAction(Path.GetFileName(requestPath), act, requestPath);
Act.AddArtifactToAction(Path.GetFileName(responsePath), act, responsePath);
}
}Then simplify both methods:
public Task GetCaptureNetworkLog(ActBrowserElement act)
{
ArgumentNullException.ThrowIfNull(act);
try
{
ProcessNetworkLogs(act);
return Task.CompletedTask;
}
catch (Exception ex)
{
Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex);
throw;
}
}
public Task StopCaptureNetworkLog(ActBrowserElement act)
{
ArgumentNullException.ThrowIfNull(act);
try
{
_playwrightPage.Request -= OnNetworkRequestSent;
_playwrightPage.Response -= OnNetworkResponseReceived;
isNetworkLogMonitoringStarted = false;
ProcessNetworkLogs(act, createFiles: true);
return Task.CompletedTask;
}
catch (Exception ex)
{
Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex);
throw;
}
}Also applies to: 695-734
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (1)
620-808: Consider extracting network logging to a separate class.The network logging functionality has grown complex enough to warrant its own class. This would:
- Improve separation of concerns
- Make the code more maintainable
- Reduce the complexity of the PlaywrightBrowserTab class
Consider creating a
NetworkLoggerclass that encapsulates all network logging functionality, including:
- Log collection and storage
- Event handling
- File creation
- URL monitoring
This would make the code more modular and easier to test.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (1)
11096-11134: Improve network monitoring logicThe URL monitoring logic has been updated but could be optimized:
- Consider caching the monitoring type check result
- Add request/response size limits
+ private const int MAX_LOG_SIZE = 5 * 1024 * 1024; // 5MB limit + private string cachedMonitorType; private void OnNetworkResponseReceived(object sender, NetworkResponseReceivedEventArgs e) { try { - string monitorType = mAct.GetOrCreateInputParam(nameof(ActBrowserElement.eMonitorUrl)).Value; + cachedMonitorType ??= mAct.GetOrCreateInputParam(nameof(ActBrowserElement.eMonitorUrl)).Value; + var responseSize = JsonConvert.SerializeObject(e).Length; + if (responseSize > MAX_LOG_SIZE) + { + Reporter.ToLog(eLogLevel.WARN, $"Response size {responseSize} exceeds limit of {MAX_LOG_SIZE}"); + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/BrowserHelper.cs(1 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs(4 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/BrowserHelper.cs
🧰 Additional context used
📓 Learnings (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (1)
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4010
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs:773-801
Timestamp: 2024-11-28T14:05:03.513Z
Learning: In `PlaywrightBrowserTab.OnNetworkResponseReceived`, it's not necessary to call `Response.BodyAsync()`.
🔇 Additional comments (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (1)
814-824: 🛠️ Refactor suggestion
Fix async void event handler and improve dialog assignment.
The dialog event handler has several issues:
- Using async void unnecessarily
- Inconsistent field naming
- Swallowing exceptions
-private async void OnPlaywrightDialog(object? sender, IDialog e)
+private void OnPlaywrightDialog(object? sender, IDialog e)
{
+ ArgumentNullException.ThrowIfNull(e);
try
{
- dialogs = e;
+ _dialogs = e;
}
catch (Exception ex)
{
Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex);
+ throw;
}
}Likely invalid or redundant comment.
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (1)
74-74: DevTools version upgrade requires validation
The upgrade from V125 to V127 of Selenium DevTools may introduce breaking changes. Please ensure all DevTools functionality has been tested, especially network monitoring features.
Also applies to: 10896-10898
✅ Verification successful
Let me gather more information about the DevTools usage to ensure there are no other potential version-related issues.
DevTools version upgrade is properly implemented
The verification shows that the upgrade from V125 to V127 has been consistently implemented across the codebase. All DevTools-related code is properly using V127, including:
- The DevTools session initialization with correct version:
devToolsSession = devTools.GetDevToolsSession(127) - The version-specific domains usage:
DevToolsDomains = OpenQA.Selenium.DevTools.V127.DevToolsSessionDomains - Network commands with V127 namespace:
OpenQA.Selenium.DevTools.V127.Network.EnableCommandSettings()
The commented-out V125 reference is just a historical comment and doesn't affect the code execution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any V125 references that might have been missed
rg "V125" --type cs
Length of output: 201
Script:
#!/bin/bash
# Search for DevTools related usage
ast-grep --pattern 'OpenQA.Selenium.DevTools'
# Also check for any DevToolsSession usage
rg "DevToolsSession" --type cs -A 2
Length of output: 7693
| public async Task AcceptMessageBox() | ||
| { | ||
| try | ||
| { | ||
| if (dialogs != null) | ||
| { | ||
| await dialogs.AcceptAsync(); | ||
| } | ||
| else | ||
| { | ||
| Reporter.ToLog(eLogLevel.WARN, "No dialog to accept."); | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex); | ||
| } | ||
|
|
||
| } | ||
| /// <summary> | ||
| /// This asynchronous method dismisses (closes or cancels) a message box or dialog. | ||
| /// </summary> | ||
| /// <returns></returns> | ||
| public async Task DismissMessageBox() | ||
| { | ||
| try | ||
| { | ||
| if (dialogs != null) | ||
| { | ||
| await dialogs.DismissAsync(); | ||
| } | ||
| else | ||
| { | ||
| Reporter.ToLog(eLogLevel.WARN, "No dialog to dismiss."); | ||
| } | ||
|
|
||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex); | ||
| } | ||
|
|
||
| } | ||
| /// <summary> | ||
| /// This method retrieves the text content of the current message box or dialog. | ||
| /// </summary> | ||
| /// <returns></returns> | ||
| public string GetMessageBoxText() | ||
| { | ||
| try | ||
| { | ||
| if (dialogs != null) | ||
| { | ||
| return dialogs.Message; | ||
| } | ||
| else | ||
| { | ||
| Reporter.ToLog(eLogLevel.WARN, "No dialog to get message."); | ||
| return string.Empty; | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex); | ||
| return "Error While Get Message Box Text"; | ||
| } | ||
|
|
||
| } | ||
| /// <summary> | ||
| /// This asynchronous method sets the text (usually for a prompt dialog) in the message box. | ||
| /// </summary> | ||
| /// <param name="MessageBoxText"></param> | ||
| /// <returns></returns> | ||
| public async Task SetMessageBoxText(string MessageBoxText) | ||
| { | ||
| try | ||
| { | ||
| if (dialogs != null) | ||
| { | ||
| await dialogs.AcceptAsync(promptText: MessageBoxText); | ||
| } | ||
| else | ||
| { | ||
| Reporter.ToLog(eLogLevel.WARN, "No dialog to accept."); | ||
|
|
||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, "Error While Accept Message", ex); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve dialog handling methods.
The dialog operation methods need improvements:
- Extract common dialog validation
- Consistent error handling
- Better error messages
private IDialog GetActiveDialog()
{
if (_dialogs == null)
{
throw new InvalidOperationException("No active dialog available");
}
return _dialogs;
}
public async Task AcceptMessageBox()
{
try
{
await GetActiveDialog().AcceptAsync();
}
catch (Exception ex)
{
Reporter.ToLog(eLogLevel.ERROR, $"Failed to accept dialog: {ex.Message}", ex);
throw;
}
}
public async Task DismissMessageBox()
{
try
{
await GetActiveDialog().DismissAsync();
}
catch (Exception ex)
{
Reporter.ToLog(eLogLevel.ERROR, $"Failed to dismiss dialog: {ex.Message}", ex);
throw;
}
}
public string GetMessageBoxText()
{
try
{
return GetActiveDialog().Message;
}
catch (Exception ex)
{
Reporter.ToLog(eLogLevel.ERROR, $"Failed to get dialog message: {ex.Message}", ex);
throw;
}
}
public async Task SetMessageBoxText(string messageBoxText)
{
ArgumentNullException.ThrowIfNull(messageBoxText);
try
{
await GetActiveDialog().AcceptAsync(promptText: messageBoxText);
}
catch (Exception ex)
{
Reporter.ToLog(eLogLevel.ERROR, $"Failed to set dialog text: {ex.Message}", ex);
throw;
}
}| List<Tuple<string, object>> networkResponseLogList; | ||
| List<Tuple<string, object>> networkRequestLogList; | ||
| ActBrowserElement _act; | ||
| public bool isNetworkLogMonitoringStarted = false; | ||
| IDialog dialogs; | ||
|
|
||
| public bool IsClosed => _isClosed; | ||
| BrowserHelper _BrowserHelper; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve field declarations following C# best practices.
The field declarations need improvements:
- Use readonly modifier for collections
- Follow C# naming conventions (_camelCase for private fields)
- Add proper null annotations
- Initialize collections in constructor
-List<Tuple<string, object>> networkResponseLogList;
-List<Tuple<string, object>> networkRequestLogList;
-ActBrowserElement _act;
-public bool isNetworkLogMonitoringStarted = false;
-IDialog dialogs;
-BrowserHelper _BrowserHelper;
+private readonly List<Tuple<string, object>> _networkResponseLogList = [];
+private readonly List<Tuple<string, object>> _networkRequestLogList = [];
+private ActBrowserElement? _act;
+private bool _isNetworkLogMonitoringStarted;
+private IDialog? _dialogs;
+private readonly BrowserHelper? _browserHelper;📝 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.
| List<Tuple<string, object>> networkResponseLogList; | |
| List<Tuple<string, object>> networkRequestLogList; | |
| ActBrowserElement _act; | |
| public bool isNetworkLogMonitoringStarted = false; | |
| IDialog dialogs; | |
| public bool IsClosed => _isClosed; | |
| BrowserHelper _BrowserHelper; | |
| private readonly List<Tuple<string, object>> _networkResponseLogList = []; | |
| private readonly List<Tuple<string, object>> _networkRequestLogList = []; | |
| private ActBrowserElement? _act; | |
| private bool _isNetworkLogMonitoringStarted; | |
| private IDialog? _dialogs; | |
| public bool IsClosed => _isClosed; | |
| private readonly BrowserHelper? _browserHelper; |
| public async Task StartCaptureNetworkLog(ActBrowserElement act) | ||
| { | ||
| _act = act; | ||
| _BrowserHelper = new BrowserHelper(act); | ||
| try | ||
| { | ||
| networkRequestLogList = []; | ||
| networkResponseLogList = []; | ||
| _playwrightPage.Request += OnNetworkRequestSent; | ||
| _playwrightPage.Response += OnNetworkResponseReceived; | ||
| isNetworkLogMonitoringStarted = true; | ||
| } | ||
| catch(Exception ex) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve StartCaptureNetworkLog implementation.
The method has several issues:
- Missing null check for act parameter
- Unnecessary field reassignment
- Swallowing exceptions
-public async Task StartCaptureNetworkLog(ActBrowserElement act)
+public Task StartCaptureNetworkLog(ActBrowserElement act)
{
+ ArgumentNullException.ThrowIfNull(act);
try
{
- _act = act;
- _BrowserHelper = new BrowserHelper(act);
- networkRequestLogList = [];
- networkResponseLogList = [];
+ _act = act;
+ _browserHelper = new BrowserHelper(act);
+ _networkRequestLogList.Clear();
+ _networkResponseLogList.Clear();
_playwrightPage.Request += OnNetworkRequestSent;
_playwrightPage.Response += OnNetworkResponseReceived;
- isNetworkLogMonitoringStarted = true;
+ _isNetworkLogMonitoringStarted = true;
+ return Task.CompletedTask;
}
catch(Exception ex)
{
Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex);
+ throw;
}
}📝 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 async Task StartCaptureNetworkLog(ActBrowserElement act) | |
| { | |
| _act = act; | |
| _BrowserHelper = new BrowserHelper(act); | |
| try | |
| { | |
| networkRequestLogList = []; | |
| networkResponseLogList = []; | |
| _playwrightPage.Request += OnNetworkRequestSent; | |
| _playwrightPage.Response += OnNetworkResponseReceived; | |
| isNetworkLogMonitoringStarted = true; | |
| } | |
| catch(Exception ex) | |
| { | |
| Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex); | |
| } | |
| } | |
| public Task StartCaptureNetworkLog(ActBrowserElement act) | |
| { | |
| ArgumentNullException.ThrowIfNull(act); | |
| try | |
| { | |
| _act = act; | |
| _browserHelper = new BrowserHelper(act); | |
| _networkRequestLogList.Clear(); | |
| _networkResponseLogList.Clear(); | |
| _playwrightPage.Request += OnNetworkRequestSent; | |
| _playwrightPage.Response += OnNetworkResponseReceived; | |
| _isNetworkLogMonitoringStarted = true; | |
| return Task.CompletedTask; | |
| } | |
| catch(Exception ex) | |
| { | |
| Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex); | |
| throw; | |
| } | |
| } |
| private async void OnNetworkRequestSent(object? sender, IRequest request) | ||
| { | ||
| try | ||
| { | ||
| if (_BrowserHelper.ShouldMonitorAllUrls() || _BrowserHelper.ShouldMonitorUrl( request.Url)) | ||
| { | ||
| networkRequestLogList.Add(new Tuple<string, object>($"RequestUrl:{ request.Url}", JsonConvert.SerializeObject(request, Formatting.Indented, | ||
| new JsonSerializerSettings | ||
| { | ||
| ReferenceLoopHandling = ReferenceLoopHandling.Ignore | ||
| }) | ||
| )); | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix async void event handler and improve error handling.
The event handler has several issues:
- Using async void which can lead to unhandled exceptions
- Missing null checks
- Inconsistent error handling
-private async void OnNetworkRequestSent(object? sender, IRequest request)
+private void OnNetworkRequestSent(object? sender, IRequest request)
{
+ ArgumentNullException.ThrowIfNull(request);
try
{
- if (_BrowserHelper.ShouldMonitorAllUrls() || _BrowserHelper.ShouldMonitorUrl(request.Url))
+ if (!(_browserHelper?.ShouldMonitorAllUrls() ?? false) &&
+ !(_browserHelper?.ShouldMonitorUrl(request.Url) ?? false))
{
- networkRequestLogList.Add(new Tuple<string, object>($"RequestUrl:{request.Url}",
- JsonConvert.SerializeObject(request, Formatting.Indented,
- new JsonSerializerSettings
- {
- ReferenceLoopHandling = ReferenceLoopHandling.Ignore
- })));
+ return;
}
+
+ var requestData = JsonConvert.SerializeObject(
+ request,
+ Formatting.Indented,
+ new JsonSerializerSettings
+ {
+ ReferenceLoopHandling = ReferenceLoopHandling.Ignore
+ });
+
+ _networkRequestLogList.Add(
+ new Tuple<string, object>(
+ $"RequestUrl:{request.Url}",
+ requestData));
}
catch (Exception ex)
{
Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex);
+ throw;
}
}📝 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.
| private async void OnNetworkRequestSent(object? sender, IRequest request) | |
| { | |
| try | |
| { | |
| if (_BrowserHelper.ShouldMonitorAllUrls() || _BrowserHelper.ShouldMonitorUrl( request.Url)) | |
| { | |
| networkRequestLogList.Add(new Tuple<string, object>($"RequestUrl:{ request.Url}", JsonConvert.SerializeObject(request, Formatting.Indented, | |
| new JsonSerializerSettings | |
| { | |
| ReferenceLoopHandling = ReferenceLoopHandling.Ignore | |
| }) | |
| )); | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex); | |
| } | |
| } | |
| private void OnNetworkRequestSent(object? sender, IRequest request) | |
| { | |
| ArgumentNullException.ThrowIfNull(request); | |
| try | |
| { | |
| if (!(_browserHelper?.ShouldMonitorAllUrls() ?? false) && | |
| !(_browserHelper?.ShouldMonitorUrl(request.Url) ?? false)) | |
| { | |
| return; | |
| } | |
| var requestData = JsonConvert.SerializeObject( | |
| request, | |
| Formatting.Indented, | |
| new JsonSerializerSettings | |
| { | |
| ReferenceLoopHandling = ReferenceLoopHandling.Ignore | |
| }); | |
| _networkRequestLogList.Add( | |
| new Tuple<string, object>( | |
| $"RequestUrl:{request.Url}", | |
| requestData)); | |
| } | |
| catch (Exception ex) | |
| { | |
| Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex); | |
| throw; | |
| } | |
| } |
| @@ -88,6 +88,7 @@ public class SeleniumDriver : GingerWebDriver, IVirtualDriver, IWindowExplorer, | |||
| INetwork interceptor; | |||
| public bool isNetworkLogMonitoringStarted = false; | |||
| ActBrowserElement mAct; | |||
| BrowserHelper _BrowserHelper; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
BrowserHelper initialization needs validation
The BrowserHelper field is added but its initialization is not validated. Consider adding null checks and proper initialization.
- BrowserHelper _BrowserHelper;
+ private BrowserHelper _BrowserHelper;
+ private BrowserHelper BrowserHelper => _BrowserHelper ?? throw new InvalidOperationException("BrowserHelper not initialized");📝 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.
| BrowserHelper _BrowserHelper; | |
| private BrowserHelper _BrowserHelper; | |
| private BrowserHelper BrowserHelper => _BrowserHelper ?? throw new InvalidOperationException("BrowserHelper not initialized"); |
| try | ||
| { | ||
| networkRequestLogList = []; | ||
| networkResponseLogList = []; | ||
| interceptor = webDriver.Manage().Network; | ||
|
|
||
| ProjEnvironment projEnv = GetCurrentProjectEnvironment(); | ||
| ProjEnvironment projEnv = GetCurrentProjectEnvironment(); | ||
|
|
||
| ValueExpression VE = new ValueExpression(projEnv, BusinessFlow); | ||
| ValueExpression VE = new ValueExpression(projEnv, BusinessFlow); | ||
|
|
||
| foreach (ActInputValue item in mAct.UpdateOperationInputValues) | ||
| { | ||
| item.Value = item.Param; | ||
| item.ValueForDriver = VE.Calculate(item.Param); | ||
| } | ||
| foreach (ActInputValue item in mAct.UpdateOperationInputValues) | ||
| { | ||
| item.Value = item.Param; | ||
| item.ValueForDriver = VE.Calculate(item.Param); | ||
| } | ||
|
|
||
| interceptor.NetworkRequestSent += OnNetworkRequestSent; | ||
| interceptor.NetworkResponseReceived += OnNetworkResponseReceived; | ||
| interceptor.NetworkRequestSent += OnNetworkRequestSent; | ||
| interceptor.NetworkResponseReceived += OnNetworkResponseReceived; | ||
|
|
||
| await interceptor.StartMonitoring(); | ||
| isNetworkLogMonitoringStarted = true; | ||
| await interceptor.StartMonitoring(); | ||
| isNetworkLogMonitoringStarted = true; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve StartMonitoringNetworkLog error recovery
The error handling in StartMonitoringNetworkLog should clean up resources on failure.
catch (Exception ex)
{
+ await StopNetworkMonitoring();
+ networkRequestLogList?.Clear();
+ networkResponseLogList?.Clear();
+ isNetworkLogMonitoringStarted = false;
Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex);
}
+ private async Task StopNetworkMonitoring()
+ {
+ if (interceptor != null)
+ {
+ await interceptor.StopMonitoring();
+ interceptor.NetworkRequestSent -= OnNetworkRequestSent;
+ interceptor.NetworkResponseReceived -= OnNetworkResponseReceived;
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
| string requestPath = _BrowserHelper.CreateNetworkLogFile("NetworklogRequest", networkRequestLogList); | ||
| act.ExInfo = $"RequestFile : { requestPath }\n"; | ||
| string responsePath = _BrowserHelper.CreateNetworkLogFile("NetworklogResponse", networkResponseLogList); | ||
| act.ExInfo = $"{act.ExInfo} ResponseFile : {responsePath}\n"; |
There was a problem hiding this comment.
Potential path traversal vulnerability
The file path handling for network logs should validate and sanitize paths to prevent directory traversal attacks.
- string requestPath = _BrowserHelper.CreateNetworkLogFile("NetworklogRequest", networkRequestLogList);
+ string sanitizedFileName = Path.GetFileName("NetworklogRequest").Replace("..", "").Replace("/", "").Replace("\\", "");
+ string requestPath = _BrowserHelper.CreateNetworkLogFile(sanitizedFileName, networkRequestLogList);Committable suggestion skipped: line range outside the PR's diff.
| try | ||
| { | ||
| act.AddOrUpdateReturnParamActual("Raw Request", Newtonsoft.Json.JsonConvert.SerializeObject(networkRequestLogList.Select(x => x.Item2).ToList())); | ||
| act.AddOrUpdateReturnParamActual("Raw Response", Newtonsoft.Json.JsonConvert.SerializeObject(networkResponseLogList.Select(x => x.Item2).ToList())); | ||
| foreach (var val in networkRequestLogList.ToList()) | ||
| { | ||
| act.AddOrUpdateReturnParamActual(act.ControlAction.ToString() + " " + val.Item1.ToString(), Convert.ToString(val.Item2)); | ||
| } | ||
| if (isNetworkLogMonitoringStarted) | ||
| { | ||
| act.AddOrUpdateReturnParamActual("Raw Request", Newtonsoft.Json.JsonConvert.SerializeObject(networkRequestLogList.Select(x => x.Item2).ToList())); | ||
| act.AddOrUpdateReturnParamActual("Raw Response", Newtonsoft.Json.JsonConvert.SerializeObject(networkResponseLogList.Select(x => x.Item2).ToList())); | ||
| foreach (var val in networkRequestLogList.ToList()) | ||
| { | ||
| act.AddOrUpdateReturnParamActual($"{act.ControlAction.ToString()} {val.Item1}", Convert.ToString(val.Item2)); | ||
| } | ||
|
|
||
| foreach (var val in networkResponseLogList.ToList()) | ||
| { | ||
| act.AddOrUpdateReturnParamActual(act.ControlAction.ToString() + " " + val.Item1.ToString(), Convert.ToString(val.Item2)); | ||
| } | ||
| foreach (var val in networkResponseLogList.ToList()) | ||
| { | ||
| act.AddOrUpdateReturnParamActual($"{act.ControlAction.ToString()} {val.Item1}", Convert.ToString(val.Item2)); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| act.ExInfo = $"Action is skipped,{nameof(ActBrowserElement.eControlAction.StartMonitoringNetworkLog)} Action is not started"; | ||
| act.Status = Amdocs.Ginger.CoreNET.Execution.eRunStatus.Skipped; | ||
| } | ||
| } | ||
| else | ||
| catch(Exception ex) | ||
| { | ||
| act.ExInfo = "Action is skipped," + ActBrowserElement.eControlAction.StartMonitoringNetworkLog.ToString() + " Action is not started"; | ||
| act.Status = Amdocs.Ginger.CoreNET.Execution.eRunStatus.Skipped; | ||
| Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in GetNetworkLogAsync
The error handling has been improved, but consider adding more specific error messages and cleanup in the catch block.
catch(Exception ex)
{
+ networkRequestLogList?.Clear();
+ networkResponseLogList?.Clear();
+ isNetworkLogMonitoringStarted = false;
Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex);
}📝 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.
| try | |
| { | |
| act.AddOrUpdateReturnParamActual("Raw Request", Newtonsoft.Json.JsonConvert.SerializeObject(networkRequestLogList.Select(x => x.Item2).ToList())); | |
| act.AddOrUpdateReturnParamActual("Raw Response", Newtonsoft.Json.JsonConvert.SerializeObject(networkResponseLogList.Select(x => x.Item2).ToList())); | |
| foreach (var val in networkRequestLogList.ToList()) | |
| { | |
| act.AddOrUpdateReturnParamActual(act.ControlAction.ToString() + " " + val.Item1.ToString(), Convert.ToString(val.Item2)); | |
| } | |
| if (isNetworkLogMonitoringStarted) | |
| { | |
| act.AddOrUpdateReturnParamActual("Raw Request", Newtonsoft.Json.JsonConvert.SerializeObject(networkRequestLogList.Select(x => x.Item2).ToList())); | |
| act.AddOrUpdateReturnParamActual("Raw Response", Newtonsoft.Json.JsonConvert.SerializeObject(networkResponseLogList.Select(x => x.Item2).ToList())); | |
| foreach (var val in networkRequestLogList.ToList()) | |
| { | |
| act.AddOrUpdateReturnParamActual($"{act.ControlAction.ToString()} {val.Item1}", Convert.ToString(val.Item2)); | |
| } | |
| foreach (var val in networkResponseLogList.ToList()) | |
| { | |
| act.AddOrUpdateReturnParamActual(act.ControlAction.ToString() + " " + val.Item1.ToString(), Convert.ToString(val.Item2)); | |
| } | |
| foreach (var val in networkResponseLogList.ToList()) | |
| { | |
| act.AddOrUpdateReturnParamActual($"{act.ControlAction.ToString()} {val.Item1}", Convert.ToString(val.Item2)); | |
| } | |
| } | |
| else | |
| { | |
| act.ExInfo = $"Action is skipped,{nameof(ActBrowserElement.eControlAction.StartMonitoringNetworkLog)} Action is not started"; | |
| act.Status = Amdocs.Ginger.CoreNET.Execution.eRunStatus.Skipped; | |
| } | |
| } | |
| else | |
| catch(Exception ex) | |
| { | |
| act.ExInfo = "Action is skipped," + ActBrowserElement.eControlAction.StartMonitoringNetworkLog.ToString() + " Action is not started"; | |
| act.Status = Amdocs.Ginger.CoreNET.Execution.eRunStatus.Skipped; | |
| Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex); | |
| } | |
| try | |
| { | |
| if (isNetworkLogMonitoringStarted) | |
| { | |
| act.AddOrUpdateReturnParamActual("Raw Request", Newtonsoft.Json.JsonConvert.SerializeObject(networkRequestLogList.Select(x => x.Item2).ToList())); | |
| act.AddOrUpdateReturnParamActual("Raw Response", Newtonsoft.Json.JsonConvert.SerializeObject(networkResponseLogList.Select(x => x.Item2).ToList())); | |
| foreach (var val in networkRequestLogList.ToList()) | |
| { | |
| act.AddOrUpdateReturnParamActual($"{act.ControlAction.ToString()} {val.Item1}", Convert.ToString(val.Item2)); | |
| } | |
| foreach (var val in networkResponseLogList.ToList()) | |
| { | |
| act.AddOrUpdateReturnParamActual($"{act.ControlAction.ToString()} {val.Item1}", Convert.ToString(val.Item2)); | |
| } | |
| } | |
| else | |
| { | |
| act.ExInfo = $"Action is skipped,{nameof(ActBrowserElement.eControlAction.StartMonitoringNetworkLog)} Action is not started"; | |
| act.Status = Amdocs.Ginger.CoreNET.Execution.eRunStatus.Skipped; | |
| } | |
| } | |
| catch(Exception ex) | |
| { | |
| networkRequestLogList?.Clear(); | |
| networkResponseLogList?.Clear(); | |
| isNetworkLogMonitoringStarted = false; | |
| Reporter.ToLog(eLogLevel.ERROR, $"Method - {MethodBase.GetCurrentMethod().Name}, Error - {ex.Message}", ex); | |
| } |
…olation added in Analyzer
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
Release Notes
New Features
BrowserHelperclass for URL monitoring and network log file creation.Bug Fixes