Many gherkin runner fixes and improvements

[fourpastmidnight]

1. General summary of the pull request

This is a replacement PR for #1142 which will fix #1124. This PR simplifies a lot of the code on the Gherkin side, while also improving the console output during the test run so that the output closely resembles the output of a Ruby Cucumber test run. These improvements include:

• A change to the theme to resemble the default Ruby Cucumber theme
• Adopts the output format of the Ruby Cucumber pretty-print formatter
  • The margin is still 2 spaces
  • All lines to be output (such as descriptions for features, scenarios, examples, etc.) are appropriately indented.
• Improvements to Step output
  • Steps have their arguments highlighted.
    • I often found myself using quotes to denote where my arguments to my steps were because there was no indication in the console output. This feature, adopted from Ruby Cucumber, eliminates the need to do that.
  • DocStrings and DataTables are now printed to the console. You can turn this off by specifying -NoMultiline.
  • Failed, Undefined, and Pending steps all have a consistent, one-line "error" message and one or two-line stack trace showing the step-definition and/or feature file where the error originated.
    • Previously, changes made to Pester Gherkin often resulted in the problem matcher for Pester becoming broken, especially for Gherkin-style tests. This change does mean that the existing Pester problem matcher will not work for Gherkin-style tests. However, the benefit of this change is that for Gherkin tests, there is now a consistent way of displaying errors in the console, and a problem matcher can be made for each of the Failed, Pending, or Undefined step statuses in the Visual Studio Code Problems pane. See below for the JSON for the new VS Code problem matchers for Pester Gherkin.
• First-class support for Scenario Outlines
  • Scenario Outlines are now printed to the console, including their steps with their token arguments.
  • Scenario Outline Example Scenario names are now generated based off of the scenario outline example tables.
  • Scenario Outline Example Scenarios can be displayed as they are in the feature file, in tabular form, or, using the new -Expand parameter to Invoke-Gherkin, in expanded form, where they are displayed as any other scenario (though, output indented from and below their parent Scenario Outline).
  • Scenario Outline Example Scenarios are also displayed as nested test-suites as children of the scenario outline in the NUnit XML.
  • When scenario outline example scenarios fail, and -Expand is not used, that example scenario will have its stack trace displayed in the console. (NOTE: this only occurs for failing example scenarios, and not pending or undefined ones. This also means that, when not using -Expand, pending and undefined "warnings" will not be displayed in the VS Code problems pane. I'm open to changing this. Ruby Cucumber doesn't display these--BUT, there probably is value to be had by displaying these, especially for VS Code development.)

As mentioned above, if you develop in Visual Studio Code and want to use the Problems Pane for errors and warnings in your test code, this PR introduces new consistent Failing, Pending and Undefined step error/warning output. You can use the following JSON to define Pester Gherkin problem matchers for these step statuses:

"problemMatcher": [
    {
      "owner": "powershell",
      "fileLocation": ["absolute"],
      "severity": "error",
      "pattern": [
        {
          "regexp": "^\\s*Error:\\s+(.*)$",
          "message": 1
        },
       {
          "regexp": "^\\s+at\\s+[^,]+,\\s*(.*?):line\\s+(\\d+)$",
          "file": 1,
          "line": 2
        }
      ]
    },
    {
      "owner": "powershell",
      "fileLocation": ["absolute"],
      "severity": "warning",
      "pattern": [
        {
          "regexp": "^\\s*(TODO: \\(Pester::Pending\\))$",
          "message": 1
        },
        {
          "regexp": "^\\s*at\\s+[^,]+,\\s*(.*?):line\\s+(\\d+)$",
          "file": 1,
          "line": 2
        }
      ]
    },
    {
      "owner": "powershell",
      "fileLocation": ["absolute"],
      "severity": "warning",
      "pattern": [
        {
          "regexp": "^\\s*>>> (No matching step definition found.)$",
          "message": 1
        },
        {
          "regexp": "^\\s*at\\s+(.*?):line\\s+(\\d+)$",
          "file": 1,
          "line": 2
        }
      ]
    }
  ]

[renehernandez]

@fourpastmidnight @nohwnd Is the file .vscode/launch.json expected to be added to the git repo?

[fourpastmidnight]

Well, I’m finding it very helpful while working on Pester, allowing me to debug at the click of a button as I improve the Gherkin side of Pester. So, that’s currently my intention. If it is seemed this is not wanted in the repo, the file could be dropped from the commit that added it. But I firmly believe this is an improvement to help others who may wish to help with Pester be able to be up and running with debug capabilities right after cloning the repo.

…

Sent from my Windows 10 phone From: Rene Hernandez Sent: Tuesday, April 2, 2019 6:22 To: pester/Pester Cc: Craig E. Shea; Mention Subject: Re: [pester/Pester] Many gherkin runner fixes and improvements(#1276) @fourpastmidnight @nohwnd Is the file .vscode/launch.json expected to be added to the git repo? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[nohwnd]

IDK, it does not look like it harm anything. I am not so sure about the extended formatting options though. But right now I have the formatting off and the psscript analyzer off as well because it took 30s to save a single file... :/

[fourpastmidnight]

Apparently, I lost the PSObject wrapper somewhere along the way again... 😐

[fourpastmidnight]

Just pushed up the last of the changes I planned to make, that is until I found #1285. So before I wrap up this PR, I would like to fix that issue since I'm in here and have made changes to the unit tests in that area.

[nohwnd]

Okay. I see that most of the changes are in gherkin related files only, so merging this should be no problem.

[fourpastmidnight]

Well, all the tests are passing, but I decided to take what I have so far and actually use it with tests I wrote for 3 modules I recently developed. And there are things that are wrong with this that I need to fix:

• Multiline arguments were not indented correctly
  • Fixed, but not yet pushed. I was using the old PesterState.IndentationLevel indent instead of the new one for Gherkin: GherkinIndentationLevel. Updated all the step printing code.
• For some reason, when running the Gherkin-Background.feature in the Pester test suite, everything looks ok. But, when running against actual code, the background steps don't appear to be worknig correctly. They're printed at the start of the feature, but then the steps are run "out of order" with the scenario (e.g. they execute after the scenario), leading to test failures. AND, they're printed again (after the scenario!?!). What's up with that? And why aren't the Pester tests seeing this behavior?
  • [UPDATE]: Everything's fine. What the Pester tests don't check is two scenarios in a feature file with a background, because prior to this change, there really wasn't a need to. What's happening is Invoke-GherkinStep invokes a step and immediately prints the result to the console. So, for the first scenario, the background information is displayed and the step result is output. Then the first scenario information is displayed, followed by its step results output. Then, when the next scenario is executed, the background metadata is not displayed, and there's no intervening blank line output to the console, so when the background steps are executed, their results are displayed to the console and appears to be part of the first scenario, when it's really part of the second. So, execution is OK, but the display is not. It's just a display bug.
  • [UPDATE]: Fixed, but not pushed yet.
• I have one failing unit test in one of my modules (due to the Background issue I just mentioned). The failed scenarios count is 95 even though only one scenario failed. The steps count is correct. Weird.
  • [UPDATE]: Even though I tried several things to force the PassedScenarios, FailedScenarios, etc. properties on the PesterState object to be string arrays, it didn't really work (even using the , operator). Anyway, the invalid count was the "length" of the single, failed scenario's name, and this was a result of using the Length property. So instead, use the ETS Count property.
• In the new function New-GherkinPesterState, I made the PesterOption parameter a [hashtable], but it is in fact supposed to be a [PSCustomObject].
  • Fixed, but not yet pushed.

One thing that I'm not a huge fan of is that because Write-Host does not allow you to redirect the output, there's no way to verify the console output other than "looking at it" and hoping you've done enough manual testing with enough variation to catch everything. I have a thought that there should be some way to "redirect" output to somewhere for testing/reporting purposes, while still "forwarding" the output to Write-Host to colorize the output. For example, I would be interested in capturing the test run console output to a file (instead of using NUnit Xml and relying on non-existent/not really nice NUNitXml -> HTML converters). Of course, this issue goes away with PowerShell 5.1 (I think--if not 5.1, then 6+). But in the meantime, as long as Pester needs to support PSv2 (and, even in Pester v5 we're supporting PSv3+), we're stuck. I'd love to use something akin to Tee-Object, or create some sort of proxy for Write-Host that would allow this in a way that's more flexible than the straight usage of Tee-Object.

[fourpastmidnight]

And one more issue:

I was trying to keep the console output clean, regular and consistent. This meant removing extraneous blank lines from description text. The only problem is, if the line was blank, I don't print it. This means that when you have longer descriptions, you end up having a "wall of text". Not ideal, IMO. So, I need to figure out what to do about this.

[fourpastmidnight]

OK, I did some validation of behavior in Ruby Cucumber. While Ruby does properly indent the Feature names, Scenario names, etc., it does not "reformat" descriptions--which makes perfect sense. So, I'll be pushing an update that removes the blank line suppression and line-trimming that I was doing in the various Write-* functions when descriptions were written to the console.

[fourpastmidnight]

OK, since #1475 was merged to master (thanks @nohwnd!), I once again rebased this on master. This PR is finally ready to be merged. If this is merged, it should result in a minor version bump of Pester.

[nohwnd]

Nice! I guess no-one else was able to validate your changes?

@Jaykul this is about to be merged, could you run it against your testsuites? So I don't break you when this is released?

[Jaykul]

I can. One moment ...

[Jaykul]

Looks good.

I don't quite like what happened with the colors, but I can live with it. I mean, I like the highlights, but even after I read your notes, I wish we hadn't lost all the color from the Feature and Scenario output.

This version:

Before:

[nohwnd]

@Jaykul do you still use gherkin a lot? Right now Pester v5 tries to be accomodating to be layered with another freamwork "face", and while it was useful to think about the internals as being modular to keep the internal architecture clean, it brings a lot of overhead, and unnecessary renaming, just to support <1% of users.

I am thinking about removing it entirely from v5, and splitting it from v4 to it's own repo under pester organization where hopefully @fourpastmidnight and you could rule and improve it as you see fit? Either forking v5, or maybe I could still keep the "runtime" part distinct from RSPec Pester and rename the items during build and not during runtime. And you'd do the same to build Gherkin Pester. It would then ship in a nuget under a different name, say PesterGherkin.

[nohwnd]

If you know other people that should be included in this conversation please tag them, I can move it to a new thread later if needed.

[fourpastmidnight]

Ok, wow, there's a lot to respond to here. In this comment, I'm going to address the things raised about the coloring.

@Jaykul:

Yes, the highlighting is showing what text in your steps matched the regexes in your step definition. This comes from Ruby Cucumber. It's super useful. In fact, I caught a bug in my own test suite the other day because of this highlighting (the regex was capturing more than I intended). It can save countless minutes of debugging by being able to visually see what step text matched a step definition regex.

To prevent a regex match from being highlighted, you can use a non-capturing regex: (?n:match-some-stuff-here). In fact, I have come to believe (and, this is my opinion, I realize) that it may be a best practice to write step definitions like this in the majority of cases (>95%):

Given '(?n)^This is some (?<Parameter>parameterized) text to be matched$' {
    Param([string]$Parameter)

    Set-StepPending
}

EDIT
I realize, however, that as far as text that is to be matched for the purposes of providing a value for a step definition parameter MUST be a capturing regex. The comment above was merely for the case where you may have regexes in steps to allow for easier "English-language" reading of the step rather than for parameter value purposes.

I have been considering whether or not case-insensitivity would also be a best-practice (as a starting point) since it can be turned back on for individual capture groups, if needed. In any event, I find this leads to consistent matching step definitions.

As for the Feature and Scenario colors, yes, the color was changed to match the default Ruby Cucumber coloring.

There are many implementations/ports of Ruby Cucumber (see Related Tools here). Pester is not listed among them. I believe with this PR in place, we could actually get listed here. Anyway, back to coloring. All the coloring has been consolidated to en-US\Gherkin.psd1. One thing I would love to enable in the future is for someone to be able to use their own coloring options by providing a similar data structure to Invoke-Gherkin (either directly through a parameter or via a Gherkin extension to the PesterOptions object).

Also, I think you mentioned this before—that you don't necessarily want to see the DocStrings/DataTables printed to the console? This can be achieved by passing the -NoMultiline switch to Invoke-Gherkin.

[fourpastmidnight]

@Jaykul BTW, I'm not opposed. I could put the Cyan color back in for the Feature/Scenario title.

[fourpastmidnight]

OH--and I missed the part where you said you liked the highlighting. Sorry. 😄 I originally read this at 1 AM and didn't see that bold, highlighted like.....gee--wonder how I missed that...

[fourpastmidnight]

@Jaykul

I can't seem to find where you commented on the label. BUT, I went back to look at the history of those lines. I renamed Gherkin.ps1 to Gherkin.Core.ps1, so the implementation of Invoke-GherkinHook, where you found the issue has not changed since the file rename. So this is a latent bug in the Gherkin runner that hasn't been found until now!

I'll add a commit to fix that. I'm thinking all we need to change is line 44, from break :tags to break tags. I don't use hooks often (and I'm guessing many people may not), so I've never run into an issue due to this. Thanks for catching that!

Also, I remember the reason why light cyan is not part of the scenario/feature titles: here's an example of my test run where I use scenario outlines and data tables:

(BTW, this is where argument transformation is super useful. The last column in the scenario examples table where you have something like 3 variable resources. When the scenario is printed in expanded form, it helps the scenario read like English. But obviously, I'm only interested in the 3. An argument transform would transform this int 3 and set an integer parameter to that value. In this example, I started thinking about how that might work in Pester Gherkin. I haven't gone much further than this one usage at this time, but it's on my mind still!)

[fourpastmidnight]

@nohwnd Re: future of Gherkin in Pester.

I have mixed feelings about this. I've actually wanted to approach you and ask for your thoughts on this very subject. Let's start a new issue to pick up this conversation. I feel like it's one worth having if only to "clear the air" and properly set the stage for not only the future of Gherkin, but possibly even Pester itself. 😄 I think some of what you mentioned makes a lot of sense. But like I said, I do have mixed feelings. So let's do a Cost-Benefit analysis in another thread. (I don't want to speak for @renehernandez, but he did comment on this PR; so perhaps he should be an initial participant, too?)

[fourpastmidnight]

As an FYI, I found a cosmetic bug: the highlighting isn't always highlighting what should be highlighted under all circumstances. And it's not improperly highlighting things that aren't matched—it simply isn't necessarily highlighting all things that have matched.

Again, it's more cosmetic than anything else. Regex is Regex, so whatever is matching is matching and properly setting parameter values. I don't believe that what I found should hold up merging of this PR since it is only cosmetic.

IF this PR is merged before I implement a fix, then I'll just open a new Issue and PR for the fix. If not, then I'll add it to this PR—but I don't want to hold up this PR any longer for a cosmetic blemish. 😉

[fourpastmidnight]

Well I'll be.... Something in my tests caused the highlighting to not work properly, because all of a sudden, it is. Weird. And slightly disturbing. I'll keep an eye on this and see if there isn't something to be done to make it more robust. At least it's only cosmetic and has nothing to do with actual test execution itself.

UPDATE
Part of me wonders if the issue wasn't part of the VS Code console. I notice that when VS Code's been running a while, and you've been writing quite a few debug sessions and/or tasks that things get bogged down. And later on today, VS code when completely black for a few minutes—and then the terminal was all messed up forcing me to close my terminal sessions. Then everything worked again.

[fourpastmidnight]

@Jaykul Just checking back in. So, if we used the bright cyan color for Scenario and Feature titles, that shouldn't clash too much with the surrounding output. I'm not opposed to making that change if it would help make those stand out better. I suppose the authors of Ruby Cucumber thought that indentation helped to dilineate those items—but I can see it both ways.

(FWIW, if I change the coloring, I would do it for all "headings": features, scenarios, backgrounds, and examples titles. I'm inclined to leave the descriptions as gray text, however, since yellow is used to denote undefined or pending scenarios. At some point in the future, for undefined scenarios, I would love to be able to print out text in the console that you could copy/paste into a PS script file to start off your step definition, much as Ruby Cucumber does.)

Apart from that, do you feel this is ready to go?

[Jaykul]

I'll admit I haven't given it a thorough re-code-review recently, but it worked for the test cases I have, so I'm happy with it 😁

[fourpastmidnight]

OK, I sort of figured out why highlighting sometimes "doesn't work" The short answer is, it IS working. Only it doesn't look like it.

It's been over a year since I wrote this code, so I don't quite remember how it works—and the 1-year later me doesn't quite grok it anymore (which means, I probably need to either refactor the code so I can understand it better, or barring that, put in some comments 😮 ). Anyway, the slightly longer answer is, if there's a problem with the test, then highlighting might not be accurate.

This is more cosmetic than anything. BUT, I should probably revisit that code. It should be possible to make highlighting always work, and work correctly, even if the test is failing.

I still think that this slight issue shouldn't hold up merging this PR. This is something that can wait to be fixed at a later time. It's more cosmetic than anything. And if you've got a failing test, you're either going to fix the failing steps and/or debug the test to determine what's wrong to fix it. And once you fix it, everything will "just work".

[fourpastmidnight]

@nohwnd Since JayKul indicates he likes the results of this PR and all his test cases are passing, and my own test cases have been using this PR for about a year now, perhaps it's time to get this merged? 😄

I have bug #1285 to fix, as well as a few other enhancements for the Gherkin runner all hinging on this PR being merged.

[nohwnd]

Sorry that I did not merge it, and since there were no pokes about this for over a year, do you still need this merged? I'd like to avoid breaking the current stable v4 release.

[fourpastmidnight]

@nohwnd Well, I poked and poked and poked about this over the last 2 years, but you never merged it pending "v5 breaking changes". Given that Pester v5 removes Gherkin, and given that there are no breaking changes in this PR, I'm still in favor of merging. Also, @Jaykul in his comments above seemed to be OK with these changes; and I've been regularly using "this version" of Pester for over 2 years.

Looking back over the commit history, I don't believe there are any major breaking changes. One minor change is that steps with no step definition are marked as Undefined and still cause a test-run failure. Prior to this, steps with no step definition were simply marked as failed. The behavior is mostly the same--i.e. test run is marked as failed, but the reporting details are updated to show a distinct Undefined state. Also there's an added Pending state that can be explicitly set by test implementors, so this is an addition (not a breaking change) that users can opt-in to. Again, all-in-all, I see this is a worthy feature to merge into v4.

[nohwnd]

Ok, will try to merge and publish :)

[fMichaleczek]

@nohwnd @fourpastmidnight thank you both, I will wait and test the new package 🙂

[nohwnd]

@fMichaleczek pushed as 4.10.2-beta1

[fMichaleczek]

I will test it before the end of the week end.

[fourpastmidnight]

@nohwnd Thanks for merging this!! @fMichaleczek I look forward to any feedback. Once this review has passed and becomes a non-prerelease version in the 4.x branch, I look forward to updating my fork to the latest 4.x line and seeing where to go from here with the Gherkin "port" of Pester. Thanks @nohwnd for your help with this. Thanks @Jaykul and @renehernandez for feedback over the time I was working on this.

[fMichaleczek]

@fourpastmidnight I'm a newbie in Gherkin but I like the concept. I will try to see how I can use it.