Enable Send-MailMessage on CoreCLR #3869

jeffbi · 2017-05-26T06:12:24Z

Contributes to the completion of #2123 and #2240.

Now that the requisite components are implemented in corefx, compilation of the Send-MailMessage cmdlet has been enabled.

This PR includes a test script that allows the cmdlet to be tested against a working local SMTP server, but only on Linux and under specific conditions. A separate issue will be opened for improving test coverage and including additional platforms.

iSazonov · 2017-05-26T06:47:22Z

@jeffbi You need add new record in test/powershell/engine/DefaultCommands.Tests.ps1

jeffbi · 2017-05-26T07:03:19Z

@iSazonov Thanks, I wasn't aware of this.

I see that there already is a record for Send-MailMessage. Do I just need to add -or $CoreWindows -or $CoreUnix into it?

iSazonov · 2017-05-26T07:09:55Z

Yes, you need edit and add -or $CoreWindows -or $CoreUnix

jeffbi · 2017-05-26T07:16:16Z

Done.

iSazonov · 2017-05-26T09:52:52Z

@jeffbi The tests is skipped on both CI - I believe we should remove its at all.

Maybe we can merge without tests if the cmdlet passes manual tests locally.

Related https://github.com/tedious/DovecotTesting/blob/master/resources/Scripts/Provision.sh#L28
http://michaelthessel.com/mock-mail-setup-for-travis-ci/

jeffbi · 2017-05-26T16:12:26Z

@iSazonov I would guess that the test is being marked as Pending due to the machine name not matching the one specified in the test script. The test does require some pretty specific configuration.

I have been able to manually send mail to myself on Linux with the cmdlet. I don't have an SMTP server for my Windows box to test with.

@JamesWTruher, @SteveL-MSFT, do you have thoughts on this?

jeffbi · 2017-05-26T16:45:20Z

@iSazonov So you know, when the CI checks failed, the error was pointing to test/powershell/engine/Remoting/SSHRemotingAPI.Tests.ps1 as the culprit, not my tests. It looks to me like that test script is trying to import a module that does not exist. Updating the entry in DefaultCommands.Tests.ps1 appears to have fixed the problem, but I don't quite see how.

iSazonov · 2017-05-26T18:00:27Z

I did not saw problem with test/powershell/engine/Remoting/SSHRemotingAPI.Tests.ps1 - I saw only failed DefaultCommands.Tests.ps1 that is expected.

I believe it is very expensive install a mail server on CI VMs.

SteveL-MSFT · 2017-06-01T16:50:27Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Send-MailMessage.Tests.ps1

+
+    It @PesterArgs {
+        $body = "Greetings from me."
+        $subject = "Test message"


shouldn't you also validate the subject?

SteveL-MSFT · 2017-06-01T16:52:19Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Send-MailMessage.Tests.ps1

+        }
+
+        $user = "jeff"
+        $inPassword = Select-String "^${user}:" /etc/passwd -ErrorAction SilentlyContinue


seems like user 'jeff' will almost always not exist so this test is always pending?

Changed to current user

SteveL-MSFT · 2017-06-01T16:54:12Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Send-MailMessage.Tests.ps1

+            return
+        }
+
+        $domain = "Chinstrap"


similar to above, this will almost always fail and thus be Pending, seems like if mail is installed you should be able to rely on root@computername?

What it actually should be is current_user$computername. While the test takes steps to avoid deleting an existing mailbox, it still made me nervous, so I left those hard-coded names in, partially to call attention to the situation.

To use root@computername, the test would need to be running as root in order to access root's mailbox. Is that what the CI environment is?

current user is probably correct

Changed to current user on current machine

SteveL-MSFT · 2017-06-01T16:56:44Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Send-MailMessage.Tests.ps1

+            $PesterArgs["Name"] += " (pending: mailbox not empty)"
+            return
+        }
+        $alreadyHasMail = $false


I don't understand this, you set $alreadyHasMail to $true above and then it's always set to $false here

This is one of those steps to avoid deleting an existing mailbox. In the AfterAll block, the $alreadyHasMail variable is checked before deleting the mailbox. The early return at line 104 keeps that set to $true if the mailbox is not empty.

SteveL-MSFT · 2017-06-01T16:59:42Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Send-MailMessage.Tests.ps1

+        $PesterArgs = @{ Name = "Can send mail message from user to self"}
+        $alreadyHasMail = $true
+
+        if (-not $IsLinux)


do we need to check if a smtp server is installed or is that always the case on Linux/Mac?

On Linux, an SMTP server is not installed by default, since there are at least two to choose from, perhaps more.

I'll have to look into how to detect from PowerShell if anyone's listening on port 25.

JamesWTruher

the important check to be added is to be sure that an SMTP server is installed and running on the system

JamesWTruher · 2017-06-01T17:02:31Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Send-MailMessage.Tests.ps1

+            return
+        }
+
+        $user = "jeff"


make this testuser please. I'm using this account name in another upcoming PR

Is testuser the username the CI tests will run as?

The permission bits on a user's mailbox are rw------- so a test might be able to send mail to another user, but it won't be able to read the mailbox to verify that the mail was sent.

We could use an environment variables:

AppVeyor - APPVEYOR_ACCOUNT_NAME

Travis - USER

JamesWTruher · 2017-06-01T18:13:01Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Send-MailMessage.Tests.ps1

+            return
+        }
+        $alreadyHasMail = $false
+        Set-Content -Value "" -Path $mailBox -Force -ErrorAction SilentlyContinue


do permissions need to be set on this mailbox? I seem to remember that the permissions for mailbox files are special

They don't seem to be special, just rw-------

JamesWTruher · 2017-06-01T18:14:05Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Send-MailMessage.Tests.ps1

+    It @PesterArgs {
+        $body = "Greetings from me."
+        $subject = "Test message"
+        Send-MailMessage -To $address -From $address -Subject $subject -Body $body -SmtpServer 127.0.0.1


one more check needed above to be sure that there's an smtp server running on localhost

JamesWTruher · 2017-06-01T18:17:54Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Send-MailMessage.Tests.ps1

+    AfterAll {
+       if (-not $alreadyHasMail)
+       {
+           Set-Content -Value "" -Path $mailBox -Force -ErrorAction SilentlyContinue


should this just be delete the file?

Unless we're running as root or as a member of the mail group, we can write to the file but we cannot delete it.

* Add check for SMTP server * Send mail to/from currently logged-in user on currently-named machine * Do not try to clear/create the mailbox before the test * Add validation of Subject

iSazonov · 2017-06-02T03:26:11Z

Can we use .Net Fake SMTP Server ?

mirichmo · 2017-06-06T15:49:00Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Send-MailMessage.Tests.ps1

@@ -0,0 +1,147 @@
+Describe "Basic Send-MailMessage tests" -Tags CI {


Does this test require Administrator on Windows or root on all non-Windows platforms?

Currently this test runs only on Linux. So long as the sender/receiver of the email is the currently logged-in user, running as root is not required.

I've been looking into ways to make tests more cross-platform, using a suggestion from @iSazonov, but it's going to take some effort.

My personal feeling is that I'd like to create a separate issue for testing Send-MailMessage and have this PR be taken as-is, but I don't know if that works with testing policies.

@jeffbi create a separate issue for that and link it to this

mirichmo · 2017-06-06T15:49:36Z

@SteveL-MSFT @JamesWTruher Have all of you concerns been addressed? Do you have any additional comments?

mirichmo · 2017-06-10T01:17:23Z

@JamesWTruher Do you have any additional comments?

JamesWTruher · 2017-06-13T18:47:00Z

LGTM

Enable Send-MailMessage for core

ff99911

msftclas added the cla-already-signed label May 26, 2017

Update cmdlet entry in DefaultCommands.Tests.ps1

04309b2

mirichmo requested review from JamesWTruher and SteveL-MSFT May 31, 2017 22:46

mirichmo self-assigned this May 31, 2017

SteveL-MSFT requested changes Jun 1, 2017

View reviewed changes

JamesWTruher suggested changes Jun 1, 2017

View reviewed changes

Changes per code review:

cae05cc

* Add check for SMTP server * Send mail to/from currently logged-in user on currently-named machine * Do not try to clear/create the mailbox before the test * Add validation of Subject

mirichmo reviewed Jun 6, 2017

View reviewed changes

SteveL-MSFT approved these changes Jun 6, 2017

View reviewed changes

jeffbi mentioned this pull request Jun 6, 2017

Create cross-platform tests for Send-MailMessage #3950

Closed

mirichmo merged commit 06dfe41 into PowerShell:master Jun 20, 2017

jeffbi deleted the send-mail-2240 branch June 22, 2017 01:30

ThreeFive-O mentioned this pull request Aug 25, 2018

Send-MailMessage Excluded from Non-Windows systems #3164

Closed

Zenexer mentioned this pull request Dec 19, 2019

pwsh as a dotnet global tool exhibits very unusual behavior with Ctrl-C #11314

Closed

		@@ -0,0 +1,147 @@
		Describe "Basic Send-MailMessage tests" -Tags CI {

Enable Send-MailMessage on CoreCLR #3869

Enable Send-MailMessage on CoreCLR #3869

Uh oh!

Conversation

jeffbi commented May 26, 2017

Uh oh!

iSazonov commented May 26, 2017

Uh oh!

jeffbi commented May 26, 2017

Uh oh!

iSazonov commented May 26, 2017

Uh oh!

jeffbi commented May 26, 2017

Uh oh!

iSazonov commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeffbi commented May 26, 2017

Uh oh!

jeffbi commented May 26, 2017

Uh oh!

iSazonov commented May 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesWTruher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Jun 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mirichmo commented Jun 6, 2017

Uh oh!

iSazonov commented May 26, 2017 •

edited

Loading