Patch 1 for v2025.1#4134
Conversation
2. Handled TA rename issue for POM page 3. Implemented RemoteWebDriver for Safari
WalkthroughThis pull request refines the application platform retrieval in the workspace solution and introduces Safari browser support in the Selenium driver. The Changes
Sequence Diagram(s)Application Platform Lookup in Solution.cssequenceDiagram
participant Client as Client Code
participant Solution as Solution.cs
participant Lookup as ApplicationPlatform Lookup
Client->>Solution: Call GetTargetApplicationPlatform(TargetApplicationKey)
Solution->>Lookup: Lookup using Application Name
alt Platform Found
Lookup-->>Solution: Return platform
else Not found
Solution->>Lookup: Lookup using GUID
alt Platform Found by GUID
Lookup-->>Solution: Return platform
else
Lookup-->>Solution: Return default value
end
end
Solution-->>Client: Return final platform value
Safari Driver Initialization in SeleniumDriver.cssequenceDiagram
participant Tester as Test Invoker
participant Selenium as SeleniumDriver
participant SafariOpts as SafariOptions
participant Remote as RemoteWebDriver
Tester->>Selenium: Call StartDriver()
alt RemoteBrowserName == "safari"
Selenium->>SafariOpts: Create and configure SafariOptions
Selenium->>Selenium: Set unhandled prompt behavior
Selenium->>Remote: Initialize RemoteWebDriver with SafariOptions
else Other browser
Selenium->>...: Execute existing driver initialization
end
Remote-->>Tester: Return active driver instance
Suggested reviewers
Poem
✨ Finishing Touches
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Ginger/GingerCore/Drivers/JavaDriverLib/GingerJavaAgent/GingerAgent.jaris excluded by!**/*.jar,!**/*.jar
📒 Files selected for processing (2)
Ginger/GingerCoreCommon/WorkSpaceLib/Solution.cs(1 hunks)Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs(2 hunks)
🔇 Additional comments (5)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs (4)
56-56: Addition of Safari browser support.The import statement for Safari browser capabilities has been properly added to extend browser support in the framework.
958-988: Well-implemented Safari browser support for RemoteWebDriver.The implementation for Safari browser follows the same pattern as other browsers, maintaining code consistency. The code:
- Creates a
SafariOptionsinstance with appropriate proxy configuration- Adds platform and version options when provided
- Configures unhandled prompt behavior
- Creates the RemoteWebDriver with appropriate timeout and Healenium support
This is a clean addition that extends the framework's browser capabilities without introducing deviations from the existing patterns.
973-973: Verify consistent prompt behavior setup across browsers.Ensures that Safari is correctly calling the SetUnhandledPromptBehavior method like other browsers. This is important for consistent behavior across different browser implementations.
975-985: Verify Safari remote connection configuration is complete.The code correctly handles the HTTP server timeout and Healenium configuration for Safari, maintaining consistency with other browser implementations. The implementation follows the same pattern as used for Chrome, Firefox, and Edge browsers.
Ginger/GingerCoreCommon/WorkSpaceLib/Solution.cs (1)
408-416: Improved null-checking and return logic for platform lookup.The method now properly checks for null/empty values before attempting to find the application platform, and uses a cleaner conditional expression for returning the platform or a default value.
| ApplicationPlatform appPlatform = ApplicationPlatforms.FirstOrDefault(ap => string.Equals(ap.AppName, targetapp)); | ||
| return appPlatform != null ? appPlatform.Platform : default; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider supporting lookup by GUID for consistency with GetTargetApplicationPlatform.
For consistency with the GetTargetApplicationPlatform method, consider enhancing this method to also try finding the application platform by GUID if a string representation of a GUID is provided.
ApplicationPlatform appPlatform = ApplicationPlatforms.FirstOrDefault(ap => string.Equals(ap.AppName, targetapp));
+// If not found by name, try parsing as GUID
+if (appPlatform == null && Guid.TryParse(targetapp, out Guid targetGuid))
+{
+ appPlatform = ApplicationPlatforms.FirstOrDefault(ap => ap.Guid == targetGuid);
+}
return appPlatform != null ? appPlatform.Platform : default;📝 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.
| ApplicationPlatform appPlatform = ApplicationPlatforms.FirstOrDefault(ap => string.Equals(ap.AppName, targetapp)); | |
| return appPlatform != null ? appPlatform.Platform : default; | |
| ApplicationPlatform appPlatform = ApplicationPlatforms.FirstOrDefault(ap => string.Equals(ap.AppName, targetapp)); | |
| // If not found by name, try parsing as GUID | |
| if (appPlatform == null && Guid.TryParse(targetapp, out Guid targetGuid)) | |
| { | |
| appPlatform = ApplicationPlatforms.FirstOrDefault(ap => ap.Guid == targetGuid); | |
| } | |
| return appPlatform != null ? appPlatform.Platform : default; |
| ApplicationPlatform appPlatform = ApplicationPlatforms.FirstOrDefault(ap => string.Equals(ap.AppName, targetAppName)); | ||
| if (appPlatform == null) | ||
| { | ||
| string targetapp = TargetApplicationKey.ItemName; | ||
| ePlatformType platform = (from x in ApplicationPlatforms where x.AppName == targetapp select x.Platform).FirstOrDefault(); | ||
| return platform; | ||
| appPlatform = ApplicationPlatforms.FirstOrDefault(ap => ap.Guid == targetAppGuid); | ||
| } | ||
| return ePlatformType.Web; | ||
| ePlatformType platform = appPlatform != null ? appPlatform.Platform : default; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding logging for platform resolution failures.
When an application platform can't be found, the code silently returns a default value. Adding logging in these cases could help troubleshoot issues where applications aren't correctly mapping to their expected platforms.
ApplicationPlatform appPlatform = ApplicationPlatforms.FirstOrDefault(ap => string.Equals(ap.AppName, targetAppName));
if (appPlatform == null)
{
appPlatform = ApplicationPlatforms.FirstOrDefault(ap => ap.Guid == targetAppGuid);
+ if (appPlatform == null)
+ {
+ Reporter.ToLog(eLogLevel.WARN, $"Could not find ApplicationPlatform for app name '{targetAppName}' or GUID '{targetAppGuid}'");
+ }
}📝 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.
| ApplicationPlatform appPlatform = ApplicationPlatforms.FirstOrDefault(ap => string.Equals(ap.AppName, targetAppName)); | |
| if (appPlatform == null) | |
| { | |
| string targetapp = TargetApplicationKey.ItemName; | |
| ePlatformType platform = (from x in ApplicationPlatforms where x.AppName == targetapp select x.Platform).FirstOrDefault(); | |
| return platform; | |
| appPlatform = ApplicationPlatforms.FirstOrDefault(ap => ap.Guid == targetAppGuid); | |
| } | |
| return ePlatformType.Web; | |
| ePlatformType platform = appPlatform != null ? appPlatform.Platform : default; | |
| ApplicationPlatform appPlatform = ApplicationPlatforms.FirstOrDefault(ap => string.Equals(ap.AppName, targetAppName)); | |
| if (appPlatform == null) | |
| { | |
| appPlatform = ApplicationPlatforms.FirstOrDefault(ap => ap.Guid == targetAppGuid); | |
| if (appPlatform == null) | |
| { | |
| Reporter.ToLog(eLogLevel.WARN, $"Could not find ApplicationPlatform for app name '{targetAppName}' or GUID '{targetAppGuid}'"); | |
| } | |
| } | |
| ePlatformType platform = appPlatform != null ? appPlatform.Platform : default; |
| public ePlatformType GetTargetApplicationPlatform(RepositoryItemKey TargetApplicationKey) | ||
| { | ||
| { | ||
| if (TargetApplicationKey != null) | ||
| { | ||
| string targetAppName = TargetApplicationKey.ItemName; | ||
| Guid targetAppGuid = TargetApplicationKey.Guid; | ||
| ApplicationPlatform appPlatform = ApplicationPlatforms.FirstOrDefault(ap => string.Equals(ap.AppName, targetAppName)); | ||
| if (appPlatform == null) | ||
| { | ||
| string targetapp = TargetApplicationKey.ItemName; | ||
| ePlatformType platform = (from x in ApplicationPlatforms where x.AppName == targetapp select x.Platform).FirstOrDefault(); | ||
| return platform; | ||
| appPlatform = ApplicationPlatforms.FirstOrDefault(ap => ap.Guid == targetAppGuid); | ||
| } | ||
| return ePlatformType.Web; | ||
| ePlatformType platform = appPlatform != null ? appPlatform.Platform : default; | ||
| return platform; | ||
| } | ||
| return ePlatformType.Web; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improved application platform lookup by adding GUID check.
The method now intelligently attempts to find an ApplicationPlatform first by name, then by GUID, which makes the lookup more robust. This enhancement is particularly valuable when an application might be referenced by different identifiers in different parts of the system.
While the implementation is sound, I noticed inconsistent default return values between this method (returning ePlatformType.Web) and GetApplicationPlatformForTargetApp (returning ePlatformType.NA). Consider aligning these default values for consistency:
- return ePlatformType.Web;
+ return ePlatformType.NA;📝 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 ePlatformType GetTargetApplicationPlatform(RepositoryItemKey TargetApplicationKey) | |
| { | |
| { | |
| if (TargetApplicationKey != null) | |
| { | |
| string targetAppName = TargetApplicationKey.ItemName; | |
| Guid targetAppGuid = TargetApplicationKey.Guid; | |
| ApplicationPlatform appPlatform = ApplicationPlatforms.FirstOrDefault(ap => string.Equals(ap.AppName, targetAppName)); | |
| if (appPlatform == null) | |
| { | |
| string targetapp = TargetApplicationKey.ItemName; | |
| ePlatformType platform = (from x in ApplicationPlatforms where x.AppName == targetapp select x.Platform).FirstOrDefault(); | |
| return platform; | |
| appPlatform = ApplicationPlatforms.FirstOrDefault(ap => ap.Guid == targetAppGuid); | |
| } | |
| return ePlatformType.Web; | |
| ePlatformType platform = appPlatform != null ? appPlatform.Platform : default; | |
| return platform; | |
| } | |
| return ePlatformType.Web; | |
| } | |
| public ePlatformType GetTargetApplicationPlatform(RepositoryItemKey TargetApplicationKey) | |
| { | |
| if (TargetApplicationKey != null) | |
| { | |
| string targetAppName = TargetApplicationKey.ItemName; | |
| Guid targetAppGuid = TargetApplicationKey.Guid; | |
| ApplicationPlatform appPlatform = ApplicationPlatforms.FirstOrDefault(ap => string.Equals(ap.AppName, targetAppName)); | |
| if (appPlatform == null) | |
| { | |
| appPlatform = ApplicationPlatforms.FirstOrDefault(ap => ap.Guid == targetAppGuid); | |
| } | |
| ePlatformType platform = appPlatform != null ? appPlatform.Platform : default; | |
| return platform; | |
| } | |
| return ePlatformType.NA; | |
| } |
9370b93
into
Releases/Published-Official-Releases/Official-Release-2025.1
2 participants
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
New Features
Refactor