Arvados

Reviewing c088fcf

I am consistently getting UUID mismatches in the new test:

  1) Failure:
WebsocketTest#test_connect,_subscribe,_multiple_filters [/home/brett/repos/arvados/services/api/test/integration/websocket_test.rb:240]:
<"zzzzz-7a9it-hwe3vpcyrk8iv3j"> expected but was
<"zzzzz-j58dm-l6egg61nr8lxalp">.

302 tests, 997 assertions, 1 failures, 0 errors, 0 skips

The last part of the UUIDs are random, but the second part of the mismatch (7a9it expected vs. j58dm) is consistent.

Code-wise, my only concern is that #{cond_out.join ') OR ('} is dead code that could potentially confuse a reader, now that the conditions are already joined earlier. I think you could clear this up by rolling back the changes to the ws.filters.each block, and then changing OR to AND here. But I'd be happy with any change that only has one join on cond_out rather than two.

Thanks.

Brett Smith wrote:

Reviewing c088fcf

I am consistently getting UUID mismatches in the new test:

[...]

The last part of the UUIDs are random, but the second part of the mismatch (7a9it expected vs. j58dm) is consistent.

Fixed.

Code-wise, my only concern is that #{cond_out.join ') OR ('} is dead code that could potentially confuse a reader, now that the conditions are already joined earlier. I think you could clear this up by rolling back the changes to the ws.filters.each block, and then changing OR to AND here. But I'd be happy with any change that only has one join on cond_out rather than two.

I think you're misunderstanding the purpose of the bug fix here. The clauses within a single filter are joined with AND, but if there are multiple subscriptions, the compound filter statements are joined with OR. Added a a couple comments to try and clarify.

Peter Amstutz wrote:

Brett Smith wrote:

Code-wise, my only concern is that #{cond_out.join ') OR ('} is dead code that could potentially confuse a reader, now that the conditions are already joined earlier. I think you could clear this up by rolling back the changes to the ws.filters.each block, and then changing OR to AND here. But I'd be happy with any change that only has one join on cond_out rather than two.

I think you're misunderstanding the purpose of the bug fix here. The clauses within a single filter are joined with AND, but if there are multiple subscriptions, the compound filter statements are joined with OR. Added a a couple comments to try and clarify.

Okay, I get it now, thanks for explaining. b8fdb90 is good to merge, thanks.