SPEC-1221 Reinstate DuplicateKey code assertions in error labels tests

[p-mongo]

No description provided.

[jmikola]

Is there an issue number for this PR?

[p-mongo]

@jmikola This goes with the discussion we were having in https://jira.mongodb.org/browse/SERVER-39706 with James.

[ShaneHarvey]

#482 was the previous change that removed the DuplicateKey code name assertions. If we're going to add them back in the "E11000" form can you add them to all the other tests too?

[p-mongo]

I can do that if there is consensus that the change in this PR is wanted.

[jmikola]

I don't have objections if errorContains: "E11000" is truly a portable solution. I suppose it still requires drivers to populate the top-level exception/error message with the string from the write error, which may not be dictated by any spec (although it's likely all drivers are consistent here).

I appreciate the desire to be strict here. Although the operations make it clear that we're causing a duplicate key exception, we can't really be sure without inspecting the error.

[p-mongo]

Updated all test files as requested by @ShaneHarvey .

[jmikola]

LGTM. Over to @ShaneHarvey.

[ShaneHarvey]

LGTM, tested in python: https://evergreen.mongodb.com/version/5d7fc2710305b931eb9a230c

Note that I had to make a small test code change to the "assertErrorContains" logic to look inside our BulkWriteException class otherwise the test would fail with AssertionError: 'e11000' not found in 'batch op errors occurred'.

[p-mongo]

That is expected since the error was moved from top level to the bulk write errors.

[p-mongo]

https://github.com/mongodb/specifications/tree/master/source/crud/tests mentions BulkWriteException but not errorContains, should I add a description for errorContains there as part of this PR?

[ShaneHarvey]

I don't think we need to add errorContains to the crud spec. It's described in https://github.com/mongodb/specifications/blob/master/source/transactions/tests/README.rst

[jmikola]

I don't think we need to add errorContains to the crud spec. It's described in https://github.com/mongodb/specifications/blob/master/source/transactions/tests/README.rst

I think we should err on the side of having each spec document the test fields it actually uses, until we actually have a unified format in place.

That said, this PR didn't actually modify any CRUD test files so I'm a bit confused by @p-mongo's question of adding a note in this comment.

@ShaneHarvey, was this comment referring to you needing to add the BulkWriteException inspection to your transactions spec test runner? If so, perhaps we should add a note about that to the transactions spec test README.

[ShaneHarvey]

was this comment referring to you needing to add the BulkWriteException inspection to your transactions spec test runner? If so, perhaps we should add a note about that to the transactions spec test README.

Yes it was. I just remembered that BulkWriteException is part of the crud spec. Maybe we can add somethign like this to the errorContains doc:

If the result document has an "errorContains" field, verify that the method threw an exception or returned an error, and that the value of the "errorContains" field matches the error string. "errorContains" is a substring (case-insensitive) of the actual error message. If the actual error is a BulkWriteException then the error string should be checked against the writeConcernError and writeErrors fields.

[jmikola]

The transaction test format docs already say that "errorContains" is " A substring of the expected error message.", so I think we could get by with amending the docs for the result element itself. Also consider that errorCodeName is another field that could conceivably apply to an error within a BulkWriteException (rather than the error itself).

That said, I believe some drivers create the BulkWriteException by concatenating the errors within it, in which case some drivers may not even care about this. It's still worth noting in the README, though.

result currently states:

The return value from the operation, if any. This field may be a single document or an array of documents in the case of a multi-document read. If the operation is expected to return an error, the result is a single document that has one or more of the following fields:

I'd propose the following note section after the test format syntax:

Error Assertions for Bulk Write Operations
``... (more backticks, omitted here due to Markdown issues)

When asserting errors (e.g. `errorContains`, `errorCodeName`) for bulk write
operations, the test harness should inspect the `writeConcernError` and/or
`writeErrors` properties of the bulk write exception. This may not be needed for
`errorContains` if a driver concatenates all write and write concern error
messages into the bulk write exception's top-level message.

I didn't mention anything about error labels, but I don't think those would apply to writeConcernError or writeErrors since labels are meant for the top-level exception.

[p-mongo]

Added the suggested note.

[ShaneHarvey]

Still LGTM.