Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@philos3 - thanks for the contribution! Before we can accept it you'll need to sign the CLA at https://cla.developers.google.com/.

Also, can you update the description of the PR to describe more what this PR is trying to accomplish. cc @brianquinlan

Thank you for your contribution! This project uses Gerrit for code reviews. Your pull request has automatically been converted into a code review at:

https://dart-review.googlesource.com/c/sdk/+/313042

Please wait for a developer to review your code review at the above link; you can speed up the review if you sign into Gerrit and manually add a reviewer that has recently worked on the relevant code. See CONTRIBUTING.md to learn how to upload changes to Gerrit directly.

Additional commits pushed to this PR will update both the PR and the corresponding Gerrit CL. After the review is complete on the CL, your reviewer will merge the CL (automatically closing this PR).

https://dart-review.googlesource.com/c/sdk/+/313042 has been updated with the latest commits from this pull request.

https://dart-review.googlesource.com/c/sdk/+/313042 has been updated with the latest commits from this pull request.

I left some feedback at https://dart-review.googlesource.com/c/sdk/+/313042

@philos3 would you like to address @brianquinlan's comments or should we close this PR?

@mraleph Could we keep this open for now? Let's say that, if @philos3 isn't responsive by Monday, then I'll complete this change.

Approved @philos3

@mraleph Could we keep this open for now? Let's say that, if @philos3 isn't responsive by Monday, then I'll complete this change.

The deadline has passed, so it is on your plate now :)

It's on my plate. Adding the messages was easy - writing tests for all the failure variations is hard ;-)

Any updates @brianquinlan ?

gentle ping

https://dart-review.googlesource.com/c/sdk/+/313042 has been updated with the latest commits from this pull request.

@mit-mit @vsmenon

Is this a priority? The original PR doesn't have tests so isn't mergable as-is. I can write the tests myself if getting this in is important.

Given we're just updating a status message, perhaps we can exempt this one. wdyt Vijay?

The PR outputs the entire response message in the error - this could be arbitrarily large.

I have a modified version of the PR that decodes the error in more detail without including the entire response body:

- if (response.statusCode != HttpStatus.switchingProtocols ||
- connectionHeader == null ||
- !connectionHeader.any((value) => value.toLowerCase() == "upgrade") ||
- response.headers.value(HttpHeaders.upgradeHeader)!.toLowerCase() !=
- "websocket") {
- return error("Connection to '$uri' was not upgraded to websocket");
- }
+ if (response.statusCode != HttpStatus.switchingProtocols) {
+ return error("Unexpected status code: ${response.statusCode}");
+ }
+ if (connectionHeader == null) {
+ return error("Response did not contain a 'connection' header");
+ }
+ if (connectionHeader.first.toLowerCase() != "upgrade") {
+ return error(
+ "Unexpected 'connection' header: ${connectionHeader.first}");
+ }
+ if (upgradeHeader == null) {
+ return error("Response did not contain an 'upgrade' header");
+ }
+ if (upgradeHeader.first.toLowerCase() != "websocket") {
+ return error("Unexpected 'upgrade' header: ${upgradeHeader.first}");
+ }
+ if (acceptHeader == null) {
+ return error(
+ "Response did not contain a 'Sec-WebSocket-Accept' header");
+ }

But writing tests for this is not trivial and it didn't seem worth it. @mit-mit if there is some reason why this should be a priority (e.g., people are complaining about error messages) then just let me know.

I assume this is stale. Please reopen if not.