Fix tests on WinPS, PS Core Windows and Linux #533
Conversation
|
Updated to include merged #532. |
|
Do we want to run |
|
WIndows and Linux pass. Just waiting for the macOS build. |
|
So there are failures on macOS but I'm not going to be able to debug these - don't have a mac. But both Windows and Linux are passing. I want to wait for the next macOS build to finish to see if the latest commit made any difference. |
test/Shared.ps1
Outdated
| } | ||
| else { | ||
| # On Linux/macOS, we can access the git binary via its path /usr/bin/git | ||
| $global:gitbin = (Get-Command -Name git -CommandType Application).Path |
There was a problem hiding this comment.
I've got two versions of git installed on Mac OS, and iirc that's normal with Homebrew, so test using this break at the moment. (Get-Command blah)[0] fixes them.
This PR shouldn't be running on |
|
I think Travis' thinking is this is a PR going in to develop, so gets treated as being on develop, rather than on the source branch? Kind of makes sense, I guess. With the fix above, it's 94 passes and 2 failures on Mac OS- Test Worktree, and Test bare repository in the Get-GitDirectory tests. Something to do with Mac OS's temp folders. |
|
The remaining Mac OS failures are due to the fact Mac OS's temp folder has a symlink in its path- |
|
@theaquamarine Do you want to take a whack at fixing the temp dir issue? Maybe we should have a function in Shared.ps1 called |
|
Wow, the wait on the macOS build is epic. :-( |
|
Argh, this is like waiting for water to boil. Strike that - much worse. :-) |
|
@theaquamarine Nevermind. I took a stab at it. Really want to get this PR in so my other (soon to be submitted) PRs won't have test failures. |
| @@ -1,6 +1,8 @@ | |||
| . $PSScriptRoot\Shared.ps1 | |||
| . $modulePath\Utils.ps1 | |||
|
|
|||
| $expectedEncoding = if ($PSVersionTable.PSVersion.Major -le 5) { "utf8" } else { "ascii" } | |||
There was a problem hiding this comment.
For PS Core we should default BOM-less files to UTF8NoBOM instead of ASCII, or maybe Default adequately covers both?
There was a problem hiding this comment.
There is no Default value for the -Encoding parameter on PS Core. But obviously there is a default value that is used when -Encoding isn't specified. I'm pretty sure that is utf8NoBom which is probably what we should be using on PS Core Linux/macOS. I think UTF8 is still a good default for Windows.
There was a problem hiding this comment.
Default encoding is UTF-8 without a BOM
PowerShell Core changes the default encoding to conform with the broader ecosystems.
There was a problem hiding this comment.
Looks like PS Core on Windows uses utf8NoBom as well.
The tricky part here that where we use this we are typically adding to an existing profile. I would consider it a bit rude to change the file encoding of someone's profile script. I think we are fairly safe to assume that PS Core profiles will be utf8NoBom given the default. Not so much for Windows PowerShell though. And our function to detect file encoding is pretty dumb. It can't tell the diff between uft8NoBom and ASCII because it only looks for a BOM (or lack thereof).
There was a problem hiding this comment.
This might work for encoding detection: https://gist.github.com/sdwheeler/777b9ec25e914bdeb0774471a149951c#file-get-fileencoding-ps1
I think Lee Holmes wrote this. I found it on poscode.org a few years back. It does a decent job with BOM-less files.
There was a problem hiding this comment.
It is probably better than what we've got but not by much. It appears to be just looking for a BOM as well.
There was a problem hiding this comment.
Re: no Default, I guess i misread https://github.com/PowerShell/PowerShell/blob/4bc52d2358222084738157a08907fac32d13bd3a/src/System.Management.Automation/utils/EncodingUtils.cs#L37
If there's no BOM, you have to make an assumption; trying to detect encoding is notoriously difficult, and we'll beyond our needs here. Pre-Core the PS assumption is ANSI; in Core the assumption is UTF8, on all platforms. I don't see much reason not to align with that. If it causes a problem for folks, they should get on board with UTF8.
The Holmes code is basically equivalent to what we have here: match by BOM, otherwise default to something (UTF7, in that case).
There was a problem hiding this comment.
in Core the assumption is UTF8
Well, UTF8 with no BOM just to be clear/pedantic. :-)
| $res | Should BeExactly "$PSScriptRoot [master +1 ~0 -0 | +0 ~1 -1 !]> " | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Describe 'Default Prompt Tests - ANSI' { |
| $GitPromptSettings.DefaultPromptPrefix = '[$(hostname)] ' | ||
| $GitPromptSettings.DefaultPromptSuffix = ' - $(6*7)> ' | ||
| $GitPromptSettings.DefaultPromptPrefix.Text = '[$(hostname)] ' | ||
| $GitPromptSettings.DefaultPromptSuffix.Text = ' - $(6*7)> ' |
There was a problem hiding this comment.
If we're assuming no colors, is there a downside to setting the property directly (meaning a new span is created from the string constructor) instead of setting Text?
There was a problem hiding this comment.
No, just a minor perf hit no one would likely notice.
|
I think the macOS build is hung. It has been > 5 hours now and the counter on the build page isn't even running like it usually is. UPDATE: I updated the branch and that has re-kicked the builds. |
|
Ugh, now the Linux build is hung as well. Have we exceeded some limit for the number of build allowed for a single PR? :-) |
|
Doh, looks like I broke Travis. :-) |
|
Should we just go ahead and merge this? The wait is getting ridiculous. |
|
According the incident status page, they're going to cancel all queued builds to try to get it running again. |
|
Looks like we're good https://travis-ci.org/dahlbyk/posh-git/builds/329647338 |
dahlbyk
changed the title
|
Merged!
We don't actually have CI running tests on PS Core Windows yet, correct? |
|
We do not. #527 |
Fixes #531
Still have a bit of clean-up work to do but wanted to see how the updated tests performed. Well, after PR #532 is merged since that PRs fixes several Pester test failures.