search

uyar / calico

GNU General Public License v3.0
8 stars 4 forks source link

Defining Global Timeout #2

Closed talha252 closed 4 years ago

talha252 commented 6 years ago

Some programs we test is not computationally challenging. Thus, they perform the given actions quickly. The current timeout of Calico is what pexcept uses which is 30 seconds unless we override it in the comment part. However, if want to use the same timeout value for multiple places (especially for trivial prompts), it is a real burden. We already can define variables in _define clause, I think we can add global timeout support to it.

ghost commented 6 years ago

That is reasonable. I think there should be multiple timeout settings, from more general to more specific:

  1. A default setting for Calico that's used if nothing else is given. I hadn't checked it but at the moment, this is the default timeout of pexpect, which you say is 30 secs. This is way too high for our use. I'm thinking something less than 5 secs, probably around 2 secs.

  2. A command line argument that will override this default.

  3. A setting in the test specification file's _define section which will be applied if not overridden in the test case.

  4. A timeout comment in a test case.

In case of multiple rules for a test case, the more specific one wins.

talha252 commented 6 years ago

I think 2 secs timeout sounds good. However, I think it should be documented very clearly with shiny warning signs, since it may cause false negatives for some users.

I have a question regarding multi-level timeout scheme. Assuming the given order above, don't you think "command line argument timeout" and "setting timeout in specs" has the same level in the hierarchy? I mean If I set timeout value in spec and then I run my program with the timeout argument, which timeout will be used? Currently, it's timeout in specs but I think it can be the other way around as well

ghost commented 6 years ago

You mean 2 and 3? You may be right. I think 4 should win in any case but between 2 and 3, I'm undecided. I still feel like 3 should be the winner; I see 2 only as an override for 1. Maybe calling the cli parameter default timeout instead of just timeout will reduce the confusion.

talha252 commented 6 years ago

Makes sense to define as default timeout rather than timeout. Yet again, it should be documented clearly to prevent any confusion.

ghost commented 6 years ago

Until we come up with a clean design, I think it's better to support only one of 2 and 3. I'm more inclined to implement 2 and postpone 3 at the moment because it's not a variable substitution as is the case with other definitions in the _define section. That will require special handling of the variable and should be better thought out.

talha252 commented 6 years ago

It turns out the timeout issue is worse than I thought. Currently, Calico passes None as a timeout value to the expect method if no timeout is specified. According to pexpect documentation1 and source code 2 (yes, I looked up to the source code, the documentation was not clear), if None is given to the timeout parameter of expect method, this overrides the default timeout value (which is 30 seconds as I mentioned earlier) and causes the expect method to wait indefinitely. We should pass -1 to the timeout parameter to not to override the default timeout value. I am currently working on a patch to fix this and the timeout issue in general.