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

Add timeout for Getter keywords which use a selector #3371

Open
simonmeggle opened this issue Jan 12, 2024 · 6 comments
Open

Add timeout for Getter keywords which use a selector #3371

simonmeggle opened this issue Jan 12, 2024 · 6 comments

Comments

@simonmeggle
Copy link
Contributor

I often catch myself writing something like this:

Click  button
Get Text  table.center-grid  matches  sometext

(= click somewhere and do a text assertion)

The problem is that often times the pages takes time to load and Get Text is executed too early and fails then.

I would therefore always have to write an element-visible assertion before such text assertions - because only Wait For Elements State allows me to set a custom timeout (e.g. 30s):

Click  button
Wait For Elements State  table.center-grid  state=visible  timeout=30s   # 👈
Get Text  table.center-grid  matches  sometext

It does not feel right to write two assertions (Wait For / Get Text) for exactly the same selector, just to achieve one goal (to assert a text).

retry_assertions_for (as suggested by @mkorpela) is not a complete solution to the problem because each assertion can require another setting. The is no global setting which is right for all of them.

Would it be an idea to add an optional timeout argument to the Get-keywords which have a selector as argument?

Thanks -
Simon


(Question was asked originally on Slack: https://robotframework.slack.com/archives/C015KB1QSDN/p1703179017661919)

@aaltat
Copy link
Member

aaltat commented Jan 16, 2024

@Snooz82 did you have an opinion about this feature request?

@Snooz82
Copy link
Member

Snooz82 commented Jan 17, 2024

I am really cautious about adding more arguments to so many keywords.

I would really want to take some time to discuss these.
the problem is always that reverting these decisions is nearly impossible. And I am at the moment digging through SeleniumLibrary and it has so many inconsistencies and is in some places so komplex because of all the hidden magic in the code below.

this timeout would interfere with the normal timeout. And assertion timeout etc. we really do not want to end where SL is with all its timeouts.

@simonmeggle
Copy link
Contributor Author

@Snooz82 - I well understand your hesitation. Once implemented, it's almost impossible to revert.

But perhaps it helps to remember that this applies to pretty much any implementation you choose.
I'm just saying that even if the argument is important, it shouldn't be overemphasized. Otherwise every step forward gets even harder.

I wouldn't see any "hidden magic" here - if there is a timeout, I can use it, but I don't have to.
Would such a timeout really lead to a conflict?
I think not.
The keywords can still use the default assertion timeout of retry_assertions_for=1s.
But there must be some way to customize that on keyword level.

Anyway...
Assuming it is "best practice" to verify the visibility of the elements to be checked (👈) before each text assertion...

Click  button
Wait For Elements State  table.center-grid  state=visible  timeout=30s   # 👈
Get Text  table.center-grid  matches  sometext

, I wonder why there is a timeout in assertions at all...? 🤔

In my opinion, a global fixed timeout of 1s in assertions is neither sensible nor practicable.

@Snooz82
Copy link
Member

Snooz82 commented Feb 10, 2024

To add a timeout to every assertion keyword, we would probably need to wrap the actual keyword and the return verify_assertion(value, assertion_operator, assertion_expected, f"Property {property}", message, formatter) with an assertion decorator.

That could create some confusions because the argument interface would be different in python than in RF.

@Snooz82
Copy link
Member

Snooz82 commented Feb 17, 2024

@simonmeggle now that i started implementing that and me seeing some of my team mates using waiters for assertion, what you are demanding already works.

Just use Wait For Condition

Click  button
Wait For Condition    Text     table.center-grid     matches     sometext   timeout=30s

Therefore i would close that issue, after make some documentation changes explaining that principle.
Because adding a new argument to many keywords when we have implemented that already makes no sense to me.

@simonmeggle
Copy link
Contributor Author

Hey @Snooz82 , that sounds like a good solution to the problem. Thanks for the hint!

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

No branches or pull requests

3 participants