[dotnet] use correct devtools session id after reinitialization by schrufygroovy · Pull Request #13768 · SeleniumHQ/selenium

schrufygroovy · 2024-04-03T07:52:18Z

User description

Description

After reinitialization of the dev tools session, that new active session id should be used.

Motivation and Context

Fixes regression issue: #13755

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)
PLEASE double check: in case a sessionId is provided to SendCommand() I removed the check if attachedTargetId == null and the reinitialization of the session. My assumption was in case the sessionId is given, we don't need a session re-initialization.

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.

Type

bug_fix, tests

Description

Enhanced DevTools session management by ensuring the session is reattached if not currently attached to a target before sending commands.
Added a new test to ensure that closing a tab does not break the DevTools session, addressing regression issue [🐛 Bug]: dotnet | DevTools | closing tab leaves broken DevToolsSession behind. Cannot be repaired/freshly created #13755.

Changes walkthrough

Relevant files

Enhancement

DevToolsSession.cs `Improve DevTools Session Management in Selenium WebDriver` dotnet/src/webdriver/DevTools/DevToolsSession.cs Modified `SendCommand` to reattach session if `attachedTargetId` is null before sending a command. Removed redundant session reattachment logic from overloaded `SendCommand`.	+8/-8

Tests

DevToolsTabsTest.cs `Add Test for Verifying DevTools Session After Closing Tab` dotnet/test/common/DevTools/DevToolsTabsTest.cs Added a new test `ClosingTabDoesNotBreakDevToolsSession` to verify DevTools session remains active after closing a tab.	+32/-0

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

CLAassistant · 2024-04-03T07:52:25Z

All committers have signed the CLA.

qodo-code-review · 2024-04-03T07:52:36Z

PR Description updated to latest commit (bacefcc)

Copy walkthrough table to "Files Changed" Tab

qodo-code-review · 2024-04-03T07:52:44Z

PR Review

⏱️ Estimated effort to review [1-5]	2, because the changes are focused and well-scoped, impacting DevTools session management and adding a specific test case. The logic is straightforward, and the added test directly addresses the regression issue.
🧪 Relevant tests	Yes
🔍 Possible issues	Possible Regression: Removing the check for `attachedTargetId == null` in the `SendCommand` overload that accepts a `sessionId` might introduce regressions if there are scenarios where reinitialization was necessary despite having a `sessionId`. It's important to ensure that all use cases have been considered and that this change doesn't inadvertently affect existing functionality.
🔒 Security concerns	No

✨ Review tool usage guide:

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

qodo-code-review · 2024-04-03T07:53:31Z

PR Code Suggestions

Category	Suggestions
Enhancement	Add a check for an active connection before reinitializing the session. Consider checking if `this.connection` is not null and active before attempting to reattach the session in the `SendCommand` method. This can prevent unnecessary reinitialization if the connection is already active. dotnet/src/webdriver/DevTools/DevToolsSession.cs [244-247] -if (this.attachedTargetId == null) +if (this.attachedTargetId == null && (this.connection == null \|\| !this.connection.IsActive)) { LogTrace("Session not currently attached to a target; reattaching"); await this.InitializeSession().ConfigureAwait(false); }
Enhancement	Simplify the method signature by omitting explicit default value for CancellationToken. Avoid using `default(CancellationToken)` explicitly, as it is the default value for optional CancellationToken parameters. Simplify the method signature by omitting it. dotnet/src/webdriver/DevTools/DevToolsSession.cs [242] -public async Task<JToken> SendCommand(string commandName, JToken commandParameters, CancellationToken cancellationToken = default(CancellationToken), int? millisecondsTimeout = null, bool throwExceptionIfResponseNotReceived = true) +public async Task<JToken> SendCommand(string commandName, JToken commandParameters, CancellationToken cancellationToken = default, int? millisecondsTimeout = null, bool throwExceptionIfResponseNotReceived = true)
Possible issue	Rename the overloaded method to prevent potential stack overflow. To avoid potential stack overflow errors, consider renaming the overloaded `SendCommand` method to something more descriptive and to differentiate it from the recursive call. dotnet/src/webdriver/DevTools/DevToolsSession.cs [242] -public async Task<JToken> SendCommand(string commandName, JToken commandParameters, CancellationToken cancellationToken = default(CancellationToken), int? millisecondsTimeout = null, bool throwExceptionIfResponseNotReceived = true) +public async Task<JToken> SendCommandWithSessionCheck(string commandName, JToken commandParameters, CancellationToken cancellationToken = default(CancellationToken), int? millisecondsTimeout = null, bool throwExceptionIfResponseNotReceived = true)
Best practice	Use `Assert.DoesNotThrowAsync` for clearer assertion of non-throwing async operations. Consider using `Assert.DoesNotThrowAsync` instead of `Assert.That` with `Throws.Nothing` for better readability and intent clarity when asserting that an async operation does not throw an exception. dotnet/test/common/DevTools/DevToolsTabsTest.cs [24-28] -Assert.That( - async () => { - await domains.Console.Enable(); - }, - Throws.Nothing +Assert.DoesNotThrowAsync( + async () => await domains.Console.Enable() );
Maintainability	Use `nameof` for parameter names in logs for better maintainability. Use `nameof` for parameter names in exceptions and logs to ensure refactoring tools update them automatically and to avoid potential typos. dotnet/src/webdriver/DevTools/DevToolsSession.cs [246] -LogTrace("Session not currently attached to a target; reattaching"); +LogTrace($"{nameof(this.attachedTargetId)} not currently attached to a target; reattaching");

✨ Improve tool usage guide:

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.

dotnet/src/webdriver/DevTools/DevToolsSession.cs

Fix regression issue, appeared in v4.17. Closing a tab breaks the devtools session Fixes SeleniumHQ#13755

nvborisenko · 2024-04-05T21:22:46Z

CI failure is not related to the changes in this PR, merging it.

Thanks @schrufygroovy for your contribution!

p0deje · 2024-04-06T20:33:58Z

@schrufygroovy Could you please sign the contributor license agreement via https://cla-assistant.io/SeleniumHQ/selenium?pullRequest=13768?

schrufygroovy · 2024-04-08T06:27:38Z

@schrufygroovy Could you please sign the contributor license agreement via https://cla-assistant.io/SeleniumHQ/selenium?pullRequest=13768?

done

qodo-code-review bot added tests labels Apr 3, 2024

qodo-code-review bot added the Review effort [1-5]: 2 label Apr 3, 2024

schrufygroovy force-pushed the fix-devtools-dealing-with-closing-tab branch from bacefcc to e306d50 Compare April 3, 2024 07:57

schrufygroovy commented Apr 3, 2024

View reviewed changes

dotnet/src/webdriver/DevTools/DevToolsSession.cs Show resolved Hide resolved

[dotnet] use correct devtools session id after reinitialization

c23158c

Fix regression issue, appeared in v4.17. Closing a tab breaks the devtools session Fixes SeleniumHQ#13755

nvborisenko force-pushed the fix-devtools-dealing-with-closing-tab branch from e306d50 to c23158c Compare April 5, 2024 19:45

nvborisenko self-requested a review April 5, 2024 20:03

nvborisenko approved these changes Apr 5, 2024

View reviewed changes

nvborisenko mentioned this pull request Apr 5, 2024

[dotnet] Added function for sending multiple DevTools command #13539

Closed

8 tasks

nvborisenko merged commit 12ed6cc into SeleniumHQ:trunk Apr 5, 2024

MiguelPelegrina mentioned this pull request Jun 6, 2024

[Snyk] Upgrade selenium-webdriver from 4.20.0 to 4.21.0 MiguelPelegrina/java_school_online_store_frontend#16

Merged

x-way mentioned this pull request Jun 10, 2024

[Snyk] Upgrade selenium-webdriver from 4.18.1 to 4.21.0 x-way/cal-heatmap#451

Closed

farmmanic mentioned this pull request Jun 14, 2024

[Snyk] Upgrade selenium-webdriver from 4.3.0 to 4.21.0 farmmanic/social-analyzer#15

Open

LunarArtemis mentioned this pull request Jun 22, 2024

[Snyk] Upgrade selenium-webdriver from 4.19.0 to 4.21.0 LunarArtemis/tuktuk#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[dotnet] use correct devtools session id after reinitialization#13768

[dotnet] use correct devtools session id after reinitialization#13768
nvborisenko merged 1 commit intoSeleniumHQ:trunkfrom
schrufygroovy:fix-devtools-dealing-with-closing-tab

schrufygroovy commented Apr 3, 2024 •

edited by qodo-code-review bot

Loading

Uh oh!

CLAassistant commented Apr 3, 2024 •

edited

Loading

Uh oh!

qodo-code-review bot commented Apr 3, 2024

Uh oh!

qodo-code-review bot commented Apr 3, 2024

Uh oh!

qodo-code-review bot commented Apr 3, 2024

Uh oh!

Uh oh!

nvborisenko commented Apr 5, 2024

Uh oh!

p0deje commented Apr 6, 2024

Uh oh!

schrufygroovy commented Apr 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

schrufygroovy commented Apr 3, 2024 • edited by qodo-code-review bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Description

Motivation and Context

Types of changes

Checklist

Type

Description

Changes walkthrough

Uh oh!

CLAassistant commented Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qodo-code-review bot commented Apr 3, 2024

Uh oh!

qodo-code-review bot commented Apr 3, 2024

PR Review

Uh oh!

qodo-code-review bot commented Apr 3, 2024

PR Code Suggestions

Uh oh!

Uh oh!

nvborisenko commented Apr 5, 2024

Uh oh!

p0deje commented Apr 6, 2024

Uh oh!

schrufygroovy commented Apr 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

schrufygroovy commented Apr 3, 2024 •

edited by qodo-code-review bot

Loading

CLAassistant commented Apr 3, 2024 •

edited

Loading