Skip to content

Conversation

@bazile-clyde
Copy link
Contributor

No description provided.

[](document::view doc, bsoncxx::array::element ele) {
REQUIRE_BSON_MATCHES(doc, ele.get_document().value);
return true;
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

During my debugging efforts, I had a difficult time trying to parse what was happening here. I realized all this did was assert that two lists contained the same elements and had the same length. REQUIRE_BSON_MATCHES will show mismatches in the debugging output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this in the last commit. Since this checks from begin(range1) to end(range1) and iterates range2 this would've returned true if range1 was empty. I swapped the ranges so we check at least the length of expected_data. Afterward actual should be empty since it's a cursor if they're the same length.

Copy link
Contributor Author

@bazile-clyde bazile-clyde Jul 17, 2020

Choose a reason for hiding this comment

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

Also, I removed the write_concern swap logic and ran it on evergreen (here). All of the tests pass so I'm not sure if we still need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, nevermind. After investigating CDRIVER-2901 it seems better to leave it in.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks a lot cleaner. And I agree, using local read concern seems necessary.

@bazile-clyde bazile-clyde requested a review from kevinAlbs July 17, 2020 01:04
@codecov-commenter
Copy link

codecov-commenter commented Jul 17, 2020

Codecov Report

Merging #694 into master will increase coverage by 0.14%.
The diff coverage is 92.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #694      +/-   ##
==========================================
+ Coverage   83.61%   83.75%   +0.14%     
==========================================
  Files         346      346              
  Lines       16009    16107      +98     
==========================================
+ Hits        13386    13491     +105     
+ Misses       2623     2616       -7     
Impacted Files Coverage Δ
src/mongocxx/collection.hpp 100.00% <ø> (ø)
src/mongocxx/model/delete_many.hpp 100.00% <ø> (ø)
src/mongocxx/model/delete_one.hpp 100.00% <ø> (ø)
src/mongocxx/model/replace_one.hpp 100.00% <ø> (ø)
src/mongocxx/model/update_many.hpp 100.00% <ø> (ø)
src/mongocxx/model/update_one.hpp 100.00% <ø> (ø)
src/mongocxx/options/delete.hpp 100.00% <ø> (ø)
src/mongocxx/options/find_one_and_delete.hpp 100.00% <ø> (ø)
src/mongocxx/options/replace.hpp 100.00% <ø> (ø)
src/mongocxx/options/update.hpp 100.00% <ø> (ø)
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b49ec57...4b12fad. Read the comment docs.

@bazile-clyde bazile-clyde requested a review from kevinAlbs July 17, 2020 20:47
Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Changes LGTM

@bazile-clyde bazile-clyde merged commit 1a7f2ee into mongodb:master Jul 17, 2020
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