-
Notifications
You must be signed in to change notification settings - Fork 178
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
refactor(app,api): Add an "awaiting-recovery" run status #14651
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #14651 +/- ##
==========================================
- Coverage 67.34% 67.34% -0.01%
==========================================
Files 2485 2485
Lines 71439 71438 -1
Branches 9057 9056 -1
==========================================
- Hits 48114 48113 -1
Misses 21173 21173
Partials 2152 2152
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you got all the relevant spots in the app! This LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python code looks good to me and so does the js changes although I think someone a little more in tune with how the app works probably should take a look there. We should pretty rapidly follow this up with re-adding the ability to cancel from this state at least.
Overview
Closes EXEC-299.
Changelog
awaiting-recovery
run status. It's not actually used by Protocol Engine or emitted by robot-server yet. See the Python docstring for the semantics that we have in mind so far.Test plan
There's nothing substantial to test here yet because the new status isn't used by Protocol Engine or emitted by robot-server yet.
I've deliberately omitted unit tests because I think, at this stage, we want to keep it cheap to fundamentally rework the semantics of this status. But I'm happy to go back and add them if anyone thinks they'd be helpful.
Review requests
Any questions or suggestions about the semantics of this new status?
Risk assessment
Low until this new status actually gets emitted.