eiri commented on a change in pull request #560: Fix broken eunit test in
changes_since_test_ test suite
URL: https://github.com/apache/couchdb/pull/560#discussion_r120648939
##########
File path: src/couch_mrview/src/couch_mrview.erl
##########
@@ -708,5 +708,5 @@ make_view_changes_opts(StartSeq, Options, Args) ->
true ->
couch_mrview_util:changes_key_opts(StartSeq, Args);
false ->
- [[{start_key, {StartSeq+1, <<>>}}] ++ Options]
+ [[{start_key, {StartSeq, <<>>}}] ++ Options]
Review comment:
You understanding is correct, your fix is wrong. But this is a subtle bug,
so it'll take some explanation to do.
When we are folding over btree with `start_key` we are discarding all the
keys that less than our starting key. We can do that because our keys are
ordered. **In this case we are using [erlang native
collision](https://github.com/apache/couchdb/blob/master/src/couch/src/couch_btree.erl#L32)**.
The issue here is that the keys in `seq_btree` not just sequence numbers,
but tuples of `{Seq, Key}` and `Key` could be any data type. In case of
`test_basic_since` tests keys are integers.
What we want here is for our `start_key` is to be less than any key right
after sequence 5, which in this case `{6, 4}`. But in erlang binaries are
always bigger than numbers, so the key we are creating `{6, <<>>}` doesn't
work. When you are changing update sequence to be `Seq` instead `Seq + 1` it
fixes the case when view keys are integers, but **breaks all the cases when
keys are binaries**.
In other words you are changing the wrong part, we want `Seq` to be bigger
than value in `since`, but we want `Key` there to be less than any key view can
emit. The proper fix is to set custom `less` function to seq_btree. But I'd
prefer to keep this PR's scope about tests fixing and do the bug fixing
separately.
For now - revert this change and fix the test instead. You need to change
ddoc in `couch_mrview_test_util.erl` to emit binaries like this
```
{<<"baz">>, {[
{<<"map">>, <<"function(doc) {emit(doc.val.toString(), doc.val);}">>}
]}},
```
and then change expect in the test like this
```
Expect = {ok, [
{{6, <<"4">>, <<"4">>}, 4},
{{7, <<"5">>, <<"5">>}, 5},
{{8, <<"6">>, <<"6">>}, 6},
{{9, <<"7">>, <<"7">>}, 7},
{{10, <<"8">>, <<"8">>}, 8},
{{11, <<"9">>, <<"9">>}, 9}
]},
```
Does it make sense?
I'll open a different PR with custom less function for seq_btree after we'll
get this tests fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
