Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Conversation

@MaherJendoubi
Copy link

No description provided.

@dnfclas
Copy link

dnfclas commented Mar 26, 2017

@MaherJendoubi,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@MaherJendoubi
Copy link
Author

image

$"Response Content-Length mismatch: too many bytes written (12 of 5).",
logMessage.Exception.Message);
"Response Content-Length mismatch: too many bytes written (12 of 5).",
logMessage?.Exception.Message);
Copy link
Contributor

Choose a reason for hiding this comment

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

The ? is unnecessary since the previous assert will fail if it can't find a log message.

$"Response Content-Length mismatch: too many bytes written (12 of 5).",
logMessage.Exception.Message);
"Response Content-Length mismatch: too many bytes written (12 of 5).",
logMessage?.Exception.Message);
Copy link
Contributor

Choose a reason for hiding this comment

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

The ? is unnecessary since the previous assert will fail if it can't find a log message.

$"Response Content-Length mismatch: too many bytes written (12 of 11).",
logMessage.Exception.Message);
"Response Content-Length mismatch: too many bytes written (12 of 11).",
logMessage?.Exception.Message);
Copy link
Contributor

Choose a reason for hiding this comment

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

The ? is unnecessary since the previous assert will fail if it can't find a log message.

$"Response Content-Length mismatch: too many bytes written (12 of 11).",
logMessage.Exception.Message);
"Response Content-Length mismatch: too many bytes written (12 of 11).",
logMessage?.Exception.Message);
Copy link
Contributor

Choose a reason for hiding this comment

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

The ? is unnecessary since the previous assert will fail if it can't find a log message.

var token = context.RequestAborted;
token.Register(() => requestAborted.Release(2));
await requestAborted.WaitAsync().TimeoutAfter(TimeSpan.FromSeconds(10));
await requestAborted.WaitAsync(token).TimeoutAfter(TimeSpan.FromSeconds(10));
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't tie the semaphore to the cancellation token.

var stop = Assert.Single(events, e => e.EventName == "ConnectionStop");
Assert.All(new[] { "connectionId" }, p => Assert.Contains(p, stop.PayloadNames));
Assert.Same(KestrelEventSource.Log, stop.EventSource);
Assert.Same(KestrelEventSource.Log, stop?.EventSource);
Copy link
Contributor

Choose a reason for hiding this comment

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

The ? is unnecessary since Assert.Single will fail if it can't find the stop event.

@davidfowl
Copy link
Member

@MaherJendoubi I would hold off any of these changes until #1551 is this. Can you close this PR and revisit after it is merged?

@MaherJendoubi
Copy link
Author

@CesarBS Thanks a lot for the review. I learnt a lot. @davidfowl for sure. Thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants