[SERVER-83787] Make shell helpers throw by default - removing need for wrapping in assert.commandWorked Created: 01/Dec/23 Updated: 04/Jan/24 |
|
| Status: | Backlog |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Task | Priority: | Major - P3 |
| Reporter: | Memento Slack Bot | Assignee: | Backlog - Query Integration |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | make-server-testing-joyful, quick-tech-debt | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Attachments: |
|
| Assigned Teams: |
Query Integration
|
| Participants: |
| Description |
|
insert() and insertMany() are the worst offenders, which occur in most tests and are always expected to work. I think helpers like 'createIndex' are also great candidates. We may want to split this into multiple commits or even multiple tickets. |
| Comments |
| Comment by Gregory Noma [ 14/Dec/23 ] |
|
If we do this in conjunction with SERVER-78457 (and/or SERVER-28567) then hopefully we won't have to worry about the `assert.throws` |
| Comment by Max Hirschhorn [ 14/Dec/23 ] |
FWIW, insertMany() does automatically throw a JavaScript error when a write error or write concern error occurs.
In particular, all DBCollection methods which were introduced as part of the CRUD API ( Flipping the default so JavaScript errors are thrown by default on error seems ok to me. However I would be sad if the result increased the number of usages of assert.throws() in testing. assert.throws() passes when any JavaScript exception occurs and means it can be somewhat easy to pass for the wrong reason (SERVER-28567). |
| Comment by Gregory Noma [ 01/Dec/23 ] |
|
I think it definitely make sense to make the behavior consistent among shell helpers. Currently some always return the command response whereas other throw upon a non-ok response. |