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

feat(host,provers,tasks): handle local job cancellation #345

Merged
merged 4 commits into from
Aug 16, 2024

Conversation

petarvujovic98
Copy link
Contributor

No description provided.

Copy link
Contributor

@smtmfft smtmfft left a comment

Choose a reason for hiding this comment

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

Can you make some tests for the request & cancel process? I think we need to cover more as the v2 will be online soon. to make our future life easier, we need to do it. however, I don't know if there is a good way to test server API, maybe not easy to write simple unit test for it??
anyway, I came up with few cases, and I believe there are more than these, let's try to cover them as more as possible.

  1. request->mock crash->request(should return the right error status)
  2. request->cancel->request(returns cancel by user or restarts the task??)
  3. request repeatly or cancel repeatly

@petarvujovic98
Copy link
Contributor Author

petarvujovic98 commented Aug 15, 2024

@smtmfft I agree, we need to have testing for this. This, however is not a simple unit test, rather a end-to-end test where we go through a whole request flow with all the components spun up.

I've already been playing around with this, but I need to add it to CI and polish it further to be good.
I'll do it in a separate PR which will be addressing testing (so it will be a bit bigger).

BTW

  1. request->cancel->request(returns cancel by user or restarts the task??)

This restarts the task, but we should have a test enforcing this behavior

@smtmfft
Copy link
Contributor

smtmfft commented Aug 15, 2024

@smtmfft I agree, we need to have testing for this. This, however is not a simple unit test, rather a end-to-end test where we go through a whole request flow with all the components spun up.

great! I think you can take current ontake upgrade to improve it.

@petarvujovic98 petarvujovic98 added this pull request to the merge queue Aug 16, 2024
Merged via the queue into main with commit cce1371 Aug 16, 2024
14 checks passed
@petarvujovic98 petarvujovic98 deleted the risc0-local-cancel branch August 16, 2024 08:48
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