Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[grid]: platformName is empty should be considered as enum ANY instead of WINDOWS #15036

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Jan 6, 2025

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

When adding a test to verify when "platformName": "" it should map to enum ANY

  @Test
  void testAnyIsFromStringEmpty() {
    assertThat(ANY).isEqualTo(Platform.fromString(""));
  }

However, it failed with expected that is WINDOWS.

testAnyIsFromStringEmpty() (org.openqa.selenium.PlatformTest)
org.opentest4j.AssertionFailedError: 
expected: windows
 but was: any

In a end2end scenario, it also behaves the same. For example TOML given

[[node.driver-configuration]]
display-name = "chrome"
stereotype = '{"browserName": "chrome", "browserVersion": "", "platformName": "", "goog:chromeOptions": {"binary": "/usr/bin/google-chrome"}, "se:containerName": "selenium-node-chrome-69847c778d-8qzwg"}'
max-sessions = 1

When Node started up, the Node stereotypes could be seen platformName: windows

INFO: Adding chrome for {"browserName": "chrome","browserVersion": "","goog:chromeOptions": {"binary": "\u002fusr\u002fbin\u002fgoogle-chrome"},"platformName": "windows","se:containerName": "selenium-node-chrome-69847c778d-8qzwg","se:downloadsEnabled": true,"se:noVncPort": 7900,"se:vncEnabled": true} 1 times

It does not make sense when checking from Grid UI, the OS logo for session requests and ongoing.

image

In case, Node stereotypes setting "platformName": "". It should be loaded "platformName":"any" and reflect to Grid UI the unknown os icon, e.g

[[node.driver-configuration]]
display-name = "firefox"
stereotype = '{"browserName": "firefox", "browserVersion": "133.0", "platformName": "", "moz:firefoxOptions": {"binary": "/usr/bin/firefox"}, "se:containerName": ""}'
max-sessions = 1

INFO: Adding firefox for {"browserName": "firefox","browserVersion": "133.0","moz:firefoxOptions": {"binary": "\u002fusr\u002fbin\u002ffirefox"},"platformName": "any","se:containerName": "","se:noVncPort": 7900,"se:vncEnabled": true} 1 times

image

This also helps autoscaling have a consistent pattern to correctly count pending and ongoing sessions for populating the right external metrics.

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Contributor

qodo-merge-pro bot commented Jan 6, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Breaking Change

Changing WINDOWS platform string from empty to "windows" could break existing code that relies on the empty string behavior. This change should be carefully validated with existing Grid deployments.

WINDOWS("windows") {

@VietND96 VietND96 changed the title [grid]: platformName is empty should be considered as enum ANY instea… [grid]: platformName is empty should be considered as enum ANY instead of WINDOWS Jan 6, 2025
Copy link
Contributor

qodo-merge-pro bot commented Jan 6, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
General
Return a non-null family value for consistency with other platform implementations

The WINDOWS platform's family() method returns null which is inconsistent with other
platforms that return their family type. Consider returning WINDOWS as its own
family.

java/src/org/openqa/selenium/Platform.java [35-39]

 WINDOWS("windows") {
   @Override
   public Platform family() {
-    return null;
+    return WINDOWS;
   }
  • Apply this suggestion
Suggestion importance[1-10]: 8

Why: Returning null for family() is inconsistent with other platforms (like SEQUOIA returning MAC) and could lead to NullPointerExceptions. Having WINDOWS return itself as family aligns with the enum's design pattern.

8
Add missing toString() override to ensure consistent string representation across platform enums

The WINDOWS platform should override toString() method to provide a meaningful
string representation, consistent with other platform enums.

java/src/org/openqa/selenium/Platform.java [35-39]

 WINDOWS("windows") {
   @Override
   public Platform family() {
     return null;
   }
+  
+  @Override
+  public String toString() {
+    return "Windows";
+  }
  • Apply this suggestion
Suggestion importance[1-10]: 6

Why: Adding toString() would improve consistency with other platform enums like SEQUOIA that have explicit string representations, enhancing code maintainability and debugging experience.

6

Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense.

@VietND96
Copy link
Member Author

VietND96 commented Jan 6, 2025

This makes sense.

Thanks for your confirmation. I will merge this.

@VietND96 VietND96 merged commit 81f435d into trunk Jan 6, 2025
33 checks passed
Copy link
Contributor

@pujagani pujagani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @VietND96!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants