Skip to content

[rb] Remove fedcm test guard due to fix in chrome #16119

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 11 commits into
base: trunk
Choose a base branch
from
Open

[rb] Remove fedcm test guard due to fix in chrome #16119

wants to merge 11 commits into from

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Aug 2, 2025 •
edited
Loading

User description

🔗 Related Issues

#16118 Enables the test for the Python bindings due to the same reason as this PR

It can only be merged after the pipeline uses Chrome 140

💥 What does this PR do?

This PR re-enables the fedCM ruby binding tests that had to be skipped due to the regression added on Chrome and described here: https://issues.chromium.org/issues/425801332

🔧 Implementation Notes

This PR just removes the standard guard added to the fedCM tests

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Tests

Description

  • Remove Chrome-specific test exclusion for FedCM tests

  • Re-enable previously skipped Ruby binding tests

  • Clean up test guard after Chrome regression fix

Diagram Walkthrough

flowchart LR
  A["FedCM Ruby Tests"] -- "Remove Chrome exclusion" --> B["Tests Now Enabled"]
  C["Chrome Regression Fix"] -- "Enables" --> B
Loading

File Walkthrough
Relevant files
Tests
fedcm_spec.rb
Remove Chrome exclusion from FedCM tests                                 

rb/spec/integration/selenium/webdriver/fedcm_spec.rb

  • Remove exclude parameter targeting Chrome browser
  • Clean up test guard referencing Chromium issue
  • Keep existing BiDi and browser exclusions intact
 +1/-3     

@selenium-ci selenium-ci added the C-rb Ruby Bindings label Aug 2, 2025
@qodo-merge-pro Qodo Merge Pro
Copy link
Contributor

qodo-merge-pro bot commented Aug 2, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

Test Exclusion

The test still excludes Chrome and Edge browsers in the exclusive parameter. Verify if this exclusion is still necessary given that the Chrome regression has been fixed and the test is being re-enabled.

 
describe FedCM, exclusive: [{bidi: false, reason: 'Not yet implemented with BiDi'}, {browser: %i[chrome edge]}] do
  let(:dialog) { driver.fedcm_dialog }

@qodo-merge-pro Qodo Merge Pro
Copy link
Contributor

qodo-merge-pro bot commented Aug 2, 2025

PR Code Suggestions ✨

No code suggestions found for the PR.

@aguspe aguspe added the R-awaiting external PR depends on another library for merging label Aug 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
C-rb Ruby Bindings R-awaiting external PR depends on another library for merging Review effort 1/5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aguspe @selenium-ci @titusfortner