WIP: Fix #1124 pester does not print doc strings or data tables#1142
Conversation
fourpastmidnight
left a comment
There was a problem hiding this comment.
Looking for feedback on this PR. @Jaykul maybe you could take a look when you have some time.
| if ($Step.Argument -is [Gherkin.Ast.DocString]) { | ||
| $StepText += "$([Environment]::NewLine) `"`"`"$([Environment]::NewLine) $(foreach ($str in ($Step.Argument.Content -split [Environment]::NewLine)) { " $str$([Environment]::NewLine)" }) `"`"`"" | ||
| } | ||
| if ($Step.Argument -is [Gherkin.Ast.DataTable]) { |
There was a problem hiding this comment.
One possible improvement that could be made is to extract this functionality out into a private Function. The same could be said for where this function processes Scenario Outlines to create new Scenarios for each table row of data.
| Exit-MockScope | ||
| } | ||
|
|
||
| function Find-GherkinStep { |
There was a problem hiding this comment.
I meant to completely remove this--not just comment it out. I guess I should first solicit comments on the removal of this unused function--because perhaps it's there for a reason? Depending on the responses I get, I'll either drop this commit or rebase the actual removal of the function.
There was a problem hiding this comment.
This is meant for tooling --hypothetically-- your editor can call this when it sees you writing in a .feature file to show you intellisense, to auto-complete "when ..." sentences, or highlight missing steps, etc. It's basically the first step toward integrating with the PSES...
There was a problem hiding this comment.
OH!! Definitely useful in the future. Ok, I’ll drop that commit. Thanks!!
There was a problem hiding this comment.
Wow, I recently used Find-GherkinStep to find an existing step and execute it from within another step to help keep my test code dry. So this is definitely a useful cmdlet.
fdf5254 to
c903d03
Compare
|
|
||
| [Pester.OutputTypes]$Show = 'All', | ||
|
|
||
| [Switch]$HideStepData, |
There was a problem hiding this comment.
@Jaykul: Just wanted to check, based on my previous comment on #1124, do you think it's a good idea to definitely add the option to disable the displaying of DocStrings/DataTables in the console to the OutputType enum? I'd like to wrap this up.
@nohwnd Do you have any preference as to how this should be handled, especially given that RSpec style tests don't make use of DocStrings nor DataTables (to my knowledge—I did see something called implicit docstrings, but that also implied the use of an extension to RSpec, so I wouldn't think this applies to Pester)?
58b7048 to
9d5a0fa
Compare
6a04aa7 to
48fc7c3
Compare
48fc7c3 to
fe174f6
Compare
|
Well this is a bummer. I think printing the DocStrings and Tables to the console messes up the problem matcher. At least there's an option to turn off displaying these in this feature. This seems to be a common issue however: changes made to the output mess up the problem matcher. Perhaps whenever a step fails, immediately after the step text, there's some "sentinal" text, like |
fe174f6 to
ac0ba5e
Compare
ac0ba5e to
66d6c6e
Compare
4851e3c to
0a84607
Compare
|
OK, I think the problem matcher being broken is not a huge deal. Let me explain why. First: There's a way to turn off "pretty printing" of Tables and DocStrings. So, if you really want the problem matcher to work, just don't print Tables and DocStrings. Ideal? No. (NOTE: it might be nice to output the Tables and DocStrings to the NUnit XML report, even if pretty-printing of Tables and DocStrings to the console is disabled. Hmm, and this may require another option switch to Second: I think I could get more bang for my buck if I were to look into implementing a test adapter VS Code extension that can be used by the VS Code Test Explorer extension. Again, this could be an "opt-in" type thing in lieu of using the problem matcher. And again we could leverage the NUnit XML Report output. I believe that even though this option requires additional work, longer-term it could inoculate us from breaking the all-too-fragile problem matcher everytime console output changes. I also believe this would provide a much better user experience within VS Code for surfacing test failures and navigating to them from the Test Explorer hub. (As an aside, just now I thought how it's funny that Pester, a test framework which does not require VS Code to run, has code specifically to support VS Code. It would be nice to remove this accidental coupling/knowledge about the outside world from Pester.) Any thoughts, comments, concerns? |
0a84607 to
e6f8553
Compare
|
However, I did just manage to get the VS Code problem matcher to work, and I kind of like the results. I'll push up the necessary changes later, so please hold off on merging this until then. It did require a change to the problem matcher regular expressions, however. It appears that the problem matcher is acquired one of two ways:
Perhaps we should publish the example |
e6f8553 to
ca0015d
Compare
|
@fourpastmidnight are there more changes needed? I renamed it to |
|
@nohwnd: Yes, more changes are needed and forthcoming. I should’ve renamed this awhile back. Sorry about that. And, thanks for renaming.
…Sent from my Windows 10 phone
From: Jakub Jareš
Sent: Saturday, January 5, 2019 8:17
To: pester/Pester
Cc: Craig E. Shea; Mention
Subject: Re: [pester/Pester] WIP: Fix #1124 pester does not print doc stringsor data tables (#1142)
@fourpastmidnight are there more changes needed? I renamed it to WIP: so I stop re-reading it when I go through open PRs. When it's complete please remove the WIP and ideally tag me in comment. Thx
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
This commit appends DocStrings and DataTables to Step Definition text so that they are printed to the console during the test run. A new parameter to Invoke-Gherkin is also added: HideStepData, which when set will not print DocString and DataTable parameters to the console.
ca0015d to
ba42aec
Compare
|
OK, so now that a separate scenario is created for each row of an example table for a scenario outline, the implementation of this needs to change a great deal. BUT, that's OK. Because this has caused me to realize that we've mixed two unrelated concerns in Pester's Gherkin test implementation. The mixing of these two concerns was going to cause greater and greater friction at some point anyway. Adding in #1168 just caused this friction to happen sooner. But again, this is a good thing. It provides the opportunity to clean up the code and separate these concerns. The concerns in question are:
|
|
Hmm, reading The Cucumber Book on around pg. 70 where they delve more into Scenario Outlines and DataTables, So, it would seem after the implementation of #1168, though it's not printed correctly with that implementation, the effect is that In fact, based on how |
|
OK, looking at the implementation of Gherkin.dll, Gherkin does not make a distinction between Scenario Outlines and Scenarios. It parses a "ScenarioLine" and description and then some steps and then "Examples", which consist of an "ExamplesLine" (e.g. the keyword and name for the example table(s)), a description (optional) and one or more "ExamplesTable"s. This is interesting considering the output shown in the previously linked-to blog article. It's almost as if a Scenario Outline functions much like an RSpec Context would within a Describe (except that, there's no shared state being created--the "context" is merely acting as a container for all the scenarios created from the example table(s)). This is exactly how I was planning on modeling this in Pester for Gherkin style tests. It's good to know I was on the right track. |
|
Note to self: Rename |
|
I've come to the realization that the current implementation of Gherkin tests in Pester isn't conducive to running them as cucumber would run them. In fact, the current implementation leaves a lot to be desired in terms of performance and reporting. Now, I'm not "bashing" all the work that went into this--I think what has been done is absolutely phenomenal!, and my hat's off to all who have contributed to make possible what we have with Gherkin-style tests in Pester! But it seems to me that this organically grown implementation of Gherkin tests living within the Pester framework needs a different approach. How will this fit in with the RSpec style of tests Pester supports? I'm unsure. But I am working on overhauling the Gherkin implementation, with the focus on these key areas:
So, if you think I've been quiet lately, it's because I just finished reading Windows PowerShell in Action, 3rd Ed. about one week ago and have been taking my first stab at a new approach to Gherkin style tests in Pester. When I have something to show, I will submit a new PR and officially abandon this one. The new PR will be far from "Mergeable", more a WIP, but with the intent of garnering feedback. |
|
I think it would be better to wait for beta of v5 and do it there. We can then work together to do this, if you are able to describe to me what the changes should be, and I to you how the new internals work :D |
|
I was thinking this would be a v5 change. 😉 Maybe I could create a v5 “issue” item with the details where further discussions can be had.
…Sent from my Windows 10 phone
From: Jakub Jareš
Sent: Tuesday, February 12, 2019 22:30
To: pester/Pester
Cc: Craig E. Shea; Mention
Subject: Re: [pester/Pester] WIP: Fix #1124 pester does not print doc stringsor data tables (#1142)
I think it would be better to wait for beta of v5 and do it there. We can then work together to do this, if you are able to describe to me what the changes should be, and I to you how the new internals work :D
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@fourpastmidnight sure :) |
|
I've recently made significant progress on this for the 4.x branch of Pester. I'm still wrapping up some loose ends. I will warn however, that I have a lot of stuff coming up that will require some time/attention in the next three weeks, so it may be just a bit longer than I would like. But, wow. Significant progress here! |
|
@fourpastmidnight that's great :) I still did not have time to look into your code :/ Hopefully when Pester v5 beta is out. |
|
This is being superseded by #1276. |
1. General summary of the pull request
Fixes #1124.
For Gherkin Step Definitions which have DocString or DataTable data, this data is now printed to the console by default. A new switch parameter has been added to
Invoke-GherkincalledHideStepDatawhich controls whether or not DocString/DataTable data is printed to the console: when enabled, this data is not printed. The default is for the data to be printed.