PowerShell / PowerShell

PowerShell for every system!
https://microsoft.com/PowerShell
MIT License
44.63k stars 7.22k forks source link

Verify defaultRefAssemblies list capacity #12815

Closed xtqqczze closed 10 months ago

xtqqczze commented 4 years ago

static test to verfiy number of reference assemblies, see https://github.com/PowerShell/PowerShell/pull/12520#issuecomment-623543851

Shriram0908 commented 4 years ago

@xtqqczze I am getting error while running Start-PSPester on a fresh cloned git repo. The error message is

Process terminated. Assertion failed.
defaultRefAssemblies was resized because of insufficient initial capacity! A capacity of 153 is required.

Can you help me with this. Error.txt

vexx32 commented 4 years ago

Yeah, I don't think a debug.assert call was the right call (hehe) in that specific case. It's not worth bricking folks' dev builds over IMO.

/cc @SteveL-MSFT @iSazonov

xtqqczze commented 4 years ago

@vexx32 I have written #12840 to expand the capacity and remove the assert, but I am unable to fix this issue as I am not sure how to implement a static test.

kilasuit commented 4 years ago

@xtqqczze you could add a simple test along the lines of something like the below to this file to ensure that it is what it should be

https://github.com/PowerShell/PowerShell/blob/master/test/powershell/Modules/Microsoft.PowerShell.Utility/Add-Type.Tests.ps1

like

 It "loads the right number of assembilies" {
    ((Get-ChildItem ./src/Microsoft.PowerShell.Commands.Utility/commands/utility/AddType.cs | Select-String -pattern 'numberOfPowershellRefAssemblies' -AllMatches -Quiet -List ).Line -match '151')
}
GitHub
PowerShell/PowerShell
PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.
iSazonov commented 4 years ago

Really we have no need in the test. It is better to generate the const at design time, perhaps in TypeGen.

kilasuit commented 4 years ago

@iSazonov I added the above as way to show how it could be done in a pester test, not suggesting that it should be done in this instance,

Just like you also could also write a hyper strict test that detailed all of the required assembly names, not that one should be written

IISResetMe commented 4 years ago

What exactly are we trying to defend ourselves against here, and why does it have to violently crash my local builds?

xtqqczze commented 4 years ago

HEAD of master has been broken since 99da109 (#12772), when .NET was updated to 5.0.100-preview.5.20278.13, I'm wondering why there was no failure from CI at that time?

vexx32 commented 4 years ago

Unless I'm mistaken, CI tests on release builds which will automatically bypass the debug assertions by design.

xtqqczze commented 4 years ago

@vexx32 Perhaps tests should run on a debug build for a bot PR, since local tests will not have performed.

vexx32 commented 4 years ago

Not a bad call. No idea how you'd go about setting that up in CI, but it might be something for @TravisEz13 and the team to consider if it's possible.

TravisEz13 commented 4 years ago

I'd open an issue. We have made an active decision to keep the build release. The advice has to NOT use asserts. You should write a unit test if you need an assert.

xtqqczze commented 4 years ago

@TravisEz13 I did remove the assert in #12840, but you and @iSazonov wanted this change reverted https://github.com/PowerShell/PowerShell/pull/12840#discussion_r432908977

vexx32 commented 4 years ago

@xtqqczze something we could do is make the check in the method and store it in a static private field on the class; a test could check the value via reflection.

There are potentially other solutions, but I'm not sure what's appropriate here. I do agree that it's not a great idea to have a debug assertion here since it would get hit immediately on startup and wouldn't show up in CI.

microsoft-github-policy-service[bot] commented 10 months ago

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

microsoft-github-policy-service[bot] commented 10 months ago

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

microsoft-github-policy-service[bot] commented 10 months ago

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

microsoft-github-policy-service[bot] commented 10 months ago

This issue has been marked as "No Activity" as there has been no activity for 6 months. It has been closed for housekeeping purposes.