search

SeleniumHQ / selenium

A browser automation framework and ecosystem.
https://selenium.dev
Apache License 2.0
29.76k stars 8.02k forks source link

Fix FedCM command definition #14070

Open cbiesinger opened 1 month ago

cbiesinger commented 1 month ago

User description

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

In commit 7ad44ee the session ID prefix was removed from the FedCM commands. This change restores it.

This lets us re-enable the FedCM tests, which this change does as well.

Motivation and Context

This test failure was pointed out here: https://github.com/SeleniumHQ/selenium/pull/12096#issuecomment-2017760822

Types of changes

Checklist

PR Type

Bug fix, Tests

Description

Changes walkthrough 📝

Relevant files
Bug fix
AbstractHttpCommandCodec.java
Restore session ID prefix in FedCM command definitions.   
java/src/org/openqa/selenium/remote/codec/AbstractHttpCommandCodec.java
  • Restored session ID prefix to FedCM commands.
  • Updated command definitions to include session ID.
    		•  +9/-8     
    fedcm.json
    Update FedCM configuration to include `login_url`.             
    common/src/web/fedcm/fedcm.json - Added `login_url` field to FedCM configuration.
    		+2/-1     
    Tests
    FederatedCredentialManagementTest.java
    Re-enable FedCM tests by removing `@NotYetImplemented` annotation.
    java/test/org/openqa/selenium/federatedcredentialmanagement/FederatedCredentialManagementTest.java
  • Removed @NotYetImplemented annotation from FedCM tests.
  • Re-enabled testDismissDialog and testSelectAccount tests.
    		•  +0/-7     

    💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    codiumai-pr-agent-pro[bot] commented 1 month ago

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5] 2, because the changes are straightforward and localized to specific files, involving simple updates to method calls and JSON configurations. The logic is not complex, and the changes are well-described in the PR description.
    🧪 Relevant tests Yes
    ⚡ Possible issues Possible Redundancy: The JSON file `fedcm.json` now contains both "signin_url" and "login_url" with the same endpoint "/signin". If these fields are meant to serve the same purpose, one of them might be redundant unless required for backward compatibility.
    🔒 Security concerns No
    codiumai-pr-agent-pro[bot] commented 1 month ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Remove redundant login_url field if it is not intended to be different from signin_url ___ **The signin_url and login_url fields have the same value. If they are intended to be
    different, ensure they are correctly set; otherwise, consider removing one to avoid
    redundancy.** [common/src/web/fedcm/fedcm.json [5-6]](https://github.com/SeleniumHQ/selenium/pull/14070/files#diff-a01d9d9718317c5012496046288cc44a5d4841657663e56dc013b74d80142125R5-R6) ```diff -"signin_url": "/signin", -"login_url": "/signin" +"signin_url": "/signin" ```
    Suggestion importance[1-10]: 8 Why: Removing redundant fields reduces confusion and potential errors in configuration, making this a valuable suggestion for maintainability and clarity.
    		8
    Define endpoint paths as constants to improve maintainability and reduce the risk of typos ___ **To improve maintainability and avoid potential issues with hardcoded strings, consider
    defining the endpoint paths as constants. This will make the code easier to update and
    reduce the risk of typos.** [java/src/org/openqa/selenium/remote/codec/AbstractHttpCommandCodec.java [200-208]](https://github.com/SeleniumHQ/selenium/pull/14070/files#diff-640f7348338f49b2894694516ec1682936ca87803d7602531a56de5b537d7745R200-R208) ```diff -String fedcm = sessionId + "/fedcm"; +final String FEDCM_BASE_PATH = "/fedcm"; +String fedcm = sessionId + FEDCM_BASE_PATH; defineCommand(CANCEL_DIALOG, post(fedcm + "/canceldialog")); defineCommand(SELECT_ACCOUNT, post(fedcm + "/selectaccount")); defineCommand(CLICK_DIALOG, post(fedcm + "/clickdialogbutton")); defineCommand(GET_ACCOUNTS, get(fedcm + "/accountlist")); defineCommand(GET_FEDCM_TITLE, get(fedcm + "/gettitle")); defineCommand(GET_FEDCM_DIALOG_TYPE, get(fedcm + "/getdialogtype")); defineCommand(SET_DELAY_ENABLED, post(fedcm + "/setdelayenabled")); defineCommand(RESET_COOLDOWN, post(fedcm + "/resetcooldown")); ```
    Suggestion importance[1-10]: 7 Why: Using constants for endpoint paths enhances maintainability and reduces typo errors, which is a good practice, especially in a large codebase.
    		7
    Enhancement
    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]](https://github.com/SeleniumHQ/selenium/pull/14070/files#diff-2e0a0d71dc441c4816d5588bbd502c512fed777c28cc8f5d389284e679671d07R83-R85) ```diff void testDismissDialog() { fedcmDriver.setDelayEnabled(false); assertNull(fedcmDriver.getFederatedCredentialManagementDialog()); + // Additional assertions to verify dialog dismissal + assertFalse(fedcmDriver.isDialogVisible()); + assertEquals("Expected message", fedcmDriver.getDialogMessage()); ```
    Suggestion importance[1-10]: 6 Why: Adding more assertions would indeed make the test more robust by verifying additional expected behaviors, although the existing test already checks the main functionality.
    		6
    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]](https://github.com/SeleniumHQ/selenium/pull/14070/files#diff-2e0a0d71dc441c4816d5588bbd502c512fed777c28cc8f5d389284e679671d07R115-R117) ```diff void testSelectAccount() { fedcmDriver.setDelayEnabled(false); assertNull(fedcmDriver.getFederatedCredentialManagementDialog()); + // Additional assertions to verify account selection + assertTrue(fedcmDriver.isAccountSelected()); + assertEquals("Expected account", fedcmDriver.getSelectedAccount()); ```
    Suggestion importance[1-10]: 6 Why: Similar to the previous suggestion, adding assertions enhances the test's robustness by checking more conditions, which is beneficial for ensuring the correct behavior of the system.
    		6
    cbiesinger commented 1 week ago

    @titusfortner while I couldn't follow your comment (if they fail on Windows shouldn't they be in the skipped tests file?), I have pushed an update that removes the test from there

    titusfortner commented 1 week ago

    RBE runs on Linux, so I want to see if they pass. As for the Windows failures, if these are the affected tests it's an issue of a bazel failure not a JUnit failure. Let's see what the test results show.