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

Timeouts don't work for all cases #162

Open
HunterJKI opened this issue May 8, 2023 · 2 comments
Open

Timeouts don't work for all cases #162

HunterJKI opened this issue May 8, 2023 · 2 comments
Labels

Comments

@HunterJKI
Copy link

Timeout is not wired in Caraya\classes\Test Runner\onTestExecution.vi
This is misleading, and may cause confusuon.

image

@HunterJKI HunterJKI added the bug label May 8, 2023
@francois-normandin
Copy link
Collaborator

All this overridable family of VI passes the public RunTest timeout to the inner onTestExecution. I agree that none of the methods provided by Caraya's core classes make use of it, but since the framework is extendable by developers, the timeout input is passed down the chain to provide developers with the flexibility to make use of it. Not passing it down

The alternative is to remove the terminal (breaking change) and inject it before the overridable VIs in the thread.
In this instance, I'll argue against making a change and call that a feature request: we could implement onTestExecution as an asynchronous process and force-terminate it when time is elapsed.

image

@HunterJKI
Copy link
Author

Fair point; having it for overrides is valuable, but it is misleading for the vast majority of cases that don't override. In my case I piped a CLI timeout argument all the way through my CI infrastructure before realizing it was not implanted.

Since it does not behave as most developer would expect, I think that's at least a documentation defect.

Options

X. remove the terminal - I agree this is a bad idea, let's not break any overrides.

A. Implementing a real timeout is the best case. We could set the default to -1 so the default behavior would be the same as the current code. This will take a decent amount of work and testing, and I'm sure this will cause at least some users to have to go update long dormant wired timeouts.

B. Alternatively it could default to -1, and throw an error (in the base class) if was was set to anything else: `Error timeout is not implemented in the base version of this VI". This would also throw errors for anyone who had wired it up and assumed it worked, but that should be a smaller subset, and we're actually identifying incorrect usage of the current API.

C. Update documentation to make it clear that timeout is not yet implemented, and only intended for extensibility. There is currently no VI documentation for any of the test runner VIs.

I think A is the best case, but I am not (yet) signing up to implement it :)
Both A and B may cause people to have to update timeouts in mature test setups.
C doesn't fix it, doesn't break anyone, but at least makes it a "feature" not a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants