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

feat: Increase timeout to 5 minutes if a debugger is attached #1178

Merged
merged 6 commits into from
Aug 13, 2023

Conversation

linkdotnet
Copy link
Collaborator

Closes #1177

@egil
Copy link
Member

egil commented Aug 13, 2023

Why not disable timeout completely (TimeOut.Infinite)? That makes more sense to me.

I remember having that at some point. Cannot remember why I took it out again.

@linkdotnet
Copy link
Collaborator Author

Why not disable timeout completely (TimeOut.Infinite)? That makes more sense to me.

I remember having that at some point. Cannot remember why I took it out again.

Simplicity - we can not just have no timeout for timer.Change and it also doesn't except things like TimeSpan.MaxValue as it would throw an exception:

NotSupportedException
The dueTime or period parameter, in milliseconds, is greater than 4294967294.

Source

@linkdotnet
Copy link
Collaborator Author

So in the end we would use some "arbitrary" value. Sure we can put it to another, higher value.

@egil
Copy link
Member

egil commented Aug 13, 2023

Hmm you can just turn of the timer by passing in a https://learn.microsoft.com/en-us/dotnet/api/system.threading.timeout.infinite?view=net-7.0#system-threading-timeout-infinite as duetime/period.

@linkdotnet
Copy link
Collaborator Author

Fair point - updated the PR.

@egil
Copy link
Member

egil commented Aug 13, 2023

Looks good.

Think we need to add a code doc as well as docs updates that remark to all WaitFor methods that tells the user about this feature.

@linkdotnet
Copy link
Collaborator Author

I added the docs - fun fact, as WaitForElement and WaitForElements don't have an optional timeout (TimeSpan? timeOut) - the trick doesn't work here. I was tempted to introduce an overload, but I guess it might make more sense to align this in v2 rather than adding more stuff to v1.
Let me know if you think differently here.

egil
egil previously approved these changes Aug 13, 2023
Copy link
Member

@egil egil left a comment

Choose a reason for hiding this comment

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

Just update the remaining docs and then it looks alright.

Ps. Excuse the brevity. Reviewing from the car on my phone.

@linkdotnet
Copy link
Collaborator Author

Just update the remaining docs and then it looks alright.

Ps. Excuse the brevity. Reviewing from the car on my phone.

image

@linkdotnet linkdotnet merged commit c8202c4 into main Aug 13, 2023
4 checks passed
@linkdotnet linkdotnet deleted the feature/debugger-timeout branch August 13, 2023 19:12
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.

Increase WaitForTimeout when a debugger is attached
2 participants