Self healing operation refactoring#3637
Conversation
1. Avoid learning html and Body tags during POM creation 2. Update Auto Update of self healing to only trigger once during single run for Automate tab run. Removed existing 5 hours check 3. Changes to SeleniumDriver.cs file to improve readability, flexibility, and functionality. 3. Changes in `PluginsManager.cs` to prevent potential NullReferenceExceptions and improve readability and performance. 4. Changes in `AgentOperations.cs` to prevent a potential NullReferenceException. 5. Changes in `XPathHelper.cs` to improve readability and performance, and to use new features introduced in C# 9.0. 6. Avoid learning screenshots of unmapped elements during POM learn 7. Convert Exact and Contains Text match to be case insensitive in case of auto learned element 8. De-Select tag name loctor in case friendly locators are not available for element and its not only locator.
WalkthroughThe recent updates across various components in the Ginger software suite focus on refining and optimizing code efficiency and readability. These changes include enhancements in error handling, refactoring of core functionalities, and improved management of data structures and conditions, ensuring a smoother and more reliable operation of the software. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| { | ||
| int Counter = 0; | ||
| while (Status != Agent.eStatus.Running && String.IsNullOrEmpty(Driver.ErrorMessageFromDriver)) | ||
| while (Status != Agent.eStatus.Running && String.IsNullOrEmpty(Driver?.ErrorMessageFromDriver)) |
There was a problem hiding this comment.
Use null-conditional operator for Driver in the entire condition.
- while (Status != Agent.eStatus.Running && String.IsNullOrEmpty(Driver?.ErrorMessageFromDriver))
+ while (Status != Agent.eStatus.Running && String.IsNullOrEmpty(Driver?.ErrorMessageFromDriver))This change ensures that the entire condition is safe against null references, maintaining consistency in how Driver is accessed within the condition.
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.
| while (Status != Agent.eStatus.Running && String.IsNullOrEmpty(Driver?.ErrorMessageFromDriver)) | |
| while (Status != Agent.eStatus.Running && String.IsNullOrEmpty(Driver?.ErrorMessageFromDriver)) |
There was a problem hiding this comment.
null-conditional operator is used
There was a problem hiding this comment.
@Maheshkale447, thank you for confirming the use of the null-conditional operator. If there's anything else you need, feel free to ask!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| // Get start time stamp of RunSet or BusinessFlow | ||
| DateTime? startTimeStamp = ExecutedFrom == eExecutedFrom.Run | ||
| ? WorkSpace.Instance.RunsetExecutor.RunSetConfig?.StartTimeStamp | ||
| : ((GingerCore.Agent)currentAgent).BusinessFlow?.StartTimeStamp; | ||
|
|
||
| // Check if the POM was updated after the start time stamp, if yes, don't update the POM | ||
| if (startTimeStamp.HasValue && Convert.ToDateTime(GetCurrentPOMElementInfo().LastUpdatedTime).ToUniversalTime() > startTimeStamp.Value) | ||
| { | ||
| var pomLastUpdatedTimeSpan = (System.DateTime.Now - Convert.ToDateTime(GetCurrentPOMElementInfo().LastUpdatedTime)).TotalHours; | ||
| if (pomLastUpdatedTimeSpan < 5) | ||
| { | ||
| return null; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
Consider handling potential exceptions when converting LastUpdatedTime to DateTime.
- if (startTimeStamp.HasValue && Convert.ToDateTime(GetCurrentPOMElementInfo().LastUpdatedTime).ToUniversalTime() > startTimeStamp.Value)
+ if (startTimeStamp.HasValue && DateTime.TryParse(GetCurrentPOMElementInfo().LastUpdatedTime, out DateTime lastUpdatedTime) && lastUpdatedTime.ToUniversalTime() > startTimeStamp.Value)This change ensures that the conversion does not throw an exception if LastUpdatedTime is not in a valid format.
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.
| // Get start time stamp of RunSet or BusinessFlow | |
| DateTime? startTimeStamp = ExecutedFrom == eExecutedFrom.Run | |
| ? WorkSpace.Instance.RunsetExecutor.RunSetConfig?.StartTimeStamp | |
| : ((GingerCore.Agent)currentAgent).BusinessFlow?.StartTimeStamp; | |
| // Check if the POM was updated after the start time stamp, if yes, don't update the POM | |
| if (startTimeStamp.HasValue && Convert.ToDateTime(GetCurrentPOMElementInfo().LastUpdatedTime).ToUniversalTime() > startTimeStamp.Value) | |
| { | |
| var pomLastUpdatedTimeSpan = (System.DateTime.Now - Convert.ToDateTime(GetCurrentPOMElementInfo().LastUpdatedTime)).TotalHours; | |
| if (pomLastUpdatedTimeSpan < 5) | |
| { | |
| return null; | |
| } | |
| return null; | |
| // Get start time stamp of RunSet or BusinessFlow | |
| DateTime? startTimeStamp = ExecutedFrom == eExecutedFrom.Run | |
| ? WorkSpace.Instance.RunsetExecutor.RunSetConfig?.StartTimeStamp | |
| : ((GingerCore.Agent)currentAgent).BusinessFlow?.StartTimeStamp; | |
| // Check if the POM was updated after the start time stamp, if yes, don't update the POM | |
| if (startTimeStamp.HasValue && DateTime.TryParse(GetCurrentPOMElementInfo().LastUpdatedTime, out DateTime lastUpdatedTime) && lastUpdatedTime.ToUniversalTime() > startTimeStamp.Value) | |
| { | |
| return null; |
There was a problem hiding this comment.
LastUpdatedTime will always be correct DateTime
There was a problem hiding this comment.
@Maheshkale447, thank you for confirming the reliability of the LastUpdatedTime format. I'll remember this detail for future reviews to ensure we maintain streamlined and efficient code.
(\(\
( -.-)
o_(")(")
This little bunny is thoughtful, remembering your guidance for future interactions!
New Learnings Added
User: Maheshkale447
PR: Ginger-Automation/Ginger#3637
File: Ginger/GingerCoreNET/Application Models/Execution/POM/POMExecutionUtils.cs:226-234
Timestamp: 2024-04-25T21:51:07.425Z
Learning: The `LastUpdatedTime` in the `POMExecutionUtils.cs` file of the Ginger project is always in the correct DateTime format, as confirmed by the user. This information is crucial for avoiding unnecessary error handling suggestions in future reviews.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
| if (deltaElementInfos.Count > 0) | ||
| { | ||
| UpdateElementSelfHealingDetails(deltaElementInfos.ToList()); | ||
| UpdateElementSelfHealingDetails([.. deltaElementInfos]); |
There was a problem hiding this comment.
Ensure that deltaElementInfos is not null before calling UpdateElementSelfHealingDetails.
- UpdateElementSelfHealingDetails([.. deltaElementInfos]);
+ if (deltaElementInfos != null) UpdateElementSelfHealingDetails(deltaElementInfos);This modification adds a null check to prevent potential null reference exceptions.
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.
| UpdateElementSelfHealingDetails([.. deltaElementInfos]); | |
| if (deltaElementInfos != null) UpdateElementSelfHealingDetails(deltaElementInfos); |
There was a problem hiding this comment.
Its checked in if before this piece of code
There was a problem hiding this comment.
@Maheshkale447, thank you for pointing that out. It's good practice to ensure that methods returning collections, like GetUpdatedVirtulPOM, do not return null but rather an empty collection if there are no elements to return. This approach avoids the need for null checks when the method is used.
If you're confident that GetUpdatedVirtulPOM is implemented this way, then the existing check using Any() should suffice to handle cases where there are no elements to update. However, if there's any uncertainty about this behavior, it might be beneficial to review the implementation of GetUpdatedVirtulPOM to confirm that it never returns null. This can help prevent potential issues in scenarios where the method might unexpectedly return null due to changes or exceptions within it.
Let me know if there's anything else you'd like to discuss or if you need further assistance!
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (1)
Ginger/GingerCoreNET/Run/RunListenerLib/LiteDBRepository.cs (1)
251-253: Ensure consistent naming conventions for dictionary keys.The keys used in the dictionary for
ChildExecutableItemsCount,ChildExecutedItemsCount, andChildPassedItemsCountare derived from an enum usingnameof. This is good for maintainability, but ensure that the enum values are consistent and meaningful across the application to avoid confusion.
| businessFlow.ChildExecutableItemsCount.TryGetValue(nameof(HTMLReportConfiguration.eExecutionStatisticsCountBy.Activities), out count); | ||
| ChildExecutableItemsCountActivity = ChildExecutableItemsCountActivity + count; | ||
|
|
||
| businessFlow.ChildExecutedItemsCount.TryGetValue(HTMLReportConfiguration.eExecutionStatisticsCountBy.Activities.ToString(), out count); | ||
| businessFlow.ChildExecutedItemsCount.TryGetValue(nameof(HTMLReportConfiguration.eExecutionStatisticsCountBy.Activities), out count); | ||
| ChildExecutedItemsCountActivity = ChildExecutedItemsCountActivity + count; | ||
|
|
||
| businessFlow.ChildPassedItemsCount.TryGetValue(HTMLReportConfiguration.eExecutionStatisticsCountBy.Activities.ToString(), out count); | ||
| businessFlow.ChildPassedItemsCount.TryGetValue(nameof(HTMLReportConfiguration.eExecutionStatisticsCountBy.Activities), out count); | ||
| ChildPassedItemsCountActivity = ChildPassedItemsCountActivity + count; | ||
|
|
||
| businessFlow.ChildExecutableItemsCount.TryGetValue(HTMLReportConfiguration.eExecutionStatisticsCountBy.Actions.ToString(), out count); | ||
| businessFlow.ChildExecutableItemsCount.TryGetValue(nameof(HTMLReportConfiguration.eExecutionStatisticsCountBy.Actions), out count); | ||
| ChildExecutableItemsCountAction = ChildExecutableItemsCountAction + count; | ||
|
|
||
| businessFlow.ChildExecutedItemsCount.TryGetValue(HTMLReportConfiguration.eExecutionStatisticsCountBy.Actions.ToString(), out count); | ||
| businessFlow.ChildExecutedItemsCount.TryGetValue(nameof(HTMLReportConfiguration.eExecutionStatisticsCountBy.Actions), out count); | ||
| ChildExecutedItemsCountAction = ChildExecutedItemsCountAction + count; | ||
|
|
||
| businessFlow.ChildPassedItemsCount.TryGetValue(HTMLReportConfiguration.eExecutionStatisticsCountBy.Actions.ToString(), out count); | ||
| businessFlow.ChildPassedItemsCount.TryGetValue(nameof(HTMLReportConfiguration.eExecutionStatisticsCountBy.Actions), out count); |
There was a problem hiding this comment.
Review the use of TryGetValue for performance optimization.
The repeated use of TryGetValue in a loop can be optimized. Consider caching the results or restructuring the data access patterns to reduce the overhead of dictionary lookups in a tight loop.
1 participant
PluginsManager.csto prevent potential NullReferenceExceptions and improve readability and performance.AgentOperations.csto prevent a potential NullReferenceException.XPathHelper.csto improve readability and performance, and to use new features introduced in C# 9.0.Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit