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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly handle test timeouts (#54) #55

Merged
merged 1 commit into from
Feb 7, 2023
Merged

Conversation

ZloeSabo
Copy link
Contributor

@ZloeSabo ZloeSabo commented Sep 6, 2022

This PR fixes #54. The actual issue is not the output size; The runner currently does not support test timeouts.

Currently, the rust test suite itself does not support setting time limits to abort a test that runs for too long (see rust-lang/rfcs#2798).

There's an option --ensure-time, but it only controls the time threshold to mark a test failed (generates event like { "type": "test", "name": "tests::test_empty_a", "event": "failed", "exec_time": 7.000362262, "reason": "time limit exceeded" }) and does not abort the test itself.

So I see three ways of dealing with badly written code:

  • wait for cargo to abort the hanging test after 120 secs. In this case, it emits timeout event like this: { "type": "test", "event": "timeout", "name": "test_empty" } that could be parsed with the transformer. The downside is that for two minutes the test runner is going to be blocked. Not sure how bad it is for excercism, but generally blocked workers are no good. 馃槃
  • implement a custom test harness with support for test timeouts. While rust supports this, it looks to me like a very labor-intensive way to solve the problem.
  • use an external utility to abort the test runner after some time.

It seems to me, that the latter is currently the best way to solve the issue. I've taken the inspiration from the rust playground and changed the code to return {status: "error", message:"Tests timed out"} if the test suite does not finish after 5 sec.

Given the speed of rust, a timeout of 5 seconds seems reasonable to me, but please let me know if some shorter/longer fits better.

@iHiD
Copy link
Member

iHiD commented Sep 6, 2022

@ZloeSabo Thanks so much for this! It requires a review by @ErikSchierboom, who's on his summer break and won't be back for 2 more weeks, so I'll have to ask for your patience before we get this merged. But please know how much we appreciate it!

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

It works great! Just a couple of tiny nits.

bin/run.sh Outdated Show resolved Hide resolved
tests/example-timeout/expected_results.json Show resolved Hide resolved
@jovv
Copy link

jovv commented Jan 24, 2023

This seems near completion with minimal effort to fix the PR comments. What would be the recommended way to contribute so it can be merged?

@ErikSchierboom
Copy link
Member

I currently have fairly limited time, so it'll take me a while to be able to review this. Sorry.

@ZloeSabo
Copy link
Contributor Author

Hey @jovv @ErikSchierboom! Updated the PR. Sorry, that was a somewhat hectic time for me and I didn't really have time to look at this before.

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Finally had time to review! Sorry about the wait. One small comment.

bin/run.sh Outdated Show resolved Hide resolved
@ErikSchierboom ErikSchierboom merged commit 9e33c74 into exercism:main Feb 7, 2023
@ErikSchierboom
Copy link
Member

Thanks!

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.

Tests are skipped if enough data is written to stdout
4 participants