Skip to content
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.

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

[dotnet] Simplify and add examples for bidi network intercepring #1467

Merged
merged 6 commits into from
Sep 16, 2023

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Sep 5, 2023

Description

Provide examples to intercept network in .NET

Motivation and Context

Fixing lack of docs

Types of changes

  • Change to the site (I have double-checked the Netlify deployment, and my changes look good)
  • Code example added (and I also added the example to all translated languages)
  • Improved translation
  • Added new translation (and I also added a notice to each document missing translation)

Checklist

  • I have read the contributing document.
  • I have used hugo to render the site/docs locally and I am sure it works.

@netlify
Copy link

netlify bot commented Sep 5, 2023

Deploy Preview for selenium-dev ready!

Name Link
🔨 Latest commit fc5ca0a
🔍 Latest deploy log https://app.netlify.com/sites/selenium-dev/deploys/6503cde871d12e0008874cb1
😎 Deploy Preview https://deploy-preview-1467--selenium-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@diemol
Copy link
Member

diemol commented Sep 5, 2023

We need to check that failing test...

@nvborisenko
Copy link
Member Author

I don't mind.

@titusfortner
Copy link
Member

We were getting the known location of the driver from PATH, but we can't do that any more. The goal is to be able to show that it will use a driver passed in. Maybe we download a different driver to cache and pass it in?

@nvborisenko
Copy link
Member Author

I reproduced it locally:

var service = ChromeDriverService.CreateDefaultService();
service.DriverServicePath = "C:\\Temp";

driver = new ChromeDriver(service);

And getting:

System.ArgumentNullException : Value cannot be null. (Parameter 'path2')
Path.Combine(String path1, String path2)
DriverService.Start() line 266

@nvborisenko
Copy link
Member Author

Do you think changes in this PR caused errors? I believe no. I am not planning to fix failed tests in scope of this PR.

@titusfortner titusfortner force-pushed the dotnet-network-examples branch 2 times, most recently from f84d8f8 to 8f4facc Compare September 10, 2023 19:29
Copy link
Member

@titusfortner titusfortner left a comment

Choose a reason for hiding this comment

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

Please replace all the code in the md files with:

{{< gh-codeblock path="examples/dotnet/SeleniumDocs/ChromeDevTools/NetworkInterceptor.cs#L13-L24" >}}

Because all the other tabs are using automatic code formatting, we need to put text=true in the tabpane and code=true in the CSharp tab.

I need to (soon) go through and update all the marking so that we know what needs to be fixed.

@nvborisenko
Copy link
Member Author

Interesting, I created PR to add examples for BiDi network intercepting. BUT .net binding doesn't implement any BiDi protocol. @titusfortner Do you still think is it valid example?

@jimevans
Copy link
Member

We need to make a distinction here when we use the term "BiDi" in Selenium. There are features that are bidirectional in nature, and can be referred to as "BiDi" features. Network interception is one of these types of features. Note very carefully that use of the term "BiDi" in this context has no bearing whatsoever on what protocol is used to implement that feature. Ideally, it will one day be implemented using the WebDriver BiDi protocol; currently it's still a "BiDi" feature, but implemented using CDP. So, in this context, the .NET bindings most certainly do implement a "BiDi protocol," it's just CDP, not WebDriver BiDi.

@nvborisenko
Copy link
Member Author

Thank you @jimevans , then show must go on.

@nvborisenko
Copy link
Member Author

nvborisenko commented Sep 12, 2023

Please replace all the code in the md files with:

{{< gh-codeblock path="examples/dotnet/SeleniumDocs/ChromeDevTools/NetworkInterceptor.cs#L13-L24" >}}

Because all the other tabs are using automatic code formatting, we need to put text=true in the tabpane and code=true in the CSharp tab.

I need to (soon) go through and update all the marking so that we know what needs to be fixed.

But why java examples are not done in this way? Java examples are "hardcoded", I would like to do the same. For instance I don't want to include using statements in the examples just to achieve more simplified way how it will look like for end user.

And, what if I change C# source code like adding new lines in the code, will it update examples accordingly?

@titusfortner
Copy link
Member

Because the Java examples are wrong.
I just went through to try to make this process more transparent and prep things for people to make it easier: #1473

When lines of code are updated in the examples, we need to make sure that the md files all get updated as necessary.
Yes, it's a pain, but it is really nice when everything is working (like everything in Selenium project)

@diemol
Copy link
Member

diemol commented Sep 13, 2023

Also, this guarantees that code examples are working.

@titusfortner
Copy link
Member

We need to keep text=true on those tabs.

Docsy had it right in their previous version when they had code be opt-in instead of text as opt-in. Without text=true, the website treats whatever you write as code

@titusfortner
Copy link
Member

I've been futzing with the docs and moving things around. I just moved the examples to the "correct" location (the test file locations are supposed to match the page locations, but the BiDi/CDP examples haven't been following that convention.

Since you created a 3rd test, I added a new section in docs to include a reference to it. Hmm, I think I misspelled something, it isn't building, will investigate.

@titusfortner
Copy link
Member

@titusfortner titusfortner merged commit 724105d into SeleniumHQ:trunk Sep 16, 2023
9 checks passed
@titusfortner
Copy link
Member

Thanks @nvborisenko

@nvborisenko nvborisenko deleted the dotnet-network-examples branch December 14, 2023 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants