Skip to content

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

Merged
VietND96 merged 1 commit intotrunkfrom
platform-any
Jan 6, 2025
Merged

[grid]: platformName is empty should be considered as enum ANY instead of WINDOWS#15036
VietND96 merged 1 commit intotrunkfrom
platform-any

Conversation

@VietND96
Copy link
Member

@VietND96 VietND96 commented Jan 6, 2025 •
edited
Loading

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.

@VietND96
[grid]: platformName is empty should be considered as enum ANY instea…
3b622e5 
…d of WINDOWS

Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
@qodo-code-review
Copy link
Contributor

qodo-code-review 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
@qodo-code-review
Copy link
Contributor

qodo-code-review 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

@VietND96 VietND96 requested a review from diemol January 6, 2025 13:13
@VietND96 VietND96 mentioned this pull request Jan 6, 2025
Closed
diemol
diemol approved these changes Jan 6, 2025
View reviewed changes
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
pujagani
pujagani approved these changes Jan 6, 2025
View reviewed changes
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!

@VietND96 VietND96 deleted the platform-any branch January 6, 2025 14:15
This was referenced Jan 9, 2025
Merged
Closed
@VietND96 VietND96 mentioned this pull request Jan 17, 2025
Closed
sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
@VietND96 @sandeepsuryaprasad
[grid]: platformName is empty should be considered as enum ANY instea…
86e86a8 
…d of WINDOWS (SeleniumHQ#15036)

Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@diemol diemol diemol approved these changes

@pujagani pujagani pujagani approved these changes

Assignees

No one assigned

Labels

Review effort [1-5]: 2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@VietND96 @diemol @pujagani