pester / Pester

Pester is the ubiquitous test and mock framework for PowerShell.
https://pester.dev/
Other
3.11k stars 473 forks source link

Comments needed: Allowing a single test to run #721

Closed nohwnd closed 5 years ago

nohwnd commented 7 years ago

As mentioned in #657 and possibly quite a few other places, Pester test case filtering is limited only to tags on top-level describes. I would like to see this to change to allow for running just a single test case, which would allow us to:

As in most other frameworks.

To make this work we would need to change the way, Pester works a bit, but more importantly we would need to change the way test are written.

To allow running any code on demand we would need to control all of it's sorroundings. No code would be allowed outside of pester controlled blocks, and dot sourcing would be replaced with a custom function that would dot source on demand. In a sense It would become a first class citizen. Pretty much as a method is first class citizen in any C# testing framework. Limiting where code is placed is quite hard, but we possibly do not need a strong limit, just a set of recommendations.

I went through my tests and most of them already follow this schema, with occasional guard mock here and there, or some other common setup for multiple test cases, in general the tests look something like this:

$here = Split-Path -Parent $MyInvocation.MyCommand.Path
$sut = (Split-Path -Leaf $MyInvocation.MyCommand.Path) -replace '\.Tests\.', '.'
. "$here\$sut"

Describe "Remove-InactiveADUser" {
    #guard mock to avoid calling the real command by accident
    Mock Remove-ADUser

    It "Calls Remove-ADUser with correct parameters when user has been inactive for more than a year" {
        # -- Arrange
        Mock Get-ADUser { Name = "Jakub"; LastLogon = [datetime]::Now.AddMonths(-13) }
        Mock Remove-ADUser { } -ParameterFilter { $Name = "Jakub" }

        # -- Act
        Get-ADUser | Remove-InactiveADUser

        # -- Assert
        Assert-MockCalled Remove-ADUser { } -ParameterFilter { $Name = "Jakub" } -Exactly 1
    }
}

Which would transform to something like this:

Add-Dependency Remove-InactiveADUser.ps1

Describe "Remove-InactiveADUser" {
    BeforeAll {
        #guard mock to avoid calling the real command by accident
        Mock Remove-ADUser
    }

    It "Calls Remove-ADUser with correct parameters when user has been inactive for more than a year" {
        # -- Arrange
        Mock Get-ADUser { Name = "Jakub"; LastLogon = [datetime]::Now.AddMonths(-13) }
        Mock Remove-ADUser { } -ParameterFilter { $Name = "Jakub" }

        # -- Act
        Get-ADUser | Remove-InactiveADUser

        # -- Assert
        Assert-MockCalled Remove-ADUser { } -ParameterFilter { $Name = "Jakub" } -Exactly 1
    }
}

The changes are quite subtle, the dot-sourcing is replaced with a function that holds "reference" to the dependency and adds it on demand. The guard mock moved inside of BeforeAll and will be added before any of the tests start running.

This puts us in position where we can run all the test scripts and no real code will be run unless we allow it. Which in effect splits the test discovery (discovery) from test run (runtime). This would be a possible way of discovering tests in Powershell v2. From Invoke-Pester set a flag that will set describes and its into discovery mode and then run all the test scripts. In the discovery mode all the blocks will output their "info" (name scripblock, file, context, etc). Which would also allow for simple way of finding beforeall, beforeeach, aftereach and afterall, simply because the discovery run would end up with a list of "blocks" inside of it. This would result in a tree that describes the structure of the test suite, from that tree we can filter out all blocks that are not allowed to run (such as that they do not have the correct tag). And then the runtime would run all the blocks that should run in their respective scopes.

Another option would be to use AST and process the test scripts to see what is defined and what will need to be run. This is only possible in PowerShell v3+ but it would allow us to keep the pester internals that take care of the runtime mostly the same, and only add a the discovery and filtering layer on the top.

I prototyped the first way of discovering the tests (but without trying to make the scriptblock seem like they are running from a file), and it seemed to work quite well for the limited amount of tests that I gave it. But I've bee writing a lot more tests in c# than powershell. so please comment if this:

also please comment if it sounds like a good idea :)

bielawb commented 7 years ago

I don't really see a good reason to link tagging/cherry-picking tests with moving all the code to the "it" blocks. Most of the time I want to skip tests not because of the performance, but to get a "green" build without removing certain global tests.

Also: I very often share arrange/act between multiple it's, but not all of them - not being able to do that would make both code and run longer for no good reason. It would be great to have that sharing implemented.

Implementation-wise: I think AST is a better choice. Yes, it makes it v3+ compliant. But I guess you have to kill support for v2 at some point...

nohwnd commented 7 years ago

@bielawb I probably need more explanation of what you mean. I mostly want to run fewer tests, because I am testing one thing among hundreds of tests. Running one (or ten) tests vs running five hundred tests takes ten seconds vs a minute and a half. Which is quite noticable when I run tests very often. At the moment I cannot do that without splitting my tests into multiple files, or tagged describes. I would also like to have tooling support (like from the MS Test runner/ Resharper test runner in VS, or something similar in VS Code in the future), where we have a tree of tests in the current suite and can run any of them.

Note: Describe is used in the Pester v4 sense where Describe and Context are the same thing.

You could still share arrange/act by putting it into BeforeAll block. This allows for less flexibility then putting code all over the place (you for example cannot setup for the first five Its, and then setup for the next five Its by putting your setup code inside of the Describe and after the five It blocks). But you could still share the setup within that given describe block, and possibly from all parent Describe blocks. Which from my purist point of view is more than enough, because tests should ideally be independent and bigger amount of independent work should be favored over less amount of dependent work. Of course some sacrifices need to be made from time to time, but in those cases we should minimize the bad stuff (dependencies, complexity) and maximize the good stuff (less computation needed). If we really really wanted then we could have multiple BeforeAll blocks within a describe and then say which It should depend on which BeforeAll as long as Pester can run those blocks on demand and not on script run.

adbertram commented 7 years ago

I like the built-in support for dependencies. It would be great to abstract away all of the dot-sourcing and forced module imports that are necessary now. Simply saying Add-Dependency Foo.ps1 or Add-Dependency Foo.psm1 would be awesome.

Will this change how mocks are scoped, I assume? I really dislike the current mocking scope where a mock in an It block can affect mocks in other it blocks. It'd be awesome if a mock defined in an it block was scoped down just to that it block.

I don't think that you should be concerned with infrastructure validation. Pester is a unit testing framework. Although it can be used for that purpose, I think another tool should be written for that some day.

Like @bielawb , I also like using "shared" mocks in the describe block on occasion. It would seem awkward to place them in the BeforeAll block. That doesn't seem intuitive at all.

I already use the AST in some of my tests to build a framework around Pester. PowerShell v2 is extremely old now and support should definitely be dropped. I don't think that forcing a dependency on v3+ is a problem at all.

nohwnd commented 7 years ago

@adbertram I did not think about Mocks much, but I think they should change and be scoped to It block. I don't remember the last time I used Assert-MockCalled on anything else than It scope. We should still keep the concept of guard mocks that allow you to disable call to the real cmdlets. but maybe as a separate concept. Same way we should also see if anyone is using the fall-through of mocks where you can call mock when giving some parameters, and call the real command when calling with different parameters. That also seems to get into way much more than being useful. And they probably should also return the mock object. I don't want to get too deep into fantasizing about better mocks, so I will stop here.

For the infrastructure validation, I partially agree, but if we make the framework more modular we can simply reuse the test runner part of Pester and run whatever tests/validations we want. You can do it now as well, you are just limited to the "unit testing" approach with one failure per test, which might not be ideal all the time, but you can also work around that and get the nice colors and summaries from Pester.

So what would seem intuitive for shared setup, except for putting the code directly to the body of describe?

adbertram commented 7 years ago

Agreed about the mock scoping. That is, by far, my biggest gripe with Pester. I wish that could be changed yesterday. :)

I always define my "shared" mocks together in the describe block and group them with #region tags. I'd love to have some kind of separator like a scriptblock to place them all in. Something like this:

describe 'foo' {
    Mocks {
        'Get-AdUser' = { return $null }
        'Set-AdUser' = { return $null }
    }
}

This would allow the user to define lots of mocks all at once and have them all in a controllable scriptblock. It would also prevent from having to use the mock function call over and over again.

Jaykul commented 7 years ago

Mocks are scoped to Describe|Context in 4. I think that's enough.

function A { "A" }

Describe "a test" {
    Context "B Test" {
        Mock A { "B" }
        It "Returns B" { A | Should Be "B" }
    }

    It "Returns A" { A | Should Be "A" }
}
Jaykul commented 7 years ago

Regarding the OP ...

At work, we constantly run just the tests for changed code, but we do it by convention:

  1. Our module source code has each function it it's own file.
  2. Our module tests have one Describe "FunctionName" for each function under test. Each Describe is in it's own file.
  3. Additionally, we have a Describe "ModuleName" for each module under test

I have a Invoke-Test -Auto which uses source control to determine which files have changed, and uses the -TestName parameter to run the Describe for those and for their modules.

We also only run code coverage in these -Auto scenarios, and pass the list of changed files -- testing code coverage is way too slow to run routinely on the whole code base.

The only case where I would want to run a single It instead of the whole Describe is when something fails, and I'm trying to fix it...

Gherkin syntax already works this way...

With Gherkin, there is no Tests.ps1 file that is run sequentially -- instead, there are one or more Steps.ps1 files which just define named scriptblocks (also known as functions?). Anything not inside a step block is basically just run once at startup and I strongly recommend people not run anything that way (I recommend module import/export in BeforeAll/AfterAll).

The Gherkin feature runner scans each Feature file(s) and runs the appropriate steps for each scenario.

The Gherkin runner also has a feature that allows running just the tests which -FailedLast time. This is done at the scenario level (which you would call an "it" but which I map to a "context"). In order to map Feature/Scenario/Step to Pester, I ended up mapping each "scenario" to a "context" and each step to an "it" rather than generating "it" blocks with multiple steps in them. This results in a lot more "it"s than regular Pester tests (because given, when, then = 3 blocks), but since they're reusable, it balances out ;-)

P.S. One of the problems I'm dealing with in this release is that the Describe/Context change broke my Feature/Scenario/Step relationships somehow, so this may change a little before we're done.

nohwnd commented 7 years ago

@jaykul That is almost exactly how I imagined it would work, while you already implemented it. :) I like the convention of one function per file, and I tried it for a while, but I find it quite fiddly, and slowing me down a lot on the start of a project when I am finding the best way organize the code in functions. Maybe I am not using the right tools.

I also do not want to run just a single test most of the time, but if I am able to do that I am able to run as many or as few as I want.

In both cases (future Pester, current Gherkin) the runner needs to have take control over the code that is being run, to allow running smaller scopes without missing context. I really need to look at how you do it. :) But I think the biggest hurdle will be teaching people to use the new approach. Maybe if we AST the code and throw errors when any code is found outside of the permitted blocks, but that's something I'd like to avoid.

splatteredbits commented 7 years ago

This is an area of Pester I'd like to see improved. All the examples out there use Describe to define a test fixture and It blocks to define test cases.

Descibe 'New-Item' {
    It 'should create files' {
        $path = Join-Path -Path $TestDrive.FullName -ChildPath 'fubar'
        New-Item -Path $path -ItemType 'File'
        $path | Should Exist
    }

    It 'should create directories' {
        $path = Join-Path -Path $TestDrive.FullName -ChildPath 'snafu'
        New-Item -Path $path -ItemType 'Directory'
        $path | Should Exist
    }
}

What is the point of a Describe block if there's only one? Doesn't the script file defined the test fixture? Why does Invoke-Pester have a TestName parameter to run single Describe blocks if there's only ever one in a file? It wasn't until I learned Chef and started using RSpec, its Behavior-Driven-Design (BDD) testing framework, that I realized what was wrong.

Describe blocks should be your test cases and It blocks should be your assertions, as this RSpec test shows:

require 'serverspec'

# Required by serverspec
set :backend, :exec

describe "Git Daemon" do

  it "is listening on port 9418" do
    expect(port(9418)).to be_listening
  end

  it "has a running service of git-daemon" do
    expect(service("git-daemon")).to be_running
  end

end

Following this pattern, my original test fixture would look like this:

Describe 'New-Item when creating files' {
    $path = Join-Path -Path $TestDrive.FullName -ChildPath 'fubar'
    $fileInfo = New-Item -Path $path -ItemType 'File'

    It 'should create files' {
        $path | Should -Exist
    }

    It 'should return FileInfo object' {
        $fileInfo | Should -Not -BeNullOrEmpty
        $fileInfo | Should -BeOfType ([io.fileinfo])
    }
}

Describe 'New-Item when creating directories' {
    $path = Join-Path -Path $TestDrive.FullName -ChildPath 'fubar'
    $dirInfo = New-Item -Path $path -ItemType 'Directory'

    It 'should create directory' {
        $path | Should -Exist
    }

    It 'should return FileInfo object' {
        $dirInfo | Should -Not -BeNullOrEmpty
        $dirInfo | Should -BeOfType ([io.directoryinfo])
    }
}

This is how we write tests at our company and how all new tests for my open-source projects (and here) are written. I wish this was the pattern taught and promoted across the PowerShell community because I think it is the way Pester actually wants you to construct your tests. If you read the Pester docs for the Describe function's Fixture parameter (the one that takes in the describe script block), it says:

The actual test script. If you are following the AAA pattern (Arrange-Act-Assert), this typically holds the arrange and act sections. The Asserts will also lie in this block but are typically nested each in its own It block. Assertions are typically performed by the Should command within the It blocks.

The help topic for the It function's Test parameter (the one that takes in the script block), says:

The script block that should throw an exception if the expectation of the test is not met. If you are following the AAA pattern (Arrange-Act-Assert), this typically holds the Assert.

So, Describe blocks should arrange your test and act (performing the action being tested). The It blocks should contain the assertions. What I'd like to see is the functionality currently around the It blocks copied to Describe blocks.

I think the Pester community should encourage people to switch to multiple Describe blocks in a test fixture and update the Describe parameters to make that possible. I think that would solve adding support for running individual It blocks.

Jaykul commented 7 years ago

I totally agree with @splatteredbits -- particularly now that mock scoping is fixed, the right design for Pester tests is to have a script file as a suite, with many describe blocks for tests, and it blocks for each assertion. In Pester 4, that means mocks are scoped to the test, and you can run a single test by -TestName, or a single suite of tests (i.e. all tests against a single function) by -Script file.

In fact, in that spirit, I should probably tweak the gherkin runner to only run it's assertion steps through it -- currently, the Gherkin runner treats each scenario as a describe, but runs even the arrange and act steps through it blocks, just to show them in the output πŸ˜€

nohwnd commented 7 years ago

@splatteredbits @Jaykul so I was doing it wrong the whole time? That's quite possible, I started with testing with Pester, and I actually never worked with RSpec, what I did work with were TDD frameworks and it seemed to align pretty well with the examples that already were in Pester. There were probably few articles by people who formed pester that highlighted how to use it (or maybe I am just imagining it :)) So It became synonymous to "test" and Describe and Context became just a grouping construct to me. Which lends itself very well to TDD, where you write a lot of small tests.

For BDD (or at least how I understand it), it makes total sense to do it the way you describe it, even though ideally there would be just a single assertion per It to give you the most complete picture of what is happening. Then the It would become synonymous with Should.

How would the approach that you describe work with TDD style where I want to validate very small parts of the system under test independently? Or do we need a different syntax for that?

Jaykul commented 7 years ago

The help for Describe actually suggests this method 😁

-Fixture The actual test script. If you are following the AAA pattern (Arrange-Act-Assert), this typically holds the arrange and act sections. The Asserts will also lie in this block but are typically nested each in its own It block. Assertions are typically performed by the Should command within the It blocks.

And I think the examples that @splatteredbits wrote make a great case for how it can provide value (documenting) even when all it does is wrap a single should ...

However, I'm not ready to say you're "doing it wrong" -- certainly the examples for RSpec include samples doing everything inside the it: image

To be honest, I've always hated the rspec decorators/syntax. I learned testing using Arrange-Act-Assert, and I can do that comfortably even when unit testing, so I was able to comfortably switch to Gherkin syntax for BDD because it's given-when-then maps quite closely to Arrange-Act-Assert. I'll admit that when I'm writing unit tests, the ceremony of having the separate .feature file feels like overkill.

Incidentally, regarding @splatteredbits's request about making things map to describe which currently wrap it:

  • BeforeEach and AfterEach should run before/after each Describe block instead of each It block.
  • A -Skip parameter to skip [a whole] Describe test
  • A -Pending parameter to mark a Describe test as in-progress
  • A TestCases parameter to pass in test data

It makes sense to me for those to all be implemented on Describe the same way they're implemented on It -- but you can do -Skip and -Pending with tags (and -ExcludeTags) and Set-TestInconclusive

... that's how the Gherkin runner works πŸ˜‰ -- there are BeforeEachFeature and BeforeEachScenario (and matching After) hooks, and testcase tests are handled by Scenario Outline and Examples, and you can only skip things with tags and mark things inconclusive by calling Set-TestInconclusive or not writing the code-behind step.

michalporeba commented 5 years ago

This discussion is frequently referenced but hasn't been updated for almost 2 years. What is the latest? What is the actual plan? Is Pester going to follow the RSpec example as explained by @splatteredbits and @Jaykul? Having mocs defined in individual It seems to add a lot of unnecessary code, and using BeforeEach means losing flexibility. Is there another issue where the latest plans are explained?

nohwnd commented 5 years ago

I will try to sum up the plans here:

Doing Arrange and Act inside of Describe and Assert in one It per assertion.

This is a style that is already possible in v4, and will continue to be possible in v5, but the code in Describe should be placed in BeforeAll block, to enable test discovery to work. Putting your setup code is encouraged ever since Before* blocks were introduced, but till v5 there is no strong reason to actually use them.

I think this style of writing tests is quite common for integration tests where you do one expensive setup on the top, and then assert multiple things about the result. I for example do it here where I call external API once and then look at the result.

On the other hand, this is not the single way to do it. With Assert-Equivalent I could take the whole object and compare all the properties in much less lines of code, then doing the setup directly in one It would make sense and would not lose me any information on failure.

So both ways need to be supported.

BeforeEach and AfterEach should run before/after each Describe block instead of each It block.

This won't happen, it would remove a lot of functionality that people rely on (and learned to use), and would enforce one particular style of writing tests - forcing the whole community to re-learn Pester. Instead I was thinking to add a BeforeEach that will setup the blocks (and of course teardowns), in fact it's already implemented in the new v5 internals, I just need to have a look on it and figure out when exactly the setups should run. And then add the api. #335 is requesting this (or a variation of it)

A -Skip + -Pending parameter on Describe This is planned, in v5 both blocks and tests (Describe/Context + It) should have extended filtering and result capabilities. #856 #657

A TestCases parameter to pass in test data (on describe) This is also planned #832

Breaking changes

Mock

Scoping

The scoping of different blocks will change to be more logical. Right now BeforeAll block is invoked before Describe.

Describe "a" {
    Write-Host "in describe"
    BeforeAll { write-host "In beforeaall" }  

    It "b" {
    }
}

# output: 
Describing a
In beforeaall
in describe
  [?] b, is pending 50ms

Which makes it very awkward when you put function in Describe and expect that it will be available in BeforeAll. Instead the execution will be more top down (and documented). The setup will be invoked right before the first It block. This way variables and functions defined in Describe will be available in BeforeAll. (there should be no free-floating code in the block anyway, but the change still makes sense because the execution order will be more logical, easier to explain, easier to reason about, and there can could be more blocks that are "like" BeforeAll in the future.)

BeforeEach is invoked in the same scope as test and AfterEach. So variables are shared among them (BeforeAll and AfterAll are invoked one scope above them to prevent variables in one test leaking to the next)

... there is a lot of other stuff, that I need to figure and decide on as I go.

nohwnd commented 5 years ago

The changes are implemented. Mocking and scoping is very similar to what it was before, just slightly better. Mocks scope to It by default, and BeforeAll is scoped inside it's containing describe not outside of it.

The tests are collected during run phase, and blocks that allow running code during just run and not during Discover are provided. That way the user code can be kept very similar to what it was for v4, just the setups got into Add-Dependency and BeforeAll blocks.

The approach that Gherkin takes (if I understand it correctly) I did not take, gherkin is running the code in total isolation in a module, I want the code to run "in the file" so I am not simply collecting the scriptblocks and then executing them, instead I collect the scriptblocks and when they execute in the file I lookup additional info for them, so the contexts, automatic variables and all that other stuff is correct. With exception of Before After blocks that need to be "transported" to the correct place and invoked there.

More info is here. https://github.com/pester/Pester/blob/v5.0/README.md

splatteredbits commented 5 years ago

I do not like the -Focus switch. You should not have to modify test code in order to run a single test. Everywhere else I've ever run automated tests, you could always, always, always use your tool or the command line to run a single test without modifying any code. People are going to forget all the time that they added a -Focus switch to their code. It's going to get committed and pushed and for years people will not notice that some test aren't running. Now I have to write a test that checks all my other tests to ensure I didn't forget to add the -Focus switch.

Pester must have a command line switch to allow for running the Describe/It/Context block the user chooses. I've submitted a PR with this functionality. I'd love to see it in the next version of Pester 4.

https://github.com/pester/Pester/pull/1374

nohwnd commented 5 years ago

@splatteredbits I understand where you are coming from, this exact thing happened to me few times as well, but on the other hand I find the -Focus switch to be extremely useful for local development. What I was planning to do is to add a build server detection based on the variables that most known build servers publish (azure, travis, gitlab, appveyor, jenkins and team city), and ignore this switch.

Another option would be to add Pester preference variable to be used on CI server (for CI servers that we are not detecting or that don't publish any known env variables).

On the other hand maybe this is too complicated and could be easily solved for most by integrating better with VSCode (which is something I am talking about with Chris Bergmeister) which would allow us to run just a single test / describe by clicking a context menu in VSCode, just like it is possible now but not just for top-level Describes. In that case the -Focus switch functionality would be almost entirely replaced by this integration. Except for cases where you want to run multiple Focused tests/blocks, which is something most of us can like live without.

Pester must have a command line switch to allow for running the Describe/It/Context block the user chooses. I've submitted a PR with this functionality. I'd love to see it in the next version of Pester 4.

That functionality is already there in v5, it allows you to run tests / describes based on tags and based on path to the test. In the future I expect to add -Filter which would take a scriptblock predicate (if I did not publish that already). This scriptblock would take the Test metadata object as input, and you could provide any condition. The test object describes the test and is also linked to its parent Describe (and it's parent and so on). So this would give you a ton of power to filter tests based on an arbitrary condition. This way the api of Pester stays small but the possibilities will be huge.

But I hear your feedback and will re-consider publishing the -Focus functionality as part of the first stable release. πŸ™‚

splatteredbits commented 5 years ago

After reading your comment, I realized that -Focus is designed to run by people who run their tests files:

> .\My.Tests.ps1

So, I think it is fine for use in those scenarios.

Adding build-server detection just seems like a lot of complexity that you're committing to maintaining going forward. (The best code is code that doesn't exist.) And what if someone was trying to debug why a test is only failing on the build server and wanted -Focus turned on?

But, for teams that don't run tests directly, and go through Invoke-Pester, having a switch to filter It blocks is critical. Because it is missing, we have a whole standard way of writing tests that goes against the "recommended" approach:

Describe 'MyFunction.when something' {
    It 'should behave this way' {
        Init
        # Given/When/Then
    }
}

Describe 'MyFunction.when something else' {
    It 'should behave some other way' {
        Init
        # Given/When/Then
    }
}

We can't use any of Pester's BeforeEach/AfterEach functionality, because they only apply inside a Describe block, so we have to write our own Init functions that every test calls before doing its thing. All because we need to be able to run individual tests.

I can see VS Code using a switch on Invoke-Pester, too, when someone chooses "Run this test.". (I have no idea how VS Code runs Pester tests. I run all my tests on the console by running Invoke-Pester.)

splatteredbits commented 5 years ago

Where should I got to have a conversation about some changes you're making in v5? I'm not sure I like the "run twice" approach. That's going to be a lot of work for our team to upgrade. We do a lot of stuff outside Describe blocks. I'd like to suggest using PowerShell's AST to determine the test structure before executing them.

nohwnd commented 5 years ago

But, for teams that don't run tests directly, and go through Invoke-Pester, having a switch to filter It blocks is critical.

As I said, that is present in v5 and I won't be adding it to v4 because of the complexities it has, such as running the appropriate BeforeEach / AfterEach blocks.

I can see VS Code using a switch on Invoke-Pester, too, when someone chooses "Run this test.". (I have no idea how VS Code runs Pester tests. I run all my tests on the console by running Invoke-Pester.)

That is how VSCode does it, it runs Invoke-Pester with appropriate filter, but the filtering is very limited in v4.

Where should I got to have a conversation about some changes you're making in v5? I'm not sure I like the "run twice" approach. That's going to be a lot of work for our team to upgrade. We do a lot of stuff outside Describe blocks. I'd like to suggest using PowerShell's AST to determine the test structure before executing them.

I am summing up my progress in the readme on branch v5. There is also link to a discussion thread, so you can raise your concern there, or start a new thread, or discuss it here. The "run twice" approach changed a bit, and it's not described yet I am afraid. Now the first pass will collect the It blocks into a structure and then invoke them. It is still needed to put all your code into Pester controlled blocks, as the code is first doing the discovery pass, and then the execution pass. The difference from the previous approach is that the script itself is not running twice, just once, so all code that is outside of Pester blocks would run during discovery (ideally no such code would exist).

AST is one of the things that sound like a good idea for looking up tests but it's very difficult to actually do in practice. Tests can be everywhere and can also be generated from loops and functions, that would force the discoverer to resolve files and functions and defer execution of loops, all of which is difficult during static code analysis, but quite simple when just running the code, as most of the work is done by PowerShell itself. This also allows me to be aligned with how PowerShell works without trying to re-implement a significant piece of code resolving logic myself (and keeping it aligned with the behavior of each supported version of PowerShell).

felixfbecker commented 4 years ago

I use .skip() and .only() (equivalent of -Focus) in mocha all the time. I wish I could use them anywhere in Pester too (i.e. on It, Describe, etc). Commenting tests out, or adding tags and then passing those tags into the command line takes a lot more work.

In mocha, this does not require mocha to have full control over dependencies (as proposed with Add-Dependency). imports are still top-level and if they cause side-effects, you may have problems, but usually not. Basically it's the developers responsibility to use beforeEach etc properly so that tests work correctly even if .only() is used.

Stray .only() that are merged into master can be prevented quite easily with a lint rule that fails CI. If you use CodeCov, a coverage drop of 99% on a PR will also stand out and can be configured to fail the PR status.

At first I liked the idea of detecting CI (all CI providers I know set $env:CI='true'), but I can imagine someone debugging a test in CI by pushing a branch with a -Focus on purpose, and it would be surprising to see it magically not working.

nohwnd commented 4 years ago

@felixfbecker Just for clarification, Add-Dependency is gone, and replaced by BeforeAll to keep the keywords list shorter. You can simply dot-source in the BeforeAll in most cases. So I think the current state is the same as in mocha, you need to use Before* block correctly to avoid running the code side-effects.

(Add-Dependency might come back later, but for a different reason. Like importing modules and injecting their classes into Pester to allow Pester mocking commmands that use those classes as params. Or something like that that is difficult to do and remember. )