Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RUBY-1811 ChangeStream spec's Resumable Error definition is too broad #1375

Merged
merged 2 commits into from Jun 11, 2019

Conversation

HanaPearlman
Copy link
Contributor

No description provided.

@@ -162,6 +168,16 @@ def change_stream_resumable_code?
end
private :change_stream_resumable_code?

def change_stream_resumable_label?
Copy link
Contributor

Choose a reason for hiding this comment

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

This method would return true when there is no label, which I think is confusing. Can it be changed to change_stream_not_resumable_label? which would return true if 1) there is a label and 2) it is a change stream not resumable one?

@@ -162,6 +168,16 @@ def change_stream_resumable_code?
end
private :change_stream_resumable_code?

def change_stream_resumable_label?
if labels
!labels.include? CHANGE_STREAM_NOT_RESUME_LABEL
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is the only use of the constant, the literal can be given inline.

spec/mongo/error/operation_failure_spec.rb Show resolved Hide resolved
@@ -165,8 +177,12 @@ def match_result?(result)
end

def match_commands?(actual)
@expectations.each_with_index.all? do |e, i|
actual[i] && match?(e, actual[i])
if @expectations
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer this condition be given in the spec test rather than in the matcher.

Example 1: https://github.com/mongodb/mongo-ruby-driver/blob/master/spec/spec_tests/uri_options_spec.rb#L54

Example 2: https://github.com/mongodb/mongo-ruby-driver/blob/master/spec/support/crud.rb#L153 (the crud tests have a bit more than usual meta programming around them)

@p-mongo
Copy link
Contributor

p-mongo commented Jun 5, 2019

Unfortunately we now have a bunch of other test failures.

Copy link
Contributor

@p-mongo p-mongo left a comment

Choose a reason for hiding this comment

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

👍

@HanaPearlman HanaPearlman merged commit dcb9973 into mongodb:master Jun 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants