Skip to content
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

Scenario Outline special characters workaround #509

Merged
merged 8 commits into from
Jun 15, 2018

Conversation

DominikBaran
Copy link

@dirkrombauts
Copy link
Member

I may have mentioned in another pull request that my regex skills are rusty :-)

What problem does this pull request solve? Could you perhaps create a unit tests that shows the problem?

@dirkrombauts
Copy link
Member

Isn't this pull request very similar to #506 ? You closed #506, probably because you said "source tree did something really strange during the merge".

I stand by the comment I made in #506 : If there is a bug, I want a unit test that shows the bug is solved. If you need assistance in creating a good unit test, please let me know.

@DominikBaran
Copy link
Author

DominikBaran commented Feb 22, 2018

@dirkrombauts :

  1. Yes this change is an addition to Fix for the special characters in Scenario Outline. #506. In the previous PR I fixed only white spaces in the Scenario Outline names - what is good and works perfectly fine. Then I realized there is also issue with special characters
    ex: Scenario Outline: Support different users (admin/standard/new)

  2. I fixed special characters in the way:
    2.1 in the ScenarioOutlineExampleIsMatch method for the scenario outline with special characters
    exampleElement.name value is: support different users (admin/standard/new) - spaces and special characters
    exampleElement.method value is: supportdifferentusersadminstandardnew -no spaces and special characters

Additionally the list of the "example" attributes is there : (example: 1).

So what I try to do is:

  1. keep the previous matching functionality:
    return signature.IsMatch(exampleElement.name.ToLowerInvariant())

  2. split name by group["name"] and example attributes. Then replace name (with special characters and spaces) by method (with no special characters and spaces). And try to match it then
    signature.IsMatch(scenariotNameWithNoSpacesAndSpecialCharacters.ToLowerInvariant())

This change should be totally transparent for the old functionality and gives only additional try if it's not possible to match original name.

I will need some your support with unit tests because for next few days/maybe even weeks i definitely wont have time work on anything. Here you could find some more information and my founds about unit tests for that issue: #508

hope it helps to understand - I am not the best with explanation of the technical aspects in the real language sorry.

@DominikBaran
Copy link
Author

@dirkrombauts : Hi Dirk. do you think you could help me with these unit tests ? Unfortunately I do not have any chance to update it in the reasonable time :(
Thanks

@dirkrombauts
Copy link
Member

Hi @DominikBaran I want to help you but I'm more or less in the same situation - I don't have the opportunity to invest a lot of time in this. I'm chipping away at it, an hour here, half an hour there. Progress is slow.

@DominikBaran
Copy link
Author

@dirkrombauts : understand. that's all good - if you will be able to submit some code in the future it would be awesome. if not i will also try organize some time and add unit tests (but it could be even 2nd half of the year).
Cheers,

@dirkrombauts
Copy link
Member

@DominikBaran could you please check whether this problem still exists in version 2.18.1? There are some big changes to the name matching in the new version.

@DominikBaran
Copy link
Author

hello @dirkrombauts : unfortunately still the same issue:
image

@DominikBaran
Copy link
Author

DominikBaran commented Apr 23, 2018

@dirkrombauts : I have also one observation after update my Xunit from 2.1.0 to 2.3.0/2.3.1
For the newer version of Xunit Scenario Outline reports do not work at all:
Version 2.18.1
image

Version 2.17.0
image

Will try to investigate that problem - and also create new "Issue" for that

@DominikBaran
Copy link
Author

DominikBaran commented Apr 24, 2018

@dirkrombauts : I also added small fix for Xunit 2.1.0 and 2.3.x issue. For the matcher I remove \ if this kind of encoding exists

That should be fix for:
#522

Dominik Baran added 4 commits May 1, 2018 08:27
…develop

# Conflicts:
#	src/Pickles/Pickles.TestFrameworks/XUnit/XUnit2/XUnit2ScenarioOutlineExampleMatcher.cs
@DominikBaran DominikBaran reopened this May 1, 2018
…d the expression

use Regexp.IgnoreCase instead to LowerInvariant method
@dirkrombauts
Copy link
Member

I'll be on holiday until end of May, so I won't be able to continue on this until then.

@dirkrombauts dirkrombauts merged commit 47e7989 into picklesdoc:develop Jun 15, 2018
@dirkrombauts
Copy link
Member

Thank you for your contribution (and your patience)

@MichielBerkhof
Copy link

MichielBerkhof commented Jul 11, 2018

Hi Dirk,

I wonder if this also solves the following cases im facing:

Examples:

email outcome explanation
A@b@c@example.com not created only one @ character is allowed
@example.com not created localpart can't be empty
abc@ not created domain can't be empty
john..doe@example.com not created double dot before @

It wont properly handle @ signs.

@dirkrombauts
Copy link
Member

Hi @MichielBerkhof,

Good question - could you compile the latest version and test it? I will try to create a new release next week before I leave on holiday.

@MichielBerkhof
Copy link

MichielBerkhof commented Jul 11, 2018

Hi @dirkrombauts,

Ive cloned the repo, built the dev branch, and executed the pickles.exe against the scenario's.

This resulted in the following outcome:
pickledFeatures.js.txt

I can see the @ signs in the examples, but the was executed and wassuccesfull both return false. Which makes me think it doesnt work yet?

Please let me know if i read this correctly

To add to this, i did the same on the master branch, same result

@MichielBerkhof
Copy link

MichielBerkhof commented Jul 11, 2018

Here's another add:

The -1 scenario still doesnt seem to work for us. It runs correctly in VS2017 but pickles doesnt recognize it as being run and valid:

The feature file information

rate explanation
0 exchange rate can't be 0
-1 exchange rate can't be negative
99.9999999 exchange rate can't have more than six digits in the fractional part
99999999.99 exchange rate can't have more than 7 digits in the integer part

The json pickles generates:
image

Here is the living documentation that it has resulted in:
image

Please let me know if you need any more information. I could provide you with a .trx and .feature that corresponds with it, to let you help solve this.

@MichielBerkhof
Copy link

Hi @dirkrombauts,

Did you get a chance to look into these issues?

@dirkrombauts
Copy link
Member

No, I'm sorry. I'm rather limited with my time. It would be helpful if you could provide me with .trx and .feature files - maybe I will be able to squeeze in some debugging time next week. Trying to get a release out of the door has priority though. There's some features that are waiting to see the official light of day.

@MichielBerkhof
Copy link

CreateUsers.zip

Hi @dirkrombauts,

Above ZIP contains our CreateUser scenario with the trx results generated by TeamCity. In these examples you can see the scenario: Can't create user with invalid email address

In the trx, they all pass, but pickles wont see them as passed. I hope you could squeeze in just a little extra effort, would be much appreciated by our PO ;)

@dirkrombauts
Copy link
Member

Hi @MichielBerkhof

The reality of an open source project is that the fastest way to get something done is to fix it yourself and then send a pull request :-)

Cheers,
Dirk

@dirkrombauts dirkrombauts changed the title Scenario Outline special characters workaround. Scenario Outline special characters workaround Jul 20, 2018
@dirkrombauts dirkrombauts mentioned this pull request Jul 20, 2018
@dirkrombauts
Copy link
Member

Released in version 2.19.0

@dirkrombauts dirkrombauts mentioned this pull request Jul 20, 2018
@dirkrombauts
Copy link
Member

@MichielBerkhof I created a new issue to track the problems with @ signs and such that you reported on 11 July.

@MichielBerkhof
Copy link

Thanks dirk, ill try to find some spare time at work to take a more detailed look into it, but cant promise i can find the time. Its kinda busy...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants