-
-
Notifications
You must be signed in to change notification settings - Fork 466
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Configuring when Should fails #1404
Comments
I think having the preference variable to opt out globally is good, but in terms of being more granular I'm not sure it necessarily makes sense to ignore $ErrorActionPreference. Since that variable is already widely know, introducing a new one will not be particularly discoverable. I suspect folx will only be very confused as to why Should is ignoring their set preference. Perhaps we should just use $ErrorActionPreference -- and -ErrorAction. Best to stick with what is already known and used here rather than completely reinventing this same wheel, in my opinion. |
I think the point, @vexx32 is that ErrorActionPreference is not respected by Doing so without a Pester-level configuration option would mean updating everyone's CI builds, at a minimum, but for us, it would probably require adding Here's an example of the problem. We frequently write tests like this: Describe "Set-Thing" {
Mock GetConfiguration { }
Mock GetConfiguration { "ImportantValues" }
Context "It calls GetConfiguration and passes the output to the Configuration function" {
It "Calls GetConfiguration" {
Set-Thing Important "KeyNote"
Assert-MockCalled GetConfiguration
}
It "Passes the output of GetConfiguration to the Configuration function" {
Assert-MockCalled ImportantConfiguration -ParameterFilter {
$RealKeyValue | Should -eq "ImportantValues"
$true
}
}
}
} Specifically, we frequently test our mocks by having the
Which makes it a lot easier to hunt down the bug than if the failure is just:
For us at work, if Further, although we might see some benefits on our oldest tests (where there may be lots of should statements in a single For what it's worth, if you want to play with that test, here's a dummy implementation: function ImportantConfiguration {
[CmdletBinding()]
param(
[string]$RealKeyValue
)
<# side effects that configure our environment #>
}
function GetConfiguration {
[CmdletBinding()]
param(
[string]$Name,
[string]$Value
)
<# look things up based on the current state of the environment and return #>
switch ($Name) {
# Lots of logic ...
default {
throw "Unknown Name: $Name"
}
}
}
function Set-Thing {
[CmdletBinding()]
param(
[Parameter(Mandatory)]
[string]$Name,
[Parameter(Mandatory)]
[string]$Value
)
<# checking and lookup stuff #>
$KeyValue = GetConfiguration -Name $Value -Value $Name
if ($Name -like "*Important") {
<# additional logic #>
# Set the value internally
ImportantConfiguration $KeyValue
} <# else, a few other options #>
} |
... Yep, that seems to be exactly what @nohwnd was mentioning. That's the premise of this issue, not so much a "should we do that" as more of a "how should we let users change the new default behaviour". Specifically, the main points of this issue seem to be that, in v5:
And lastly -- adaptations can be unlearned. Imo, this gives users of Pester the ability to determine how Should behaves, which gives us more granular control over our testing. Wins across the board there. I don't see a real need for specific preference variables for Pester functions; there is already functionality baked into PowerShell that lets us do the same thing such preference variables would allow us to do. |
With 5.0 being a breaking change, it's the perfect time to make this happen. I commonly get questions about how to do this. I would be happy with using -ErrorAction and ErrorActionPreference to have it stop on should failures. If we go that way, I suggest being able to specify the -ErrorAction on the It, Context, and Describe blocks too. I think it would be intuitive once we adjusted to it and I prefer it over the ErrorActionPreference because I can opt in and out on a case by case section of my tests. I would also recommend a new helper function that asserts that all of the should assertions have passed in the current scope. It could be a low priority feature, but it would fit with the idea and give users full control of how they use it. |
My original concern was a bit different from what you both mentioned, but you are raising some good points. My original concernJane uses the new behavior by keeping the global Pester setting to 'Continue', but finds out that it causes some of hers tests to fail unexpectedly. She would like to revert to the old behavior in multiple files to have easier time migrating. She expects the assertions to work as they did before, but does not expect the setting to have any impact on the rest of the code Here is some (contrieved) example of her code: function a () {
$f = Get-Item "A:\non-existing.jpeg"
[PSCustomObject]@{
Exists = $null -ne $f
Name = $f.BaseName
Path = $f.FullName
}
}
Describe "a" {
It "continues to the last error" {
$f = a
$f.Exists | Should -BeTrue
$f.Name = "file1.txt"
$f.Path = "C:\file1.txt"
}
} She expects to get non-terminating error from Get-Item, and then failure on the first assertion, because that is how it works in Pester v4: Describing a
Get-Item : Cannot find drive. A drive with the name 'A' does not exist.
At line:5 char:10
+ $f = Get-Item "A:\non-existing.jpeg"
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo : ObjectNotFound: (A:String) [Get-Item], DriveNotFoundException
+ FullyQualifiedErrorId : DriveNotFound,Microsoft.PowerShell.Commands.GetItemCommand
[-] continues to the last error 78ms
Expected $true, but got $false.
16: $f.Exists | Should -BeTrue BUT in Pester v5, specifying The solution to this would be to introduce a variable ShouldErrorActionPreference that would only impact Should, and that way she could specify it on the top of the file and get the old behavior without visiting every test in that file. Makes sense? Mocks@Jaykul that is a valid concern and I am aware of that pattern. I think an improvement would be to implement error collecting in the scope of Non-terminating errors@vexx32 It actually does not work that way, Pester is not collecting all non terminating errors, instead the Should writes into a collection of errors when the assertion does not pass. I am not sure why I did not implement it just as "error stream", it probably did not occur to me at the time. On one hand collecting the errors from Should directly and not making the test fail on all errors might be better, because then we can have scenarios as the one above, where non-terminating error is raised and the test still can pass (if we were testing that On the other hand sometimes you raise non-terminating errors by accident and you'd rather want terminating errors. Then the question is how to handle that, should Pester detect that non-terminating error was raised and do something? Should it just save it into some collection and do nothing, should it fail the test? Imho there are many different scenarios to consider and neither is a clear winner. Verify assertion pass
@KevinMarquette Could you expand on this please? What would be the benefit of having that? Right now this is what the It block does, it looks at the ErrorRecord collection on ScriptBlock invocation result and if there are any errors the test is considered to be Failed. The error can get to the collection in three ways:
In fact it should not be just the It block, it should be all code that uses Invoke-ScriptBlock, and that should be all code that invokes user provided scriptblocks. Also |
Looks like I only got half my thought on the page. I was thinking of the scenario where I have a large test with lots of the new should operators and I may want to have the ability to check half way in if anything has failed, then stop execution of the test if so. Like check twenty things and if all is good, execute the next hundred. I'm sure I would just break it into two tests. It's just a thought I had while reading this thread, not sure if there is a real need for it. |
What @nohwnd is describing sounds OK to me. It's what I was expecting (although I may not have explained myself well). It's worth pointing out that we could avoid the extra parameters by using default parameters: $PSDefaultParameters["Should:ErrorAction"] = "Stop" What @vexx32 is describing sounds like a new test tool. I'd be freaking out. Changing Pester to fail on all errors, but have |
Yep, I mentioned I wasn't sure exactly what the scope of this change would be, so appreciate the clarification @nohwnd. But it sounds like everyone's on the same page now. 👍 |
@KevinMarquette This seems like an edge case, and seeing how confused people are about Assert-VerifiableMocks I will let you implement this yourself if you ever need it. Once every test has access to the state it would be as easy as @Jaykul @vexx32 the default parameter is a nice idea, let's go that way. It removes the need for custom variable and is supported from PowerShell 3 above, which is what Pester targets. ( At least that is whay they say here: https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_parameters_default_values?view=powershell-6 ) To make it work I will still have to remove the
@Jaykul agree here. I am trying to do that within reason. I think that the change in Should is a rather big breaking change, but useful one. Luckily for all tests that have one assertion at the end of test the breaking change is transparent to the user. And for many tests where there are multiple assertions at the end of the test the new behavior is more desired anyway. Where the problems start are guard assertions. But the test will fail anyway, it's just that more work will be done than necessary. |
Okay trying it out, and the The problem is that the variable holds a hashtable which is a ref type, so we get different scoping when we do The first call modifies a the table and is then used in every scope, the second one assignes a new hashtable to a new local variable and is applied only in the current scope. Clever, and confusing... # scoped
$PSDefaultParameterValues.Clear()
&{ # one extra scope
$PSDefaultParameterValues = @{ 'Should:ErrorAction' = 'Stop' }
}
$PSDefaultParameterValues # remains empty
# not scoped
$PSDefaultParameterValues.Clear()
&{ # one extra scope
$PSDefaultParameterValues['Should:ErrorAction'] = 'Stop'
}
$PSDefaultParameterValues # remains Should:ErrorAction = 'Stop' |
@nohwnd yeah there is a bit of nuance there, and it may be worth mentioning how that plays out for Should since Pester does its own scoping in particular ways that may not be familiar to everyone (to complement docs on Describe and It, etc., And how they scope things). |
@vexx32 The scoping is a bit easier to understand in v5. What you will want in this case is probably one BeforeAll on the top of the file with the code in it. That will set it for the whole file, and just that file. But I will make sure to explain it. |
Late, since this is closed. For example
It's very easy to change the script to work but anyone who just installs the new version could be in trouble. Especially if they are using Pester to verify correct operation of a production environment (which is most of what I have used it for). The reason for posting is this a "Can / should / Must" split. So I would prefer to see a SHOULD which with the old behavior and a MAY/MIGHT/CAN which is optional . Just my 2% of a currency unit |
@jhoneill I hear you, but I think this is a better move than an additional cmdlet. With proper documentation this shouldn't pose any particular problem. Sure, it means you might need to update a few of your more sensitive tests, but on the whole I think it makes the most sense. I don't think it's wise to try to maintain a perfect backwards compatibility scene by duplicating cmdlets. Some backwards compatibility is necessary, but trying to retain the exact experience will be an exercise in frustration for both maintainers and users of Pester. In my opinion, the clearer and more visible the break is, the easier it is to document and understand, and thus adapt old tests for. Besides, until tests have been rewritten, the 4.9.x branch of Pester can still be used without issue. As far as major version changes go, this is pretty minor. 🙂 |
@vexx
Phone rings ... "Yeah ... uh-huh. So none of you can log in? OK Just let me ... oh holy .... " Same OS. Same PowerShell version. Same step to install the module. Same script we've run 1000 times before. The IT pro didn't know that since they last installed Pester a change has been made which will end their career ? It's not a call for 100% backwards compatibility because sometimes you have to break stuff to make progress. But "Breaking changes" which mean existing code stops running, are annoying, but not running does no active harm. That's failing safe (and things should go to or stay in a safe state if the script doesn't run.) We get a delay while things are put right. Having spent the holidays working with Pester and building on @adamdriscol 's selenium stuff I have a headful of syntax I'd like to see having thought about can/should/must it is fair to say that if this were a brand new project I'd be arguing that the word SHOULD needs to be "MUST"
etc. and
Must would stop dead in its tracks on a fail; should would continue but fail the whole test, and might would flag a warning but not fail the test. That's not where we are starting from. So it needs a way to make progress with safety and reasonable compatibility Three predicates doesn't mean triplicate code (yes, having the same code multiple times ... never good. D.R.Y and all that.) I wrote this doing the selenium stuff function Clear-SeAlert {
[Alias('SeAccept','SeDismiss')]
param (
... [ValidateSet('Accept','Dismiss')]
$Action = 'Dismiss' ...
)
if (-not $PSBoundParameters.ContainsKey('Action') -and
$MyInvocation.InvocationName -match 'Accept') {$Action = 'Accept'}
...
} So I have one command with 3 names. So one could get the syntax and behaviour above without tripling the code, which I agree is something to avoid. |
Once again, nobody is saying it's perfect, but nor would I recommend anyone update everything in production to a new major version without first verifying tests. Frankly, that's just not really doing due diligence? Versions 4.9.x and below of pester don't suddenly stop being available. I'm afraid I don't follow your hypothetical where everything falls apart, unless you're positing that a majority of IT professionals don't do their due diligence and upgrade production code before verifying it works as expected. This change isn't going to suddenly stop tests working at all. If your "tests" are modifying your production environment... that's shooting yourself in the foot to begin with, surely? I'm really not following how you find this harmful in the least. Inconvenient in some niche circumstances and requiring you to update some tests, sure. But not much more than that. |
@vexx32
If You don't follow how this could be harmful Copy & paste this
Do we ever see "This >><< must contain something" ? Now take this code.
Currently if $selection isn't set , delete doesn't run. Inconvenient is when code which was expect to run fails and you have to mend it. I have been telling IT pros that they should use Pester to test their infrastructure for years (I wrote https://jamesone111.wordpress.com/2016/06/01/a-different-pitch-for-pester/ 3 1/2 years ago, and I'd been saying it well before that). It isn't just used for unit testing software. I spoke to an old consultancy client a few weeks ago and they're using a script I wrote in 2017 to turn DSC changes into steps in per-machine pester tests. If they think something is wrong they run the test to see if the machine IS set the way they told DSC to set it. (Since some of the DSC classes are in house this is a seen as a better check than asking DSC)
Did you know in WWI soldiers shot themselves in the foot to get hospitalized and avoid fighting? That aside how do you test that a production environment is running normally ? How do you check that A is reachable, the B has replicated, that you can sign in to C, create, read update and delete a row on D. Or Send and receive messages on E, Or Read, Write and Delete files on F. Some are "look but don't touch", others verifications need a change. Either way pester is a good tool for it Your faith in IT pros is touching but as I near the end of a long career in support, training, consulting, evangelism, and consulting again which has taken me into and out of more places than I can count I can say: |
I think your expectation that all code from every possible new version work identically is simply unrealistic. There will always be changes in an update. Anyone running code in prod should be pinning versions and only upgrading production modules after verifying that nothing critical has changed. Yes, maybe some folx aren't. I don't think it's realistic to walk on eggshells just to enable these folx to continue to be careless. It seems highly unlikely that the one thing that trips them up would happen to be Pester if that's how they're handling their dependencies. I'm not sure where you get this idea that the tool should be assuming its users are incompetent. If we assumed that, Remove-Item wouldn't even exist as a cmdlet. It would be too dangerous. There is some level of simplifying things, of course, that's why tools exist, but Pester isn't designed to think for it's users. And shouldn't be, in my opinion. I think going that route is just an exercise in frustration for everyone involved in Pester's development. Please don't just hamper improvement to modules based on a devil's advocate worst-case scenario which assumes the person running production code is handling their dependencies completely carelessly. If that were the case, it would almost certainly break long before Pester changed something. Documentation and informing users is the answer here in my opinion, not catering to and encouraging poor coding practices. 🙂 I appreciate you may think differently, but at the end of the day I think this is a step in the right direction for Pester. ^^ |
This release is a major revision to Pester and I am already strongly advocating everyone to pin their version of Pester now before it gets released (if they are not already doing so). This release is already going to require me to carefully review every project before I move them to this version. This feels like the right release for this change. The core issue is more that PowerShell has poor dependency management. I don't want to go into that rant here, but that does contribute to the problem. Module owners should have no fear of introducing breaking changes if they bump the major version. One important question back on topic, does this change also apply to use of should outside of Pester test scripts? @jhoneill is not wrong in that People do use Pester in ways it was never intended. I am no longer surprised when I find a process that included the execution of a Pester tests and found it making production changes. Something that I would see as a validation, others see as a chance to validate and correct. If people are mixing the use of |
@jhoneill I understand your arguments, and Pester is keeping backwards compatibility where possible (v4 still supports PowerShell v2 for example.) I am also afraid of making breaking changes and that is why I am using semantic versioning, maybe not strictly, but definitely for a big breaking changes. I even had a discussion at Powershell conference Europe about what I am preparing for Pester v5 and whether or not I should make breaking changes or not. The voting was overwhelmingly for breaking changes. I am also very open about v5 having a lot of breaking changes, talk about it online and in person, open threads where people have chance to express their opinion, and will make sure that I will announce new version coming for a quite some time before putting it into powershell gallery. That said I am still sure that it will come to many people as a surprise and there will be many threads looking for reasoning for the changes, and calling for them to be reverted. But that is sadly the life of a project maintainer, calling for beta testers will give you 10 people actually trying the release out, and releasing it will give you 100 complainers, just checkout the numbers on pre-releases :) @vexx32 thanks for advocating for this change, I also think this is a step in the right direction. @KevinMarquette the expected behavior is to throw when not in Pester context. Implemented here: https://github.com/pester/Pester/blob/v5.0/Functions/Assertions/Should.ps1#L89 I should add an explicit test for it though. Filed in #1410 and the improvement @Jaykul suggested in #1409, both for 5.0 milestone so it is in the initial release. |
OK Stop there This is the complete opposite. Code which was prevented from running if an error occured now runs. A change which results in NOT RUNNING the code which tells an IT pro everything is fine is OK. That is failing safe.
Your youth, idealism and nativity and inexperience are showing*. SHOULD is the operative word. Yes they SHOULD. But you know what ? Things work if they don't. 30 years and hundreds of clients tells me where something is optional is usually not done. What you're saying is
(a) You know Barnum's famous “No one ever went broke underestimating the intelligence of the American public.” ? We would like to think that choosing our product was positively correlated with smartness, and thinking our users are dumbasses (negative correlation) is horrible and disrespectful. The [sometimes bitter] experience of 32 years full in IT* , and 3 years part time before that tells me the correlation is zero, not positive or negative. In addition people highly competent in one area sometimes work in areas where they are less competent. And even smart, competent people do stupid things. The worlds worst air accident was the fault of the most highly qualified training-rated pilot in one of the words safest airlines (he actually appear on posters for his airline). In Scuba (where I'm a lapsed instructor) highly competent experienced divers get killed by breaking basic safety rules - the more experienced you are, the worse risk you become. Intelligent, competent and rational people don't always behave that way, and two such people can combine to give a stupid/ incompetent/irrational result. Earlier this year I deleted a customers data because one of their employees wrote a script which did (b) Why do you think
This isn't about Pester thinking for users or not. It exists it does things, people decide if it meets their needs or not. The moment it went on line (like any other shared software) its creators lose control of who uses it and how. Whether it guides the user a bit better or bit worse isn't actually the point at issue. The point is are people relying an existing behaviour as a safety mechanism. Yes they are. Neither you nor I know how many are, or what damage might result from removing that safety mechanism. Remember at all times please, this is not a script which should run failing to run, it is runaway code doing things it should have been stopped from doing.
I'm not hampering anything. There is a beneficial, desirable change. It needs to be introduced in a safe way. What you see as "devil's advocate, worst-case scenario" is what those with a bit more experience call 'the real world' . Your plea not to be hampered is actually a plea to be allowed to abdicate any responsibility. You would presumably design a gun with no safety catch because everyone knows not to go the near the trigger (which has a guard on it) unless you mean to fire the weapon. Ultimately Stuff should be done. Doing Stuff has risks. Risks should be mitigated.
|
@jhoneill As I said, I recognise your opinion is different. Let's not derail this topic further; the point of whether or not breaking changes were going to be made seems to have been decided well before this topic was opened. 🙂 I'm sorry to hear that you've had some difficult experiences. I simply don't agree that those bear any relation to how a project like Pester should be maintained. Ultimately, the only way to really ensure nothing changes is to pin your versions. Otherwise, something will always break. I very much prefer @nohwnd's approach here, being communicative and interacting with the community. The alternative is oftentimes to make smaller changes over a period of time, which will break just as many things, but is much harder to communicate. I'll ask that you not derail the discussion with overly personal arguments, if you don't mind. I've no desire to see things get ugly here. 😊 |
That's all good. Breaking changes are necessary, tell people they can have progress, compatibility or freedom of choice and to be thankful if they get two.
Yes ! :-) You know the acronym "WYSIWYG" (what you see is what you get) a friend of mine coined "WIKIWIL" What I know is what I like. Whatever you change someone will say "I preferred it before" or "What idiot is in charge who changed X". Somewhere you added the Pester logo when invoke-pester runs I don't much care for. If you take it away again to please me, we both know you would get more complainers than happy people. I've have additional ideas about Should (and using MUST or MIGHT), and (of course) I think those are good ideas, but I also know that you have more knowledge of Pester, its users, and what they do than anyone else - especially me, I have my use and the use of a handful of clients which is tiny by comparison. I will back your right to make decisions on what to introduce. But I'll also weigh-in doing it safely. |
@vexx32 I've been young (and known, with certainty, that there were Right Things To Do and all the reasons for not doing things that way were Bad Reasons), but you haven't yet been old (with the the doubt that any way is much less wrong, and experience of seeing bad reasons behind more decisions good ones). Which makes it easier for me to understand the p.o.v you express (I can remember having it) than for you to understand mine. For clarity. I've come think of my typical user as someone moderately smart, under time-pressure and prone to human error. If I make possible for to do something wrong, sooner or later someone will; and as the person releasing software knows more about it than the person using it some responsibility rests on the me, to foresee negative consequences and make the grosser ones hard to get to. For, I hope the last time: This is not a breaking change. What ran before will run afterwards. On the safety of introducing specific changes in specific ways we do seem to represent the two poles. And that's OK. One voice saying "This is great - you/we should do it" and another saying "But be careful of..." is a way to make good progress - enthusiasm without experience is dangerous, experience without enthusiasm gets nothing done. Strong project groups have people who bring different perspectives - diversity of age or anything else isn't just about ideas of fairness, it brings results. I don't mind being at one pole and finding people don't agree with me - they have something more to interesting to say than "I agree", although that isn't always helpful |
This is a good discussion, maybe we can have it opt-in rather than opt-out. That will give us nice transition to v5 without yet another breaking change, and the new behavior will be easy to enable via configuration. I am sure that people who would find it beneficial will be able to do that, while opting-in will be less destructive when others will let v5 hit them unprepared. Right now I am structuring the configuration to be provided in multiple ways, as The migration per file will be still possible, you can either enable it per file (or even per scope) using the default parameters (but with 'Continue' instead of 'Stop'), or you can enable it everywhere and only disable it where needed. @jhoneill I am not trying to take cheap shots to shut down your arguments, but you should stop using the legacy syntax for your code, it will not be present in Pester v5 and will throw, see your code won't run aftewards and you are protected! 😁 Here is a migration guide if you are interested: https://pester.dev/docs/migrations/v3-to-v4#update-to-the-new-assertions-syntax |
You see what I mean about not everyone keeping up to date ? :-) Certainly a lot of the test we use for import-excel pre-date V4 and won't have been updated, and I think doug and I have just kept writing in the same old way. I'm now thinking did I ever read the notes when the version changed, or did I do that harassed IT-Pro thing of "Of course the new version will work like the old one". |
@nohwnd I have to share this which makes me laugh, although since it is heading for 2 in the morning, I'm not really laughing. I made sure I was using the new syntax, Runs fine on my machine, And what is installed in Azure Devops. ? Based on the path that's 3.4.0 . So using the new syntax I also need to be sure to update the pester version. If they skip 4 and upgrade to 5 anyone they won't get a transitional version. For people who aren't requesting a version this will be a problem -so ADO may never upgrade. |
@jhoneill if they're never upgrading, they can't possibly be factored into any decisions about new code by definition. 😂 |
@jhoneill that's most likely because Pester was shipped as part of windows 10, and still is? They won't upgrade that version and won't take it away either (because of compatibility). What host are you running this on? |
@nohwnd What Azure devops host - " vmImage: 'windows-latest' "
Indeed. |
Improvement for #1325
The Should assertion won't fail by default, instead it will store the error and continue executing, unless
-ErrorAction Stop
or$ErrorActionPreference = 'Stop'
is specified.Should ErrorActionPreference make the Should fail, or just the -ErrorAction and possibly ShouldActionPreference (and respective global Pester option) should make Should fail?
The reasoning here is that Eap will have impact on all the code that we are running and not just on the Assertions, and will also prevent the multiple assertions from working if eap Stop is used to force the rest of the code to fail.
I am inclined to do it this way:
Thoughts?
The text was updated successfully, but these errors were encountered: