Include Debug.Fail with GlobalLog.Assert in Net Mail. #12749
Conversation
| @@ -205,9 +206,14 @@ internal void EndGetConnection(IAsyncResult result) | |||
| throw new ArgumentNullException(nameof(recipients)); | |||
| } | |||
|
|
|||
| if (GlobalLog.IsEnabled && recipients.Count > 0) | |||
| if (recipients.Count <= 0) | |||
There was a problem hiding this comment.
I assume this was a bug? It's now asserting in the opposite condition. Regardless, this seems like a strange assert. Presumably _recipients is provided/populated by the dev? Are we validating that it's not empty prior to this call? If this is coming from user input, we should not be asserting.
There was a problem hiding this comment.
Yeah this is populated by _recipients, and we do throw in SmtpClient, if no recipient information was given. Will remove these asserts.
There was a problem hiding this comment.
Did the code diverge from Desktop? I would expect to see this assert when running our tests in CHK mode.
| @@ -257,10 +263,16 @@ internal MailWriter EndSendMail(IAsyncResult result) | |||
| throw new ArgumentNullException(nameof(recipients)); | |||
| } | |||
|
|
|||
| if (GlobalLog.IsEnabled && recipients.Count > 0) | |||
| if (recipients.Count <= 0) | |||
| GlobalLog.Assert("SmtpTransport::BeginSendMail()|recepients.Count <= 0"); | ||
| } | ||
|
|
||
| Debug.Fail("SmtpTransport::BeginSendMail()|recepients.Count <= 0"); |
There was a problem hiding this comment.
Search and replace: recepients => recipients
| @@ -205,11 +205,6 @@ internal void EndGetConnection(IAsyncResult result) | |||
| throw new ArgumentNullException(nameof(recipients)); | |||
| } | |||
|
|
|||
| if (GlobalLog.IsEnabled && recipients.Count > 0) | |||
There was a problem hiding this comment.
Why were these removed and how did the code work before (looks like this will always fail in the normal case)?
There was a problem hiding this comment.
Even without the inversion (should have been recipients.Count == 0), this is not something we should be asserting for. This is user-provided data. If we want to fail on such input, it should be with an exception in both debug and release; a debug assert doesn't do us any good.
Include Debug.Fail with GlobalLog.Assert in Net Mail. Commit migrated from dotnet/corefx@4db64b2
fixes #12674
cc @CIPop