All the relevant tests passed.

Wait, what signal is this test giving us now? Isn't expect_le essentially a
tautology? Maybe this test needs to be rewritten to better express its
intention?

On Tue, Mar 1, 2016, 10:43 AM Yang Gao notifications@github.com wrote:

Merged #5493 #5493.

—
Reply to this email directly or view it on GitHub
#5493 (comment).

That is true! :) - With this PR, I guess this is looking even more redundant (previously, there were not many EXPECT_LE checks)..

I guess the only value is in CANCEL_BEFORE_PROCESSING case where we do EXPECT_EQ
FWIW, The test does check all the other conditions of interest (like EXPECT_EQ(grpc::StatusCode::CANCELLED, s.error_code() and EXPECT_TRUE(context->IsCancelled()) in test_service_impl.cc etc..);

I will rewrite the test to clean up these redundant checks..

I'm not suggesting rewriting the code to eliminate redundant checks. I'm
suggesting rewriting the code to actually capture the intent of what was
there before. What was the actual intent of the test? And can we figure out
how to actually make code that represents that intention?

On Tue, Mar 1, 2016, 11:37 AM Sree Kuchibhotla notifications@github.com
wrote:

That is true! :) - With this PR, I guess this is looking even more
redundant (previously, there were not many EXPECT_LE checks)..

I guess the only value is in CANCEL_BEFORE_PROCESSING case where we do
EXPECT_EQ
FWIW, The test does check all the other conditions of interest (like EXPECT_EQ(grpc::StatusCode::CANCELLED,
s.error_code() and EXPECT_TRUE(context->IsCancelled()) in
test_service_impl.cc etc..);

I will rewrite the test to clean up these redundant checks..

—
Reply to this email directly or view it on GitHub
#5493 (comment).

Understood.

The actual intent of those test cases (in those cases where we had EXPECT_LE checks) was to somehow verify that the RPC was successfully cancelled in parallel while the request was being processed by the server (or client).

Other than verifying that the RPC does in fact get cancelled at the end (and also manually verifying - across several repeated test runs - that the client/server are processing a random number of messages between [0 to num_of_intended_msgs]), I cannot think of any other way to do this :(..

Probably worth filing an issue and thinking about it further down the road.

It seems like a problem someone will solve in the middle of a deep sleep in
a few weeks time.

On Tue, Mar 1, 2016 at 12:39 PM Sree Kuchibhotla notifications@github.com
wrote:

Understood.

The actual intent of those test cases (in those cases where we had
EXPECT_LE checks) was to somehow verify that the RPC was successfully
cancelled in parallel while the request was being processed by the server
(or client).

Other than verifying that the RPC does in fact get cancelled at the end
(and also manually verifying - across several repeated test runs - that the
client/server are processing a random number of messages between [0 to
num_of_intended_msgs]), I cannot think of any other way to do this :(..

—
Reply to this email directly or view it on GitHub
#5493 (comment).

Yeah. Created: #5520