Skip to content

Fix for #1119 in single column tables in Gherkin feature files #1127

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.

Sign up for GitHub

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

Merged
nohwnd merged 12 commits into from
Dec 6, 2018
Merged

Fix for #1119 in single column tables in Gherkin feature files #1127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ed820c8
Added Pester tests to catch issue #1119 in Gherkin integration
mikeclayton Oct 17, 2018
bd5ecb3
Added fix for issue #1119 in Gherkin integration
mikeclayton Oct 17, 2018
b0a8357
Fixed cut&paste error in Gherkin.Tests.ps1
mikeclayton Oct 17, 2018
8252e7c
Tweak to tests for issue #1119
mikeclayton Oct 17, 2018
611f0bc
Additional fix for #1119
mikeclayton Nov 7, 2018
5f9d86d
#1119 - Added some temporary debug output to diagnose build issue
mikeclayton Nov 7, 2018
ce0281e
#1119 - fix temporary debug logging to work on travis build
mikeclayton Nov 7, 2018
683a81f
#1119 - removed temporary debug logging
mikeclayton Nov 8, 2018
5e943ed
#1119 - fixed *another* PowerShell 2.0 glitch
mikeclayton Nov 8, 2018
061a0c3
#1119 - yet more PowerShell 2.0 / strict mode tweaks
mikeclayton Nov 8, 2018
dac3e66
#1119 - bugfix to new table-based gherkin tests (Length -> Count)
mikeclayton Nov 8, 2018
8ff9dd7
#1119 - added "gherkin" tag to new tests
mikeclayton Nov 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 115 additions & 0 deletions Functions/Gherkin.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,3 +120,118 @@ Describe "Mocking works in Gherkin" -Tag Gherkin {
@($gherkin.Results.PassedScenarios).Count | Should Be 3
}
}


InModuleScope "Pester" {
Copy link
Copy Markdown
Contributor

@fourpastmidnight fourpastmidnight Nov 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed two things with this test. The first thing is really simple to try, the second is a bit more work.

  1. I noticed that all tests in this file are tagged with -Tag Gherkin while 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.

  2. 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 ..\Examples directory. 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 the Describe '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"?


Describe "Get-StepParameters" -Tag Gherkin {

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 @'
Copy link
Copy Markdown
Contributor

@fourpastmidnight fourpastmidnight Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 |
`@;

Copy link
Copy Markdown
Contributor

@Jaykul Jaykul Nov 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@fourpastmidnight fourpastmidnight Nov 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Feature: Gherkin integration test
Scenario: The test data should be converted properly
Given the test data
| PropertyName |
| Property1 |
| Property2 |
| Property3 |
'@;

# parse the feature file to extract the scenario data
$Feature, $Background, $Scenarios = Import-GherkinFeature -Path $featureFile;
$Feature | Should -Not -Be $null;
$Background | Should -Be $null;
$Scenarios | Should -Not -Be $null;
$Scenarios.Steps.Count | Should -Be 1;

# call the function under test
$NamedArguments, $Parameters = Get-StepParameters -Step $Scenarios.Steps[0] -CommandName "the test data";
$NamedArguments | Should -Not -Be $null;
$NamedArguments.Table | Should -Not -Be $null;
@(, $Parameters) | Should -Not -Be $null;
$Parameters.Length | Should -Be 0;

# there must be an easier way to compare an array of hashtables?
$expectedTable = @(
Copy link
Copy Markdown
Contributor

@fourpastmidnight fourpastmidnight Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 form

So, for single-columned tables, it would be better to treat the table as a list/array.

Copy link
Copy Markdown
Contributor

@Jaykul Jaykul Nov 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@{ "PropertyName" = "Property1" },
@{ "PropertyName" = "Property2" },
@{ "PropertyName" = "Property3" }
);
$actualTable = $NamedArguments.Table;
$actualTable.Length | Should -Be $expectedTable.Length;
for( $i = 0; $i -lt $expectedTable.Length; $i++ )
{
$expectedTable[$i].Keys.Count | Should -Be $actualTable[$i].Keys.Count;
foreach( $key in $expectedTable[$i].Keys )
{
$key | Should -BeIn $actualTable[$i].Keys;
$actualTable[$i][$key] | Should -Be $expectedTable[$i][$key];
}
}

}

It "Should process a multi-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 "multicolumn.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
| Column1 | Column2 |
| Value1 | Value4 |
| Value2 | Value5 |
| Value3 | Value6 |
'@;

# parse the feature file to extract the scenario data
$Feature, $Background, $Scenarios = Import-GherkinFeature -Path $featureFile;
$Feature | Should -Not -Be $null;
$Background | Should -Be $null;
$Scenarios | Should -Not -Be $null;
$Scenarios.Steps.Count | Should -Be 1;

# call the function under test
$NamedArguments, $Parameters = Get-StepParameters -Step $Scenarios.Steps[0] -CommandName "the test data";
$NamedArguments | Should -Not -Be $null;
$NamedArguments.Table | Should -Not -Be $null;
@(, $Parameters) | Should -Not -Be $null;
$Parameters.Length | Should -Be 0;

# there must be an easier way to compare an array of hashtables?
$expectedTable = @(
@{ "Column1" = "Value1"; "Column2" = "Value4" },
@{ "Column1" = "Value2"; "Column2" = "Value5" },
@{ "Column1" = "Value3"; "Column2" = "Value6" }
);
$actualTable = $NamedArguments.Table;
$actualTable.Length | Should -Be $expectedTable.Length;
for( $i = 0; $i -lt $expectedTable.Length; $i++ )
{
$expectedTable[$i].Keys.Count | Should -Be $actualTable[$i].Keys.Count;
foreach( $key in $expectedTable[$i].Keys )
{
$key | Should -BeIn $actualTable[$i].Keys;
$actualTable[$i][$key] | Should -Be $expectedTable[$i][$key];
}
}

}

}

}

}
18 changes: 10 additions & 8 deletions Functions/Gherkin.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,7 @@ function Import-GherkinFeature {
}
}

if ($Scenario.Examples) {
if( $Scenario -is [Gherkin.Ast.ScenarioOutline] ) {
foreach ($ExampleSet in $Scenario.Examples) {
${Column Names} = @($ExampleSet.TableHeader.Cells | & $SafeCommands["Select-Object"] -ExpandProperty Value)
$NamesPattern = "<(?:" + (${Column Names} -join "|") + ")>"
Expand Down Expand Up @@ -830,16 +830,18 @@ function ConvertTo-HashTableArray {
if (!${Column Names}) {
& $SafeCommands["Write-Verbose"] "Reading Names from Header"
Copy link
Copy Markdown
Contributor

@fourpastmidnight fourpastmidnight Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]
        }
    }
}

Copy link
Copy Markdown
Contributor

@fourpastmidnight fourpastmidnight Oct 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

${InputObject Header}, ${InputObject Rows} = ${InputObject Rows}
${Column Names} = ${InputObject Header}.Cells | & $SafeCommands["Select-Object"] -ExpandProperty Value
${Column Names} = @(${InputObject Header}.Cells | & $SafeCommands["Select-Object"] -ExpandProperty Value)
}

& $SafeCommands["Write-Verbose"] "Processing $(${InputObject Rows}.Length) Rows"
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)
if( $null -ne ${InputObject Rows} ) {
& $SafeCommands["Write-Verbose"] "Processing $(${InputObject Rows}.Length) Rows"
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})
}
${Result Table} += @(${Pester Result})
}
}
end {
Expand Down