PYTHON-2683 Convert change stream spec tests to unified test format #950
Conversation
|
Docs failure is unrelated, https://wiki.centos.org/AdditionalResources/Repositories/SCL was also erroring out temporarily last week. |
|
RedHat wiki is back up, I kicked the link check job. |
test/unified_format.py
Outdated
| raise | ||
| else: | ||
| if expect_error: | ||
| if expect_error and opname != "iterateUntilDocumentOrError": |
There was a problem hiding this comment.
What is the iterateUntilDocumentOrError check for?
There was a problem hiding this comment.
For the case where this an "expectError", but the error condition isn't raised, because we got a document instead, which as I understand it is what iterateUntilDocumentOrError implies.
There was a problem hiding this comment.
It looks like iterateUntilDocumentOrError is usually (always?) accompanied by "expectError" or "expectResult". When expectError is there but the iterateUntilDocumentOrError op does not error, shouldn't we still fail the test?
There was a problem hiding this comment.
I removed the modification based on feedback about how "expectError" should be handled in the spec: "If operation.expectError is specified, the test runner MUST assert that the operation yielded an error; otherwise, the test runner MUST assert that the operation did not yield an error.
I suspect this will make "test_change_stream.TestUnifiedChangeStreamsErrors.test_change_stream_errors_on_ElectionInProgress" fail again on Server 4.2, let's find out.
There was a problem hiding this comment.
Sure enough:
AssertionError: Excepted error {'errorCode': 216} but "iterateUntilDocumentOrError" succeeded:
{'_id':
{'_data': '82628D50B2000000042B022C0100296E5A10044BBFDAAD009A4B0A8CF9E843E82603C946645F69640064628D50B26500615C7FE84A840004'},
'operationType': 'insert', 'clusterTime': Timestamp(1653428402, 4), 'fullDocument':
{'_id': ObjectId('628d50b26500615c7fe84a84'), 'z': 3}, 'ns': {'db': 'database0', 'coll': 'collection0'},
'documentKey': {'_id': ObjectId('628d50b26500615c7fe84a84')}}There was a problem hiding this comment.
This looks like the last issue to resolve. Are you saying this is a bug in the spec test itself? How about we add back the and opname != "iterateUntilDocumentOrError": behavior until the test is fixed so that we can merge this PR?
There was a problem hiding this comment.
I don't think it is a spec test failure, given that several other drivers have implemented the unified spec change.
There was a problem hiding this comment.
I'll do some testing with server 4.2 to see if I can spot a bug in our driver.
|
Moving to draft to make sure we handle PYTHON-3273 and PYTHON-3237. |
test/unified_format.py
Outdated
| raise | ||
| else: | ||
| if expect_error: | ||
| if expect_error and opname != "iterateUntilDocumentOrError": |
There was a problem hiding this comment.
It looks like iterateUntilDocumentOrError is usually (always?) accompanied by "expectError" or "expectResult". When expectError is there but the iterateUntilDocumentOrError op does not error, shouldn't we still fail the test?
| continue | ||
|
|
||
| self.assertGreaterEqual(len(actual_events), len(events), actual_events) | ||
| self.assertEqual(len(actual_events), len(events), actual_events) |
There was a problem hiding this comment.
Oh I see. So we always had the ignoreExtraEvents=True behavior before this.
|
Opend https://jira.mongodb.org/browse/PYTHON-3292 to track the removal of the workaround for the failing test. |
ShaneHarvey
left a comment
ShaneHarvey
left a comment
There was a problem hiding this comment.
LGTM once the tests pass.
2 participants
No description provided.