-
Notifications
You must be signed in to change notification settings - Fork 2
test and solve cases of find with multiple ids #19
Conversation
base1 = BaseTest.create!(name: 'joe1') | ||
base2 = BaseTest.create!(name: 'joe2') | ||
base3 = BaseTest.create!(name: 'joe3') | ||
expect(BaseTest.find([base1.id, 't', 't', base2.id, base3.id], quiet: true)).to eq([base1, base2, base3]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make sure that the ids returned by the find are not replaced? Since we add the ids here: https://github.com/doctolib/couchbase-orm/pull/19/files#diff-7cab7700be71ff406f6ca75d7f60cbec51ab79649fab7d39df67893225d7247aR257
Or is it already covered by the eq([base1, base2, base3])
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for me eq covered it but i'm not an expert of ruby
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try running it without your changes? If it still passes, I would say we are missing an assertion somewhere, if it fails we should be good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new test without the functional change is breaking some new test. So it's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Pierre Merlin <pimpin@users.noreply.github.com>
Now when we find with multiple ids and if some of them doesn't have document in couchbase the result is not coherent.