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.

Sign up for GitHub

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

Fixing of connections_and_errors::cancel_with_error test false positive error. #1196

Merged
merged 3 commits into from
Aug 23, 2019
Merged

Fixing of connections_and_errors::cancel_with_error test false positive error. #1196

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
858ffd3
Fixing of connections_and_errors::cancel_with_error test which someti…
miherius Jul 18, 2019
42ca19d
Merge remote-tracking branch 'remotes/microsoft/master'
miherius Aug 18, 2019
a241556
Merge branch 'master' into request_cancel_with_error_test_fix
miherius Aug 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Release/tests/functional/http/client/connections_and_errors.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,9 @@ SUITE(connections_and_errors)
test_http_server::scoped_server server(m_uri);
pplx::cancellation_token_source source;

const auto r = server.server()->next_request();
responseTask = c.request(methods::GET, U("/"), source.get_token());
r.wait();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test appears to be ensuring cancelling the client before it has processed the whole message works OK; but this wait makes the test no longer test that.

Perhaps the wait and the cancel are in the wrong order?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I draw your attention that wait() is called for server-side of this test. Moreover, whole pull-request is intended only for proper using of test_http_server. So we straighten client logic of this test-case, and make it performance (delay) independent.

This test makes the assumption that the request will never reach test_http_server (and this assumption is true for most of the times). But it can't guarantee that.

At first, I remind that test_http_server throws an error when it get any unregistered requests, so we use next_request() here to register subsequent request. Then, if test_http_server never received previously registered request before destruction, it also throws an error, so we have to wait while our request is received by server.

Thus, the old logic has two scenarios: server did not received request (test succeeded), and server received request (false positive error). While new logic (this pull-request) forces single scenario: server received request. And the most interesting is that all these scenarios are not related to client object at all, because test_http_server does not respond to the client request, and all the client can get from the server is timeout. I will say more: same behavior (from client's point of view) can be achieved by simply removing test_http_server part, but I decided to keep this part of old test logic.

source.cancel();
}

Expand Down