Skip to content

Conversation

jyemin
Copy link
Collaborator

@jyemin jyemin commented Apr 8, 2022

I'm doing all these as one PR because the unified change stream tests are what required the first two changes, but it was difficult to split up do to the overlapping commits to the unified tests. So submitting this as together but will not squash the commits.

JAVA-4565: this one was necessary because one of the new tests ends up throwing a MongoQueryException instead of a MongoCommandException, and MongoQueryException didn't have the errorCodeName property needed to make assertion.

JAVA-4470: this one is not fully done. It requires a bunch of things if we want to use ChangeStreamDocument for all the unified tests. The one that's now handled properly is an operationType for which there is no associated enum value. But for a couple of the new tests we're escaping to BsonDocument for the entire change stream document so that the assertions pass.

JAVA-4555: ChangeStreamDocument already handles the "to" field, so nothing needed to be done there. So it's just updates to the unified tests.

@jyemin jyemin requested review from stIncMale and rozza April 8, 2022 20:32
@jyemin jyemin changed the title Various change required to update unified change stream tests Various changes required to update unified change stream tests Apr 8, 2022
Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

jyemin added 3 commits April 12, 2022 12:10
Also, ensure that MongoQueryException pushes the property down through the constructor

JAVA-4565
* Allows applications using an older driver to access a newer operation type, for which
  there is no enumerated value in OperationType
* Allows operationType to be round-tripped with no information loss

JAVA-4470
* Represent all cursors the same in Entities
* Handle any MongoServerException in ErrorMatcher
* Support "rename" operation
* Special case some change stream tests that can't pass using ChangeStreamDocument

JAVA-4470
JAVA-4555
@jyemin
Copy link
Collaborator Author

jyemin commented Apr 12, 2022

Now that PRs are executing a larger set of variants, I'm seeing many of them fail.

I will investigate and update the PR when I understand the root cause(s).

Copy link
Member

@stIncMale stIncMale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite the force pushes, GitHub showed me that the only file I need to review / re-review is SyncMongoCollection.java, and that's what I did.

@jyemin
Copy link
Collaborator Author

jyemin commented Apr 13, 2022

Tests are still failing, so this can't be merged yet.

@jyemin
Copy link
Collaborator Author

jyemin commented Apr 15, 2022

Tests are finally green, due to two changes:

  1. enableSleepAfterCursorOpen is called for all change stream tests. This fixes an issue in many of the tests that are relying on the change stream being established before executing a crud operation, which is then expected to generate a change stream event. But if the crud operation ends up executing before the change stream is established, then it will not generate an event. By sleeping for a bit, we decrease the chances (hopefully close to zero) that the change stream is established first
  2. Skip one test that is failing on latest sharded clusters, due to SERVER-65497. This can be reverted once the fix for the sever issue makes it into the latest build.

@jyemin jyemin requested a review from stIncMale April 15, 2022 01:47
@jyemin
Copy link
Collaborator Author

jyemin commented Apr 18, 2022

Merged on command line.

@jyemin jyemin closed this Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants