[SERVER-39360] assert.commandWorked with writeError sets error code to object Created: 03/Feb/19 Updated: 06/Dec/22 |
|
| Status: | Backlog |
| Project: | Core Server |
| Component/s: | Shell |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | A. Jesse Jiryu Davis | Assignee: | Backlog - Server Tooling and Methods (STM) (Inactive) |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | tig-assertjs | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||
| Assigned Teams: |
Server Tooling & Methods
|
||||||||||||
| Operating System: | ALL | ||||||||||||
| Sprint: | Repl 2019-02-11 | ||||||||||||
| Participants: | |||||||||||||
| Description |
|
Try this:
Expected: number The problem is _getErrorWithCode(), compiled into the shell from utils.js. That function expects the provided error object to be a command failure with ok: 0, or an object with a "writeError" field, not a "writeErrors" array. I think _getErrorWithCode() should check for writeErrors, too. I'm not sure "writeError" is ever actually present. This is a problem for tests that check things like "ex.code === 123". If the command failed with a writeErrors array, the exception's code is an object, not the expected number, and the check may be false incorrectly. |
| Comments |
| Comment by Steven Vannelli [ 10/May/22 ] | |||||||||||||||||||
|
Moving this ticket to the Backlog and removing the "Backlog" fixVersion as per our latest policy for using fixVersions. | |||||||||||||||||||
| Comment by Jack Mulrow [ 25/Apr/19 ] | |||||||||||||||||||
|
Just a heads up, as part of
I didn't make any changes to the places that don't pass "res" to doassert(), so that work can still be tracked by this ticket. | |||||||||||||||||||
| Comment by Max Hirschhorn [ 25/Feb/19 ] | |||||||||||||||||||
|
I think trying to tie writeErrors from the server response and the code property in the resulting Error object is going to be challenging due to how using the === and !== operators are fairly entrenched in how we write JavaScript tests and we'd need to make those into function calls to be able allow comparing against more than one code. I think the action to take on this ticket to change the else-branch in the _getErrorWithCode() function to be based around whether the codeOrObj argument is typeof === 'number'. | |||||||||||||||||||
| Comment by A. Jesse Jiryu Davis [ 05/Feb/19 ] | |||||||||||||||||||
|
Max noticed a couple places where `res` wasn't being given as an argument to doassert() inside the _assertCommandFailed() and assert.writeErrorWithCode() functions. https://github.com/mongodb/mongo/blob/r4.1.7/src/mongo/shell/assert.js#L655 | |||||||||||||||||||
| Comment by A. Jesse Jiryu Davis [ 03/Feb/19 ] | |||||||||||||||||||
|
Agreed. The WriteResult has writeErrors but no writeError property. So it seems like the intent of this code had been to set "e.code" to one of the codes in the writeErrors array. (If there are multiple, I don't know which it should be.) That would be useful in passthrough tests with logic like this (fsm.js):
| |||||||||||||||||||
| Comment by Max Hirschhorn [ 03/Feb/19 ] | |||||||||||||||||||
I didn't find anything either. My best guess is that there was a misunderstanding of how to access properties of a WriteResult instance when it represents a write error.
| |||||||||||||||||||
| Comment by A. Jesse Jiryu Davis [ 03/Feb/19 ] | |||||||||||||||||||
|
Thanks Max. Can you point to what object includes a "writeError" field? I can't find anything called "writeError", only the "writeErrors" array. | |||||||||||||||||||
| Comment by Max Hirschhorn [ 03/Feb/19 ] | |||||||||||||||||||
The _getErrorWithCode() function set the "code" property to the entire response object because it is assumes the res argument to be a number. The _getErrorWithCode() function hasn't really been changed much since
I would argue that the _getErrorWithCode() function should have left the "code" property as undefined in this case. If you want to assert on a specific error code being returned, why not using the assert.commandFailedWithCode() function?
|