Skip to content

Conversation

@kaywux
Copy link
Contributor

@kaywux kaywux commented Oct 11, 2019

The count() function for MongoCollection has been deprecated and replaced with countDocuments() and estimatedDocumentCount(). The new API has been added, along with changes to CRUD and other tests.

JIRA ticket: https://jira.mongodb.org/browse/SWIFT-133

Evergreen: https://evergreen.mongodb.com/version/5da0c38d32f41762b94647cc

@kaywux kaywux force-pushed the SWIFT-133/count_api branch from 465c807 to 9bd3087 Compare October 11, 2019 18:13
@kaywux kaywux requested review from kmahar and patrickfreed October 11, 2019 18:22
let testDocs: [Document]
var tests: [CrudTest] { return try! testDocs.map { try makeCrudTest($0) } }
var tests: [CrudTest] { return try!
testDocs.filter { ($0["operation"] as? Document)?["name"] as? String != "count" }.map { try makeCrudTest($0) } }
Copy link
Contributor

Choose a reason for hiding this comment

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

this can probably be left as-is, and instead we just skip the test above in doTests. That way all the test skipping is centralized.

Copy link
Contributor

Choose a reason for hiding this comment

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

doing that would require that we keep around the CountTest type as a placeholder to parse into. the files mix count and countDocuments so we can't skip a whole file, either.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, good point. I actually personally prefer the placeholder approach. Imo we should be able to deserialize all the tests, regardless of which ones we're going to run, and the testing logic should decide which ones get skipped. It feels a little weird here to be inspecting the documents directly right before we deserialize them.

This isn't a huge deal of course, but the placeholder approach is the one I'm taking for the retryable reads tests, so I figured it was worth mentioning here.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's fine with me, switched to using a placeholder.

@kmahar kmahar force-pushed the SWIFT-133/count_api branch from 069f5b7 to 91ac9fe Compare October 19, 2019 19:02
@kmahar kmahar requested review from patrickfreed and removed request for kmahar October 19, 2019 19:03
@kmahar kmahar force-pushed the SWIFT-133/count_api branch from f1b493b to 764a529 Compare October 23, 2019 05:10
@kmahar kmahar merged commit bc95b1c into master Oct 23, 2019
@kmahar kmahar deleted the SWIFT-133/count_api branch October 23, 2019 15:49
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.

4 participants