RUBY-558 batch size on initial query, RUBY-505 test intervening refresh #163

Closed
wants to merge 4 commits into
from

Projects

None yet

5 participants

@estolfo
mongodb member

Change to cursor.rb and tests for applying specified batch size to initial query. Get mores have always applied batch size. RUBY-558

Test on cursors iterating with an intervening query that causes a connection refresh in a replica set. RUBY-505 and RUBY-545.

@estolfo
mongodb member

Yes, it is confirmed that batch size should be applied to the first batch.

@TylerBrock

What does everyone think about instance_variable_get in a test? I think we should make @pool accesible via an attr_reader.

@gjmurakami-10gen
@estolfo
mongodb member

I don't see a problem with using instance_variable_get in a test because it's used to inspect functionality and verify state. It doesn't seem that using it frequently in a test should determine whether a particular variable should be available publicly as providing public access is motivated by an entirely different need.

@estolfo
mongodb member

This intervening query test is going to only pass when we're lucky and get the same secondary after doing a hard_refresh. I'm going to make a comment to note this but the test can serve as a useful reference point when working on a new connection pool design.

@TylerBrock

@estolfo we don't want tests that are more brittle than what we already have. Part of that is not testing private interfaces (I also think pool should be part of the public interface).

Regarding hard_refresh... why not just refresh? If for some reason a hard_refresh! is required I wouldn't expect this to work, the member you were querying may have disappeared from the set entirely!

@estolfo
mongodb member

I used hard_refresh in the test because refresh wouldn't actually do anything as it depends on @refresh_required being true. @refresh_required wouldn't be true in the test. Do you think it would be better to set @refresh_required to true and then do just refresh?
How else do you suggest we check internals if not using instance_variable_get? I think this is very appropriate for a test, as we are supposed to be checking internals.
I'm fine with making pool part of the public interface, but that can be part of a pool_manager overhaul and I don't think it should be done just so that it's available for a test.

@gjmurakami-10gen
@TylerBrock

Its fine, we just added it a day ago to the driver, it didn't exist in 1.8.2 so we wouldn't change expectations for anyone and I think there may be external benefit to determining what pool a socket came from. That being said, I'm ok with not making that public if everyone dislikes that, keep it as is if you would like. Gary is correct in saying this is acceptable even if in this particular case I disagree.

However, hard_refresh! here is definitely incorrect and you should use a mock to make refresh_required true to test regular refresh.

There is only one scenario under which I would say committing an intermittently failing test is acceptable and that is with a fix for it. In this case, the test is incorrect I don't expect that to pass ever after a hard_refresh! and tracking this down at a later time will become a nightmare.

@brandonblack

I don't think it makes sense to make an attr_reader for @pool just for the test. However, if adding that to the public interface makes sense (I'm unclear on what purpose that would serve, not sure that it does make sense) then yes these tests should be modified.

As it stands right now, I'd say its fine to use instance_variable_get here.

@ajdavis
mongodb member
@TylerBrock

Ok. The test is still testing the wrong thing though...

I spoke with emily about hard_refresh! being irrelevant and untestable, she's going to stub refresh_required so that it always returns true and can use refresh.

@brandonblack

Looks good Emily. Ship it.

@estolfo estolfo closed this Mar 1, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment