Skip to content

Conversation

jyemin
Copy link
Collaborator

@jyemin jyemin commented Mar 11, 2024

@jyemin jyemin self-assigned this Mar 11, 2024
MongoDatabase database = entities.getDatabase(operation.getString("object").getValue());

BsonDocument arguments = operation.getDocument("arguments");
BsonDocument arguments = operation.getDocument("arguments", new BsonDocument());
Copy link
Collaborator Author

@jyemin jyemin Mar 11, 2024

Choose a reason for hiding this comment

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

This change was actually necessary to get the tests to pass. I decided to make the same change everywhere we get arguments, because this keeps happening every time we convert more tests.

BsonDocument arguments = operation.getDocument("arguments");
BsonDocument arguments = operation.getDocument("arguments", new BsonDocument());
ClientSession session = getSession(arguments);
BsonDocument filter = arguments.getDocument("filter", new BsonDocument());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This matches all the other places where filter is required. I had to make this change after changing the line two lines up, as the exception thrown on that line when there are no arguments was incorrectly causing the createFindCursor fails if filter is not specified in unified-test-format/valid-fail to pass.

@jyemin jyemin requested a review from stIncMale March 13, 2024 20:19
will be removed soon, and this will just create a merge conflict.
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.

The spec says "Test runners MUST NOT execute killAllSessions when connected to Atlas Data Lake.", yet com.mongodb.client.unified.UnifiedTest.setUp (a superclass of UnifiedAtlasDataLakeTest) calls CollectionHelper.killAllSessions unconditionally. It seems to me that we should call killAllSessions only if ClusterFixture.isDataLakeTest returns true.

@jyemin
Copy link
Collaborator Author

jyemin commented Mar 19, 2024

It seems to me that we should call killAllSessions only if ClusterFixture.isDataLakeTest returns true.

I'm inclined to leave it as is. AbstractUnifiedTest, which the now-deleted legacy test extends, executes killAllSessions without such a check, and those tests consistently passed. The new unified tests also pass.

@jmikola I looked at your PR for this change in the specs repo (that Valentin linked to above), and I'm not clear where this restriction on killAllSessions for Data Lake originated. I am likely overlooking something, but it does seem unnecessary.

@jyemin
Copy link
Collaborator Author

jyemin commented Mar 19, 2024

I confirmed that killAllSessions is not supported on Data Lake, and it only works in the Java driver because we catch exceptions in the helper for killAllSessions. So I will add the check in as Valentin requested.]

So never mind @jmikola

@jyemin jyemin requested a review from stIncMale March 19, 2024 16:36
@jyemin jyemin merged commit ddd5c5b into mongodb:master Mar 19, 2024
@jyemin jyemin deleted the j5354 branch March 19, 2024 19:15
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.

2 participants