-
Notifications
You must be signed in to change notification settings - Fork 356
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
fix: job queue panic for multirm [RM-123] #9079
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9079 +/- ##
==========================================
+ Coverage 47.07% 47.09% +0.02%
==========================================
Files 1155 1156 +1
Lines 142400 142580 +180
Branches 2421 2421
==========================================
+ Hits 67034 67153 +119
- Misses 75176 75237 +61
Partials 190 190
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.
one blocking comment
44bf31d
to
e1c0d9b
Compare
Description
Fix index bug in GetJobQueueStatsRequest by executing the proper RM call in a fanout call.
Also, add
os.Run()
to the multirm test file -- so now the tests run! Change the mock RM return values from mock.Anything to the relevant complex type.Test Plan
Without the changes to the GetJobQueueStatsRequest, its test should fail the
empty_request
case as follows:With these changes, all should pass!
Commentary (optional)
Checklist
docs/release-notes/
.See Release Note for details.
Ticket
RM-123