[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: PNG File Screenshot 2023-12-01 at 11.38.44 AM.png    
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 ]

insert() and insertMany() are the worst offenders, which occur in most tests and are always expected to work.

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 (SERVER-17953) throw JavaScript errors on write errors and write concern errors. The existing DBCollection methods had been left as-is to avoid changing the semantics for any scripts (both within the mongodb/mongo codebase and externally). Now that the mongo shell is no longer a released product, we have a lot more flexibility around changing the behavior of the existing DBCollection, etc. methods.

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.

Generated at Thu Feb 08 06:53:09 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.