Skip to content

feature/RM-9546 update selenium to 4.35.0 #1366

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 7 commits into
base: develop
Choose a base branch
from
Open

feature/RM-9546 update selenium to 4.35.0 #1366

wants to merge 7 commits into from

Conversation

Ottorium
Copy link
Contributor

No description provided.

MichaelKetting
MichaelKetting requested changes Aug 25, 2025
View reviewed changes
Copy link
Member

@MichaelKetting MichaelKetting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something went sideways, the PR has the old messages and syntax, not the ones from the previous PR.

@Ottorium Ottorium force-pushed the feature/RM-9546-update-Selenium-to-4.35.0 branch from a1ef5d0 to 190a116 Compare August 25, 2025 14:38
@MichaelKetting
Copy link
Member

MichaelKetting commented Aug 28, 2025 •
edited by dkudernatsch
Loading

  • Resolve the Bidi Connection problem with Selenium Firefox

@dkudernatsch dkudernatsch force-pushed the feature/RM-9546-update-Selenium-to-4.35.0 branch 3 times, most recently from bb7010e to 709d9f1 Compare September 11, 2025 08:10
@dkudernatsch
Copy link
Contributor

dkudernatsch commented Sep 11, 2025 •
edited
Loading

  • Resolve the Bidi Connection problem with Selenium Firefox

The firefox problem seems to be resolved, my best guess is, that there were not disposed WebSocket connections.

@dkudernatsch dkudernatsch force-pushed the feature/RM-9546-update-Selenium-to-4.35.0 branch 2 times, most recently from 709d9f1 to 9a63aee Compare September 11, 2025 08:17
MichaelKetting
MichaelKetting requested changes Sep 11, 2025
View reviewed changes
Copy link
Member

@MichaelKetting MichaelKetting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new code looks okay, I got some questions about how things actually work together.

I do have a big request: now that we got a working solution, please split the bidi-stuff into it's own PR and jira ticket (bug?) and let's do this new ticket first. I don't want to hide the semantic changes in a simple Selenium upgrade ticket, in particular since it is not actually dependet on the update and might have been incorrect from the start.

@dkudernatsch dkudernatsch force-pushed the feature/RM-9546-update-Selenium-to-4.35.0 branch 4 times, most recently from 8629ca0 to 1c41ab8 Compare September 24, 2025 08:14
@MichaelKetting MichaelKetting force-pushed the feature/RM-9546-update-Selenium-to-4.35.0 branch from 044dfa0 to b493069 Compare September 29, 2025 06:53
@dkudernatsch dkudernatsch force-pushed the feature/RM-9546-update-Selenium-to-4.35.0 branch from b493069 to 02c8508 Compare October 7, 2025 12:50
@dkudernatsch dkudernatsch force-pushed the feature/RM-9546-update-Selenium-to-4.35.0 branch from d47800e to 9940570 Compare October 8, 2025 08:36
@dkudernatsch dkudernatsch self-assigned this Oct 8, 2025
MichaelKetting
MichaelKetting requested changes Oct 8, 2025
View reviewed changes
Copy link
Member

@MichaelKetting MichaelKetting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please merge commits where the selenium version gest updated.

Some small design discussions we should desicde before this is done.

Remotion/Web/Development.WebTesting.IntegrationTests/WebTestHelperTest.cs
var webTestHelper = WebTestHelper.CreateFromConfiguration<CustomWebTestConfigurationFactory>();
if (webTestHelper.BrowserConfiguration.IsFirefox())
{
Assert.Ignore();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the jira issue number where this will get fixed and a short explanation in the ignore about why it's ignored.

Remotion/Web/Development.WebTesting/BrowserSession/BrowserSessionBase.cs
Comment on lines +106 to +114
// Accept all user prompts as they come up - IWebTestHelper.AcceptPossibleModalDialog() does not work with BiDi
// because the WebTest-Thread is not continued when a user prompt is shown.
_promptSubscription = _bidiConnection.BrowsingContext.OnUserPromptOpenedAsync(args =>
args.BiDi.BrowsingContext.HandleUserPromptAsync(args.Context, new HandleUserPromptOptions { Accept = true, Timeout = _bidiTimeout}).GetAwaiter().GetResult(),
new BrowsingContextsSubscriptionOptions(new SubscriptionOptions
{
Timeout = _bidiTimeout
}))
.GetAwaiter().GetResult();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I remember a discussion about making this conditional based on there actually being a user propmt expected right now. Did we nix this completely or just deferr? if it's deferred we should create a jira ticket and refernece the issue number here with a short explaintion that this will get aligned with the correct handling semantics in a later iteration.

Remotion/Web/Development.WebTesting/BrowserSession/BiDiBrowserLogProvider.cs
private readonly TimeSpan _bidiTimeout = TimeSpan.FromSeconds(1);

public BiDiBrowserLogProvider (IDriver driver)
public BiDiBrowserLogProvider (IBrowserSession browserSession)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

design question: should we have an interface smaller than IBrowserSession (IBidiConnectionService) or some such?

Remotion/Web/Development.WebTesting/BrowserSession/BiDiBrowserLogProvider.cs

private readonly ConcurrentQueue<BrowserLogEntry> _logEntries = new();
private readonly Subscription _eventSubscription;
private readonly TimeSpan _bidiTimeout = TimeSpan.FromSeconds(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is duplicated with BrowserSessionBase. should we move this into the IBrowserSession (or IBidiConnectionManager) interface to avoid duplication?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaelKetting MichaelKetting MichaelKetting requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@dkudernatsch dkudernatsch

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Ottorium @MichaelKetting @dkudernatsch