Fix for #1119 in single column tables in Gherkin feature files#1127
Conversation
|
Test output before applying fix: Test output after applying fix: |
|
Gah, the curse of PowerShell 2.0 strikes again... This time it's failing on code I don't think I've touched. Is it possible I've added test coverage to some code that was always broken for PowerShell 2.0 but never exercised in the tests before? |
fourpastmidnight
left a comment
There was a problem hiding this comment.
Just some observations about how The Cucumber Book documents how single-columned tables behave when attached to a step. I'm fairly new to Pester, so, I'm just drawing off of what I know of Cucumber and SpecFlow.
| $Parameters.Length | Should -Be 0; | ||
|
|
||
| # there must be an easier way to compare an array of hashtables? | ||
| $expectedTable = @( |
There was a problem hiding this comment.
And then, based on my previous comment above, the expected data should be:
$expectedArray = @(
"Property1",
"Property2",
"Property3"
);Note I'm not sure if we'd still want to call the above expectedTable versus expectedArray.
Or, it might make it easier to do the following (though, this looks weird to me):
$expectedTable = @(
@{ "Property1" = "Property1" },
@{ "Property2" = "Property2" },
@{ "Property3" = "Property3" }
};The biggest problem with the above is that using an array, we could potentially have a single-column table like:
Given the following fruit pass under the inspection machine on the conveyor belt
| Apple |
| Banana |
| Grape |
| Orange |
| Apple | # <-- This would throw a duplicate key exception, too, in the tabular formSo, for single-columned tables, it would be better to treat the table as a list/array.
There was a problem hiding this comment.
As I said on the other post, this needs to be done based on the parameter type on the step definition, and not changed in this story.
| $featureFile = Join-Path -Path $testDrive -ChildPath "singlecolumn.feature" | ||
|
|
||
| # write the temporary feature file that we're going to parse | ||
| Set-Content -Path $featureFile -Value @' |
There was a problem hiding this comment.
Shouldn't the result not be a table, but a list, when it's a single-column table, per The Cucumber Book on page 66:
Or just to specify a list:
Then my shopping list should contain: | Onions | | Potatoes | | Sausages | | Apples | | Relish |
Therefore, this should look like:
Feature: Gherkin integration test
Scenario: The test data should be converted properly
Given the test data
| Property1 |
| Property2 |
| Property3 |
`@;There was a problem hiding this comment.
The problem here is that the Gherkin syntax doesn't have lists -- it has tables.
Some test frameworks (obviously including Cucumber) have implemented a conversion based on the parameter type (see docs here).
We have not done any of that. We could. But it's outside the scope of this story.
There was a problem hiding this comment.
I agree that it's outside of the scope of this PR. But I'm curious, are you opposed, in principle, to the idea? (Before I saw this comment here, I reopened another issue I'd previously closed which precisely addresses this aspect.)
| @@ -830,7 +830,7 @@ function ConvertTo-HashTableArray { | |||
| if (!${Column Names}) { | |||
| & $SafeCommands["Write-Verbose"] "Reading Names from Header" | |||
There was a problem hiding this comment.
So, based on my comment about whether single-column tables should be returned as a list (or an array) per The Cucumber Book, couldn't we put a check in here? (NOTE: I'm not super-familiar with the Gherkin.Ast.TableRow object, so take the below as pseudo-code.)
# This code should be put in Get-StepParameters, starting @ L779 ff. -- GitHub wouldn't let me add a comment that far removed from the code change shown here:
if ($Step.Argument -is [Gherkin.Ast.DataTable]) {
if ($Step.Argument.Rows[0].Cells.Length -gt 1) {
$NamedArguments.Table = $Step.Argument.Rows | ConvertTo-HashTableArray
} else {
# How would we represent the array back to Pester if it's expecting a table?
# When we have a multi-columned table associated with a step, the parameter is
# assumed to have the name Table.
#
# According to the documentation over at [Cucumber](https://github.com/cucumber/cucumber/tree/master/datatable), it would appear
# that they would return a variable of type List<List<string>>.
#
# SpecFlow for .Net has smarts built-in to simply return a List<string>.
# So, perhaps it's simple enough to do:
& $SafeCommands["Write-Verbose"] "Processing single-columned DataTable"
$NamedArguments.Table = @() # <-- Hmm, should this still be called Table?
foreach (${Table Row} in $Step.Argument.Rows) {
# Powershell's about_Arrays documentation suggests this can be slow for large amounts of data
# Are we worried about that here?
$NamedArguments.Table += ${Table Row}.Cells[0]
}
}
}There was a problem hiding this comment.
In addition, in Gherkin.ps1, L409, could it be changed to avoid the error you're experiencing with the unit tests? To something like:
if ($Scenario.Contains("Examples")) {Hmm, and if this test is failing, doesn't his imply that running Gherkin-style tests with scenarios containing no Examples wouldn't work on PowerShell 2.0 to begin with?
|
@mikeclayton Yeah, looking at how the test is structured, and the code for Would this work: if ($Scenario.Contains("Examples")) {
# Rest of code here....
}I'll attempt to modify my review to add that as a comment there. |
|
I think it's probably beyond the scope of this PR / issue to redesign the Gherkin integration. All I was really aiming to fix here is the issue where the characters in a single column table get treated as multiple columns, which I think we can assume to be a bug. To be honest, my knowledge of Gherkin is pretty much limited to what I found out while looking at this issue, so I'm probably not the best person to comment on the design of the integration features :-) You might want to raise some of your feedback as separate issues if you think they'd improve the Gherkin functionality in Pester. M |
|
@nohwnd - could you give some pointers on how to run Pester's tests in PowerShell 2.0 locally so I can try to reproduce the build error at https://build.powershell.org/viewLog.html?buildId=16832&tab=buildResultsDiv&buildTypeId=Pester_TestPesterOnPowerShellV2? I've tried installing a Windows Server 2008 R2 VM in Hyper-V on Windows 10 but I'm getting some odd errors which I think are because I'm doing it wrong :-( |
|
@mikeclayton I am running them on a Vista virtual machine. Could you try doing that? |
|
@nohwnd - Cheers - I'll give that a go. As an aside, do you know what the agent spec is for the "psv21" agent on "build.powershell.org"? |
|
FWIW, I’ve had tests fail, but upon amending the most recent commit to update the commit hash and force-pushing to force a build, a subsequent build passed. Not sure why this has happened, but it's happened to me twice.
Oh, and always on the TC builds for various versions of PS.
…Sent from my Windows 10 phone
From: Michael Clayton
Sent: Tuesday, November 6, 2018 16:18
To: pester/Pester
Cc: Craig E. Shea; Mention
Subject: Re: [pester/Pester] Fix for #1119 in single column tables in Gherkinfeature files (#1127)
@nohwnd - Cheers - I'll give that a go.
As an aside, do you know what the agent spec is for the "psv21" agent on "build.powershell.org"?
https://build.powershell.org/viewLog.html?buildId=16832&tab=buildResultsDiv&buildTypeId=Pester_TestPesterOnPowerShellV2?
[01:57:17] : Starting the build on the agent psv21
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Ok, I've got a Vista VM running with PowerShell 2.0 (installed separately, and it also needed .Net Framework 3.5 otherwise the Gherkin "legacy" dll fails to load). I'm getting a slightly different error now though when I run the pester tests: which I think is because of this bit here when https://github.com/pester/Pester/blob/master/Functions/Gherkin.ps1#L837-L843 foreach (${InputObject row} in ${InputObject Rows}) {
${Pester Result} = @{}
for ($n = 0; $n -lt ${Column Names}.Length; $n++) {
${Pester Result}.Add(${Column Names}[$n], ${InputObject row}.Cells[$n].Value)
}
${Result Table} += @(${Pester Result})
}Basically, PowerShell 2.0 seems to have slightly different
PowerShell 2.0 PowerShell 5.1 Wrapping the entire Anyway, I'll update the PR with a fix for this as well and we'll see what the TeamCity build does... |
|
The TeamCity build's still failing, but I can't reproduce the error locally: |
|
P.S. - it'd be really handy if the TeamCity build script could call |
fourpastmidnight
left a comment
There was a problem hiding this comment.
Hopefully, this will get you "unstuck"...
| } | ||
|
|
||
|
|
||
| InModuleScope "Pester" { |
There was a problem hiding this comment.
I noticed two things with this test. The first thing is really simple to try, the second is a bit more work.
-
I noticed that all tests in this file are tagged with
-Tag Gherkinwhile this test is not. I'm not sure if there's anything "special" about this tag other than categorization (probably not), so this likely isn't it—but the test should still probably be tagged regardless, if only for consistency. -
I'm not sure why what I'm about to suggest would work any better than what you already have, but, all of the other Gherkin tests spawn a job into which they "re-load" the Pester module so that they don't "monkey with the active Pester state that's already running". It doesn't seem like your tests should affect the Pester state at all since
Import-GherkinFeature, while accepting a Pester State argument, doesn't even use it and you haven't even provided it.I do like how the "test data" (e.g. the feature file) you provided is inline with the test code, which is not a "mystery guest" like the other tests where this data is stored in external example files in the
..\Examplesdirectory. In fact, if what I suggest below works, I may end up using this style of "inline test data" for my future Pester Gherkin tests because I like it so much better! So, I think you could use a pattern very similar to theDescribe 'Invoke-Gherkin'test in this case:Describe "Get-StepParameters" { Context "Converts data in feature file steps" { $It "Should process a single-column table correctly" { # resolve the full name to the temporary feature file because gherkin doesn't support PSDrive paths $testDrive = (Get-PSDrive -Name "TestDrive").Root $featureFile = Join-Path -Path $testDrive -ChildPath "singlecolumn.feature" # write the temporary feature file that we're going to parse Set-Content -Path $featureFile -Value @' Feature: Gherkin integration test Scenario: The test data should be converted properly Given the test data | PropertyName | | Property1 | | Property2 | | Property3 | '@; # Calling this in a job so we don't monkey with the active pester state that's already running $job = Start-Job -ArgumentList $scriptRoot -ScriptBlock { param ($scriptRoot) Get-Module Pester | Remove-Module -Force Import-Module $scriptRoot\Pester.psd1 -Force $ActualDataTable = InModuleScope "Pester" { # parse the feature file to extract the scenario data $Feature, $Background, $Scenarios = Import-GherkinFeature -Path $featureFile $NamedArguments, $Parameters = Get-StepParameters -Step $Scenarios.Steps[0] -CommandName "the test data"; $NamedArguments.Table } $ActualDataTable } $actualTable = $job | Wait-Job | Receive-Job Remove-Job $job # there must be an easier way to compare an array of hashtables? $expectedTable = @( @{ "PropertyName" = "Property1" }, @{ "PropertyName" = "Property2" }, @{ "PropertyName" = "Property3" } ); $actualTable.Length | Should -Be $expectedTable.Length; for( $i = 0; $i -lt $expectedTable.Length; $i++ ) { $expectedTable[$i].Keys.Length | Should -Be $actualTable[$i].Keys.Length; foreach( $key in $expectedTable[$i].Keys ) { $key | Should -BeIn $actualTable[$i].Keys; $actualTable[$i][$key] | Should -Be $expectedTable[$i][$key]; } } } # Rinse, dry, repeat for multi-column table example } }
Perhaps this would get you "unstuck"?
|
Yeah, adding the .Count and .Length to everything so that you can pretend things are arrays is a newer thing in PowerShell. To maintain PS2 compatibility (which I still think is a waste of time) you need to wrap things that might not be arrays in @($null).Count # is one
$Null.Count # is zero |
|
I'm still struggling to reproduce the errors during the TeamCity build. I've added some temporary debug logging to the For reference, it's running on Microsoft Windows Server 2008 R2 Datacenter with Service Pack 1 I'll try to spin up a Datacenter edition VM tonight just to see if that makes any difference, This is becoming a bit of a rabbit hole given that the underlying change is just 3 bytes of text to wrap a calculated value in |
|
So it looks like Gherkin.dll has two classes: which is inherited by: Pester iterates over instances of both classes, but only "ScenarioOutline" instances have an "Examples" property, and Pester checks for this by doing: Line 409 in 5a53c46 For some reason (maybe differences in Strict Mode behaviour?), everywhere I run it locally (different operating systems, different versions of powershell) are all fine with that and evaluate $Scenario.Examples without problems, but if the TeamCity build server tries that against a I've hit a bit of a wall trying to reproduce the issue, but I could replace the if( $Scenario -is [Gherkin.Ast.ScenarioOutline] ) {Thoughts welcome... |
|
In all of my PRs, I have not had a problem building on the TC servers save for occassional transient test failures (which is annoying, and I'd love to pin down why those occur). But, considering the issues you are having, I like your approach. I especially like it because it's more "intention revealing" that we're actually processing a Scenario Outline as opposed to a simple Scenario. |
|
I should've kept my mouth shut--now I'm having similar issues. 😜 |
|
Oh, for goodness sake :-( and There's definitely some kind of Strict Mode kicking in on the build agent which doesn't take effect when I run it locally. |
|
Another error: This one's a legitimate error in the new tests I added - hashtable "Keys" have a "Count", not a "Length". It works with Strict Mode off because Incidentally, I've managed to reproduce some of these "build-server only" issues locally by temporarily adding I think it's also possible that the new tests I've added are the first bits of test coverage for some of the bits that have been throwing exceptions. I'm optimistic the most recent commit will get a green light in TeamCity. Fingers crossed... |
|
Ok, the PowerShell 2.0 build is green (finally)! I can't help but think the PowerShell 2.0 issues I've hit were there all along, just waiting for someone to add some test coverage to unearth them :-S. In any case, everything's green now so I think this PR is ready for review (once the rest of the build finishes)... |
Yep, we deliberately test with strict mode on, after getting multiple bug reports from people who were running it that way. :) |
|
@dlwyatt - ah, that would explain it then! Is there any chance you could publish the full script you run on the build agent? All I was doing locally is this: but that's clearly not enough to trigger the same issues as on the build agent. |
|
@mikeclayton Nothing special is happening on the server. When I test the release locally I run On the server the build steps look like this (the strict option means fail on skipped tests, not run in powershell strict mode): |
|
@nohwnd - Ah, cool. Based on that, I can reproduce the errors locally now... One thing to note is that the pester.bat does an Next time I get different results locally vs TeamCity I think I'll fall back to run pester.bat or the build script you've posted above, and see if I can reproduce it that way! In any case, I think this PR is ready for review now, so any comments welcome... Cheers, M |
|
This is becoming a very painful bug. I'm constantly needing to make sure my "single-column" table header names don't have duplicate letters within the name--e.g. |
|
@fourpastmidnight merged it. I have been away for a while, sorry. I will make a new release in the evening or over the weekend. Is there anything else that can be merged? I am not using Gherkin for any of my code, so I cannot validate your changes, apart from making sure the build passes and no huge changes are introduced to the rspec part of Pester. Please just agree with Jaykul on the changes, and tell me that it's ready to merge and I will include it in the next release :) |
|
@nohwnd Thank you!! I hope wherever you were, you were having a good time. I wasn't trying to be pushy, but this was rather painful. 😄 I'm just really happy to see this merged, as I now don't have to use a private, modified copy of this module to work around this issue. There are a few other PRs that are open that I would like to have merged: #1150 and #1138. I would like #1142 merged, but I have an open question to you on that one, and if anyone else would like to provide further input, that is always welcome (thinking of @Jaykul). Again, thank you nohwnd. |
Added some tests to reproduce issue #1119, and then applied a fix.
The tests are a bit long and awkward because they have to do a lot of work to arrange the test (write a temporary feature file, use Gherkin to parse it, then call the function under test), and I also couldn't find an easy way to compare two arrays of hashtables so the test has to walk the structure piecemeal. Any thoughts welcome about how to improve this.
Cheers,
Mike