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

OSOE-430: Improve stability directly after a page load #316

Merged
merged 7 commits into from
Oct 18, 2023

Conversation

sarahelsaig
Copy link
Member

@sarahelsaig sarahelsaig commented Oct 7, 2023

}
catch (StaleElementReferenceException) when (notLast)
{
Task.Delay(TimeSpan.FromSeconds(1)).Wait();
Copy link
Member

Choose a reason for hiding this comment

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

Why not Thread.Sleep(TimeSpan.FromSeconds(1));?

}
catch (StaleElementReferenceException) when (notLast)
{
Task.Delay(TimeSpan.FromSeconds(1)).Wait();
Copy link
Member

Choose a reason for hiding this comment

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

Why not Thread.Sleep(TimeSpan.FromSeconds(1));?

{
Task.Delay(TimeSpan.FromSeconds(1)).Wait();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Why not throw new InvalidOperationException("Impossible to reach."); here? Like below.

Copy link
Member Author

Choose a reason for hiding this comment

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

As the text says, that's impossible to reach. The other methods need it because they expect to return a value so if I don't throw at the end then I get a compile time error even though it's really not a possible execution path.

}
}

throw new InvalidOperationException("Impossible to reach.");
Copy link
Member

Choose a reason for hiding this comment

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

A custom exception would be better or a constant for the message at least.

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be an unreasonable effort for a theoretically impossible scenario.

@dministro dministro merged commit 4e5b18f into dev Oct 18, 2023
1 check passed
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.

2 participants