[java] Fix FedCM command definition

[cbiesinger]

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:
#12096 (comment)

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.

---

PR Type

Bug fix, Tests

---

Description

• Restored session ID prefix to FedCM commands in AbstractHttpCommandCodec.java.
• Re-enabled FedCM tests by removing @NotYetImplemented annotation in FederatedCredentialManagementTest.java.
• Updated FedCM configuration to include login_url field in fedcm.json.

---

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

[qodo-merge-pro]

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

[qodo-merge-pro]

PR Code Suggestions ✨

Category	Suggestion	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] -"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] -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 Suggestion Impact: The commit added a verification that the dialog is dismissed by checking that accessing dialog properties throws NoAlertPresentException after dismissal code diff: + // Check that the dialog was indeed closed. Unable to get the dialog type since the dialog was + // closed. + assertThrows(NoAlertPresentException.class, dialog::getDialogType); 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 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 Suggestion Impact: The commit added assertions to verify account selection behavior by checking the account list size and selecting an account, though implemented differently than suggested code diff: + List<FederatedCredentialManagementAccount> accountList = dialog.getAccounts(); + assertThat(accountList.size()).isEqualTo(2); + dialog.selectAccount(1); 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 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

[titusfortner]

I think the tests are failing in Windows for being too long, and I think we're skipping them in our RBE. Can you check if we need to remove them from the skip test file in root?

[cbiesinger]

@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]

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.