GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Enable Send-MailMessage on CoreCLR by jeffbi · Pull Request #3869 · PowerShell/PowerShell

Conversation

@jeffbi

@jeffbi jeffbi commented May 26, 2017

Copy link
Copy Markdown

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iSazonov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Jeff Bienstadt
Enable Send-MailMessage for core
ff99911
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-already-signed label May 26, 2017
@iSazonov

Ilya (iSazonov) commented May 26, 2017

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jeffbi

jeffbi commented May 26, 2017

Copy link
Copy Markdown
Author

Ilya (@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented May 26, 2017

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Jeff Bienstadt
Update cmdlet entry in DefaultCommands.Tests.ps1
04309b2
@jeffbi

jeffbi commented May 26, 2017

Copy link
Copy Markdown
Author

Done.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented May 26, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jeffbi

jeffbi commented May 26, 2017

Copy link
Copy Markdown
Author

Ilya (@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.

James Truher (@JamesWTruher), Steve Lee (@SteveL-MSFT), do you have thoughts on this?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jeffbi

jeffbi commented May 26, 2017

Copy link
Copy Markdown
Author

Ilya (@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented May 26, 2017

Copy link
Copy Markdown
Collaborator

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mirichmo
Mike Richmond (mirichmo) requested review from James Truher (JamesWTruher) and Steve Lee (SteveL-MSFT) May 31, 2017 22:46
@mirichmo Mike Richmond (mirichmo) self-assigned this May 31, 2017
SteveL-MSFT
Steve Lee (SteveL-MSFT) requested changes Jun 1, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/Send-MailMessage.Tests.ps1

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

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jun 1, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

shouldn't you also validate the subject?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jeffbi jeffbi Jun 2, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/Send-MailMessage.Tests.ps1 Outdated
}

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

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jun 1, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jeffbi jeffbi Jun 2, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Changed to current user

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/Send-MailMessage.Tests.ps1 Outdated
return
}

$domain = "Chinstrap"

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jun 1, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jeffbi jeffbi Jun 1, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jun 1, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

current user is probably correct

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jeffbi jeffbi Jun 2, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Changed to current user on current machine

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/Send-MailMessage.Tests.ps1
$PesterArgs["Name"] += " (pending: mailbox not empty)"
return
}
$alreadyHasMail = $false

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jun 1, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jeffbi jeffbi Jun 1, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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)

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jun 1, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jeffbi jeffbi Jun 1, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jeffbi jeffbi Jun 2, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
JamesWTruher
James Truher (JamesWTruher) suggested changes Jun 1, 2017
View reviewed changes

@JamesWTruher James Truher (JamesWTruher) left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/Send-MailMessage.Tests.ps1 Outdated
return
}

$user = "jeff"

@JamesWTruher James Truher (JamesWTruher) Jun 1, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jeffbi jeffbi Jun 1, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Jun 2, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We could use an environment variables:

  • AppVeyor - APPVEYOR_ACCOUNT_NAME
  • Travis - USER

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/Send-MailMessage.Tests.ps1 Outdated
return
}
$alreadyHasMail = $false
Set-Content -Value "" -Path $mailBox -Force -ErrorAction SilentlyContinue

@JamesWTruher James Truher (JamesWTruher) Jun 1, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jeffbi jeffbi Jun 1, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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

@JamesWTruher James Truher (JamesWTruher) Jun 1, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jeffbi jeffbi Jun 2, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/Send-MailMessage.Tests.ps1
AfterAll {
if (-not $alreadyHasMail)
{
Set-Content -Value "" -Path $mailBox -Force -ErrorAction SilentlyContinue

@JamesWTruher James Truher (JamesWTruher) Jun 1, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

should this just be delete the file?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jeffbi jeffbi Jun 1, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Jeff Bienstadt
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
@iSazonov

Ilya (iSazonov) commented Jun 2, 2017

Copy link
Copy Markdown
Collaborator

Can we use .Net Fake SMTP Server ?
SteveL-MSFT reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mirichmo
Mike Richmond (mirichmo) reviewed Jun 6, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/Send-MailMessage.Tests.ps1
@@ -0,0 +1,147 @@
Describe "Basic Send-MailMessage tests" -Tags CI {

@mirichmo Mike Richmond (mirichmo) Jun 6, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jeffbi jeffbi Jun 6, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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 Ilya (@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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jun 6, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iSazonov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@mirichmo

Mike Richmond (mirichmo) commented Jun 6, 2017

Copy link
Copy Markdown
Member

Steve Lee (@SteveL-MSFT) James Truher (@JamesWTruher) Have all of you concerns been addressed? Do you have any additional comments?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
SteveL-MSFT
Steve Lee (SteveL-MSFT) approved these changes Jun 6, 2017
View reviewed changes
@jeffbi jeffbi mentioned this pull request Jun 6, 2017
Create cross-platform tests for Send-MailMessage #3950
Closed
@mirichmo

Mike Richmond (mirichmo) commented Jun 10, 2017

Copy link
Copy Markdown
Member

James Truher (@JamesWTruher) Do you have any additional comments?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JamesWTruher

James Truher (JamesWTruher) commented Jun 13, 2017

Copy link
Copy Markdown
Collaborator

LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mirichmo
Mike Richmond (mirichmo) merged commit 06dfe41 into PowerShell:master Jun 20, 2017
@jeffbi
jeffbi deleted the send-mail-2240 branch June 22, 2017 01:30
@ThreeFive-O ThreeFive-O mentioned this pull request Aug 25, 2018
Send-MailMessage Excluded from Non-Windows systems #3164
Closed
@Zenexer Paul B (Zenexer) mentioned this pull request Dec 19, 2019
pwsh as a dotnet global tool exhibits very unusual behavior with Ctrl-C #11314
Closed
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
jeffbi @mirichmo
Enable Send-MailMessage on CoreCLR (PowerShell#3869)
588c71a 
* Enable Send-MailMessage for PowerShell Core
* Update cmdlet entry in DefaultCommands.Tests.ps1
* Changes per code review:
  * 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov left review comments
@SteveL-MSFT Steve Lee (SteveL-MSFT) SteveL-MSFT approved these changes
+2 more reviewers
@mirichmo Mike Richmond (mirichmo) mirichmo left review comments
@JamesWTruher James Truher (JamesWTruher) JamesWTruher requested changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@mirichmo Mike Richmond (mirichmo)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@jeffbi @iSazonov @mirichmo @JamesWTruher @SteveL-MSFT @msftclas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.