-
Notifications
You must be signed in to change notification settings - Fork 164
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.
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
Only the table of the examples in a scenario outline should have test results #515
Conversation
* Pickles is unable to deal with Danish characters (picklesdoc#477) * Version 2.16.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution. I have some ideas on how to improve the code, please take a look at them.
@@ -69,7 +70,7 @@ public void ThenTheResultShouldBe(string expectedResult) | |||
|
|||
actualResult = Regex.Replace(actualResult, @"\s+", string.Empty); | |||
expectedResult = Regex.Replace(expectedResult, @"\s+", string.Empty); | |||
|
|||
Debug.Print(actualResult); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like debug statements left after the debug session :-)
@@ -19,6 +19,7 @@ | |||
// -------------------------------------------------------------------------------------------------------------------- | |||
|
|||
using System; | |||
using System.Diagnostics; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We won't need this one anymore after you remove the Debug statement
@@ -27,8 +27,11 @@ public class JsonTableRow : List<object> | |||
public JsonTableRow(IEnumerable<string> cells, JsonTestResult result) | |||
{ | |||
AddRange(cells); | |||
this.Result = result; | |||
Add(result); | |||
if (result != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a null check?
@@ -45,6 +45,22 @@ public List<string> Cells | |||
get { return this.tableCells; } | |||
} | |||
|
|||
} | |||
|
|||
public class TestTableRow : TableRow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestTableRow
sounds like it's a table for testing purposes. I prefer a name like "TableRowWithTestResult" or something like that.
@@ -46,33 +46,46 @@ public string MapToString(G.TableCell cell) | |||
return cell?.Value; | |||
} | |||
|
|||
public TableRow MapToTableRow(G.TableRow tableRow) | |||
public TableRow MapToTableRow(G.TableRow tableRow, bool isForExample=false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The isForExample parameter is a code smell to me. You're steering the behavior of the class depending on this parameter. Is would prefer having a method MapToTableRow and another one MapToTableRowWithTestData and let the caller of the method decide which variant to use. The caller already makes that decision when they determine the value for isForExample, there is no need to make the same decision twice.
you are entirely right, i'm fixing and repushing. I was a bit lazy on this one |
Hi, Sorry to ask, but is there any progress on this issue ? Regards, |
I have scheduled some Pickles time today, I hope I'll be able to work through most of the pull requests. |
Released in version 2.18.1. |
Thanks! |
Hello,
To fix #514 I created an ExampleTable which have TestDataRows in the Example Object.
Only this kind of Table is expected to have Json containing results.
Today you have:
After you will get back to having
No change on Scenraio Ouline results
Hope I didn't miss any thing, all tests are ok.
Regards,