-
Notifications
You must be signed in to change notification settings - Fork 100
PERL-806 Check compatibility for SDAM tests #173
Conversation
t/sdam_spec.t
Outdated
|
|
||
| while ( my $path = $iterator->() ) { | ||
| next unless -f $path && $path =~ /\.json$/; | ||
| #next unless $path =~ /too_old/; |
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.
Stray comment. :-)
But in the case of the /too_old/ test, we should just fiddle the compatible result to 'true' somewhere later in this test runner so we can make sure it actually works.
| is($topology->replica_set_name, $expected_set_name, 'correct setName for topology'); | ||
| is($topology->type, $outcome->{'topologyType'}, 'correct topology type'); | ||
| is($topology->logical_session_timeout_minutes, $outcome->{'logicalSessionTimeoutMinutes'}, 'correct ls timeout'); | ||
| if ( defined $outcome->{'compatible'} ) { |
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.
Here, if it's the /too_old/ test file then we want to force the compatibility expectation to true. (Along with a comment that Perl has to support older servers.)
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.
Right, have done so :)
lib/MongoDB/_Topology.pm
Outdated
|
|
||
| $server_max_wire_version = 0 unless defined $server_max_wire_version; | ||
| $server_min_wire_version = 0 unless defined $server_min_wire_version; | ||
| # set to -1 as could be undefined, or 0. |
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.
This should stay "0" because missing is equivalent to wire version "0" and we don't need it to force the compatibility logic to fail, as the Perl driver is special and has to deviate from the spec and support older versions (because of some large customers that asked for legacy support).
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.
Right ok, have modified it back to the original version but put a comment specifically about that so it doesnt get done again heh
xdg
left a comment
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
|
Rebased and merged as 588bcaa |
Contains the fixes for SDAM spec tests as well. Issues where:
is_master.mevalue to addresstoo_old.ymltests)One thing that isnt in this one, is there is a
monitoringfolder for the SDAM spec tests repo - are these implemented elsewhere or do these need doing?