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

Flapping test failure #126

Closed
brendon opened this issue Jul 11, 2018 · 6 comments
Closed

Flapping test failure #126

brendon opened this issue Jul 11, 2018 · 6 comments

Comments

@brendon
Copy link
Owner

brendon commented Jul 11, 2018

Here's the test:

    describe "with max value and with_same pond" do

      before {
        Duck.first(50).each_with_index do |d, index|
          d.update_attributes :age => index % 10, :pond => "Pond #{index / 10}"
        end
        @duck_11 = Duck.offset(10).first
        @duck_12 = Duck.offset(11).first
        @ordered = Duck.where(:pond => 'Pond 1').rank(:age).where(Duck.arel_table[:id].not_in([@duck_11.id, @duck_12.id])).collect {|d| d.id }
        @duck_11.update_attribute :age, RankedModel::MAX_RANK_VALUE
        @duck_12.update_attribute :age, RankedModel::MAX_RANK_VALUE
      }

      context {
        subject { Duck.where(:pond => 'Pond 1').rank(:age).collect {|d| d.id } }

        it { should == (@ordered[0..-2] + [@ordered[-1], @duck_11.id, @duck_12.id]) }
      }

      context {
        subject { Duck.first.age }
        it { should == 0}
      }
      
    end

I've tried to reason this one out. It looks like you create two ponds of ducks (0 and 1). 10% of the ducks will be in Pond 1. The age of the ducks in Pond 1 will always equal 0 for the initial update_attributes.

You then grab duck 11 and 12 which could conceivably be in either Pond 1 or 0 depending on how the rankings were reshuffled. You then get a result that excludes these two ducks, and add the two ducks to the end of the list.

We then compare Pond 1 members with the original members that have had duck 11 and 12 added.

  • Why are you doing @ordered[0..-2] + [@ordered[-1]. Isn't that the same as: @ordered[0..-1]?
  • The tests sometimes fail I believe because sometimes duck 11 and/or 12 are sometimes in Pond 0 and so aren't ever selected.

This could probably be most easily fixed by just making sure duck 11 and 12 are from Pond 1. I'll have a play.

@brendon
Copy link
Owner Author

brendon commented Jul 11, 2018

Actually here are the ducks:

Age: 0   Pond: 0
Age: 1   Pond: 0
Age: 2   Pond: 0
Age: 3   Pond: 0
Age: 4   Pond: 0
Age: 5   Pond: 0
Age: 6   Pond: 0
Age: 7   Pond: 0
Age: 8   Pond: 0
Age: 9   Pond: 0
Age: 0   Pond: 1
Age: 1   Pond: 1
Age: 2   Pond: 1
Age: 3   Pond: 1
Age: 4   Pond: 1
Age: 5   Pond: 1
Age: 6   Pond: 1
Age: 7   Pond: 1
Age: 8   Pond: 1
Age: 9   Pond: 1
Age: 0   Pond: 2
Age: 1   Pond: 2
Age: 2   Pond: 2
Age: 3   Pond: 2
Age: 4   Pond: 2
Age: 5   Pond: 2
Age: 6   Pond: 2
Age: 7   Pond: 2
Age: 8   Pond: 2
Age: 9   Pond: 2
Age: 0   Pond: 3
Age: 1   Pond: 3
Age: 2   Pond: 3
Age: 3   Pond: 3
Age: 4   Pond: 3
Age: 5   Pond: 3
Age: 6   Pond: 3
Age: 7   Pond: 3
Age: 8   Pond: 3
Age: 9   Pond: 3
Age: 0   Pond: 4
Age: 1   Pond: 4
Age: 2   Pond: 4
Age: 3   Pond: 4
Age: 4   Pond: 4
Age: 5   Pond: 4
Age: 6   Pond: 4
Age: 7   Pond: 4
Age: 8   Pond: 4
Age: 9   Pond: 4

So duck 11 and 12 should be in Pond 1. The test should work all the time, but for some reason sometimes there are 2 extra ducks in the expected set.

@brendon
Copy link
Owner Author

brendon commented Jul 12, 2018

Ok, solved this one:

Simply calling:

@duck_11 = Duck.offset(10).first
@duck_12 = Duck.offset(11).first

won't retrieve ducks in a determinate way on postgresql. We need to specify an order (and a pond). I've added the fix in my current PR to keep things together. I hope that's ok.

@brendon brendon closed this as completed Jul 12, 2018
@mixonic
Copy link
Contributor

mixonic commented Jul 12, 2018

@brendon wow great hunting. Why does the query not deterministically return the appropriate ducks? The code doesn't look to have any non-deterministic characteristics?

@brendon
Copy link
Owner Author

brendon commented Jul 12, 2018

I think it's because we're grabbing the first 50 ducks and updating them, which may move them around on disk. The Postgresql docs state:

After a query has produced an output table (after the select list has been processed) it can optionally be sorted. If sorting is not chosen, the rows will be returned in an unspecified order. The actual order in that case will depend on the scan and join plan types and the order on disk, but it must not be relied on. A particular output ordering can only be guaranteed if the sort step is explicitly chosen.

Most databases return what we'd expect, but I guess Postgresql uses the non-determinism to its advantage in the way it stores things? Hard to say for sure :)

@mixonic
Copy link
Contributor

mixonic commented Jul 12, 2018

Wow. So even a sort by id (presuming sequential ids) would have been a fix. Thanks for digging this up!

@brendon
Copy link
Owner Author

brendon commented Jul 12, 2018

Haha! Yea I didn't think of that. Either one probably fits in with the spirit of the test :)

Just quickly before we walk away from this one: How come you're doing this?

it { should == (@ordered[0..-2] + [@ordered[-1], @duck_11.id, @duck_12.id]) }

vs

it { should == (@ordered[0..-1] + [@duck_11.id, @duck_12.id]) }

or

it { should == (@ordered.to_a + [@duck_11.id, @duck_12.id]) }

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

No branches or pull requests

2 participants