.pr_agent_auto_best_practices

Pattern 1: Add proper null checks and validation for method parameters and dictionary access to prevent potential null reference exceptions. This includes using null-coalescing operators, null-conditional operators, and explicit parameter validation.

Example code before:

public void ProcessValue(string key) {
    dict[key].Process();
}

Example code after:

public void ProcessValue(string key) {
    if (key == null) throw new ArgumentNullException(nameof(key));
    dict.TryGetValue(key, out var value)?.Process();
}

Relevant past accepted suggestions:

---

Suggestion 1:

Add null safety checks and proper dictionary access to prevent potential runtime exceptions

The IsEnabled method could throw a NullReferenceException if _loggers is null or if the logger's issuer type is not found in the dictionary. Add null checks and fallback logic.

dotnet/src/webdriver/Internal/Logging/LogContext.cs [104]

-return Handlers != null && level >= _level && level >= _loggers?[logger.Issuer].Level;
+return Handlers != null && level >= _level && (_loggers?.TryGetValue(logger.Issuer, out var loggerEntry) != true || level >= loggerEntry.Level);

---

Suggestion 2:

Add null checks during dictionary initialization to prevent potential null reference exceptions

The logger initialization in constructor could fail if any of the source loggers has a null Issuer. Add validation to handle this case.

dotnet/src/webdriver/Internal/Logging/LogContext.cs [51]

-_loggers = new ConcurrentDictionary<Type, ILogger>(loggers.Select(l => new KeyValuePair<Type, ILogger>(l.Key, new Logger(l.Value.Issuer, level))));
+_loggers = new ConcurrentDictionary<Type, ILogger>(loggers.Where(l => l.Value?.Issuer != null).Select(l => new KeyValuePair<Type, ILogger>(l.Key, new Logger(l.Value.Issuer, level))));

---

Suggestion 3:

Add parameter validation to prevent null reference exceptions

Add null check for value parameter in FromJson method to prevent potential NullReferenceException when deserializing JSON.

dotnet/src/webdriver/Response.cs [74-76]

 public static Response FromJson(string value)
 {
+    if (string.IsNullOrEmpty(value))
+    {
+        throw new ArgumentNullException(nameof(value));
+    }
     Dictionary<string, object> rawResponse = JsonSerializer.Deserialize<Dictionary<string, object>>(value, s_jsonSerializerOptions)

---

Suggestion 4:

Add parameter validation to prevent null reference exceptions

Add null check for key parameter in SetPreferenceValue method to prevent potential issues with null keys.

dotnet/src/webdriver/Firefox/Preferences.cs [166-168]

 private void SetPreferenceValue(string key, JsonNode? value)
 {
+    if (key == null)
+        throw new ArgumentNullException(nameof(key));
     if (!this.IsSettablePreference(key))

---

Suggestion 5:

Use null-coalescing operator with null-conditional operator for safer string conversion

Consider using the null-coalescing operator (??) instead of the null-conditional operator (?.) followed by ToString(). This will provide a default empty string if the value is null, avoiding potential null reference exceptions.

dotnet/src/webdriver/ErrorResponse.cs [55]

-this.message = responseValue["message"].ToString() ?? "";
+this.message = responseValue["message"]?.ToString() ?? string.Empty;

---

Suggestion 6:

Use null-conditional operator for safer null checking

Use the null-forgiving operator (!) only when absolutely necessary. Consider using the null-conditional operator (?.) for a safer approach to null checking.

dotnet/src/webdriver/Alert.cs [50]

-return commandResponse.Value.ToString()!;
+return commandResponse.Value?.ToString() ?? string.Empty;

---

Pattern 2: Enhance error messages by including actual values and specific details to improve debugging experience. Error messages should provide context about what went wrong and relevant variable values.

Example code before:

throw new ArgumentException("Invalid configuration");

Example code after:

throw new ArgumentException($"Invalid configuration value: '{configValue}'");

Relevant past accepted suggestions:

---

Suggestion 1:

Enhance error messages with actual values for better debugging experience

Include the actual value in the exception message to help with debugging when an invalid cookie name is provided.

dotnet/src/webdriver/CookieJar.cs [82]

-throw new ArgumentException("Cookie name cannot be empty", nameof(name));
+throw new ArgumentException($"Cookie name cannot be empty. Provided value: '{name}'", nameof(name));

---

Suggestion 2:

Provide accurate error messages that reflect the actual system locale instead of assuming Arabic

The error message incorrectly assumes the system language is Arabic when it's any non-English locale. Update the message to be more accurate and include the actual locale in the error message.

java/src/org/openqa/selenium/chrome/ChromeDriverService.java [289]

-throw new NumberFormatException("Couldn't format the port numbers because the System Language is arabic: \"" + String.format("--port=%d", getPort()) +
+throw new NumberFormatException("Couldn't format the port numbers due to non-English system locale '" + Locale.getDefault(Locale.Category.FORMAT) + "': \"" + String.format("--port=%d", getPort()) +

---

Suggestion 3:

Correct the error message by removing an unnecessary character

Remove the '=' sign from the error message as it appears to be a typo and doesn't add any meaningful information.

java/src/org/openqa/selenium/manager/SeleniumManager.java [198]

-throw new WebDriverException("Linux ARM64= is not supported by Selenium Manager");
+throw new WebDriverException("Linux ARM64 is not supported by Selenium Manager");

---

Pattern 3: Use proper thread-safe approaches when managing shared resources in multi-threaded environments. Replace class-level variables with thread-local storage or synchronized data structures.

Example code before:

private static Dictionary<string, Handler> handlers = new Dictionary<string, Handler>();

Example code after:

private static ThreadLocal<Dictionary<string, Handler>> handlers = 
    new ThreadLocal<Dictionary<string, Handler>>(() => new Dictionary<string, Handler>());

Relevant past accepted suggestions:

---

Suggestion 1:

Improve thread safety for managing authentication callbacks

Consider using a more thread-safe approach for managing AUTH_CALLBACKS. Instead of using a class variable, you could use a thread-local variable or a synchronized data structure to ensure thread safety in multi-threaded environments.

rb/lib/selenium/webdriver/common/network.rb [23-26]

-AUTH_CALLBACKS = {}
+def self.auth_callbacks
+  Thread.current[:auth_callbacks] ||= {}
+end
+
 def initialize(bridge)
   @network = BiDi::Network.new(bridge.bidi)
 end

---

Suggestion 2:

Ensure proper cleanup of resources when removing authentication handlers

Implement a cleanup mechanism in the removeAuthenticationHandler method to ensure that any resources or intercepts associated with the handler are properly cleaned up to prevent memory leaks or dangling references.

javascript/node/selenium-webdriver/lib/network.js [78]

-this.#authHandlers.delete(id)
+if (!this.#authHandlers.has(id)) {
+  throw new Error(`No authentication handler found with ID: ${id}`);
+}
+// Perform any necessary cleanup related to the handler here
+this.#authHandlers.delete(id);
 

---

Pattern 4: Use correct methods for collection operations (e.g., using len() instead of count() for list length in Python, proper Map creation in Java) to ensure efficient and correct behavior.

Example code before:

if (myList.count() == 3) {
    Map<String, Object> map = Map.of("key1", value1, "key2", value2);
    if (hasMore) {
        map = Map.of("key1", value1, "key2", value2, "key3", value3);
    }
}

Example code after:

if (len(myList) == 3) {
    Map<String, Object> map = new HashMap<>();
    map.put("key1", value1);
    map.put("key2", value2);
    if (hasMore) {
        map.put("key3", value3);
    }
}

Relevant past accepted suggestions:

---

Suggestion 1:

Optimize map creation efficiency

The toMap() method creates new Map instances unnecessarily. Optimize by using a single mutable map and conditionally adding the contexts.

java/src/org/openqa/selenium/bidi/network/SetCacheBehaviorParameters.java [37-45]

 public Map<String, Object> toMap() {
-  Map<String, Object> map = Map.of("cacheBehavior", cacheBehavior.toString());
-
+  Map<String, Object> map = new HashMap<>();
+  map.put("cacheBehavior", cacheBehavior.toString());
+  
   if (contexts != null && !contexts.isEmpty()) {
-    return Map.of("cacheBehavior", cacheBehavior.toString(), "contexts", contexts);
+    map.put("contexts", contexts);
   }
-
+  
   return map;
 }

---

Suggestion 2:

Use the correct method to check the length of a list

Use assert len(ids) == 1 instead of assert ids.count() == 1 to check the length of the list. The count() method is used to count occurrences of a specific element, not the length of the list.

py/test/selenium/webdriver/support/relative_by_tests.py [136-138]

 ids = [el.get_attribute("id") for el in elements]
-assert ids.count() == 1
+assert len(ids) == 1
 assert "bottomRight" in ids
 

---

Suggestion 3:

Use the correct method to check the number of elements in a list

Replace assert ids.count() == 3 with assert len(ids) == 3 to correctly check the number of elements in the list.

py/test/selenium/webdriver/support/relative_by_tests.py [214-218]

 ids = [el.get_attribute("id") for el in elements]
-assert ids.count() == 3
+assert len(ids) == 3
 assert "top" in ids
 assert "topLeft" in ids
 assert "topRight" in ids
 

---

Pattern 5: Ensure test methods have clear assertions that verify all relevant aspects of the behavior being tested, including edge cases and error conditions.

Example code before:

@Test
void testDialogDismissal() {
    dialog.dismiss();
    assertNull(dialog.getContent());
}

Example code after:

@Test
void testDialogDismissal() {
    dialog.dismiss();
    assertNull(dialog.getContent());
    assertFalse(dialog.isVisible());
    assertEquals(DialogState.CLOSED, dialog.getState());
}

Relevant past accepted suggestions:

---

Suggestion 1:

Add assertions to verify the dialog is dismissed to make the test more robust

Consider adding assertions to verify that the dialog is indeed dismissed after calling fedcmDriver.setDelayEnabled(false). This will make the test more robust and ensure the expected behavior.

java/test/org/openqa/selenium/federatedcredentialmanagement/FederatedCredentialManagementTest.java [83-85]

 void testDismissDialog() {
   fedcmDriver.setDelayEnabled(false);
   assertNull(fedcmDriver.getFederatedCredentialManagementDialog());
+  // Additional assertions to verify dialog dismissal
+  assertFalse(fedcmDriver.isDialogVisible());
+  assertEquals("Expected message", fedcmDriver.getDialogMessage());
 

---

Suggestion 2:

Add assertions to verify account selection behavior to make the test more robust

Similar to the testDismissDialog method, add assertions in the testSelectAccount method to verify the dialog state and account selection behavior.

java/test/org/openqa/selenium/federatedcredentialmanagement/FederatedCredentialManagementTest.java [115-117]

 void testSelectAccount() {
   fedcmDriver.setDelayEnabled(false);
   assertNull(fedcmDriver.getFederatedCredentialManagementDialog());
+  // Additional assertions to verify account selection
+  assertTrue(fedcmDriver.isAccountSelected());
+  assertEquals("Expected account", fedcmDriver.getSelectedAccount());
 

---

Suggestion 3:

Ensure that the wait condition is met before capturing and asserting the event

Consider moving the WebDriverWait logic into the async context manager to ensure that the wait condition is met before the event is captured and asserted.

py/test/selenium/webdriver/common/bidi_tests.py [73-82]

 async with log.mutation_events() as event:
     pages.load("dynamic.html")
     driver.find_element(By.ID, "reveal").click()
     WebDriverWait(driver, 10).until(
         lambda d: d.find_element(By.ID, "revealed").value_of_css_property("display") != "none"
     )
 
-assert event["attribute_name"] == "style"
-assert event["current_value"] == ""
-assert event["old_value"] == "display:none;"
+    assert event["attribute_name"] == "style"
+    assert event["current_value"] == ""
+    assert event["old_value"] == "display:none;"

---

[Auto-generated best practices - 2025-01-24]