ttutisani / Xunit.Gherkin.Quick

BDD in .NET Core - using Xunit and Gherkin (compatible with both .NET Core and .NET)
MIT License
205 stars 29 forks source link

Use US Culture for automatic tests that fail to parse with other language configurations #46

Closed ChristianPejrup closed 6 years ago

ChristianPejrup commented 6 years ago

Hi @ttutisani

Just pulled latest master to se the status of your project and if there might be something i could contribute.

I experience that 10 of the tests fail.

the last two fail with a format exception System.FormatException : String was not recognized as a valid DateTime.

This looks like it is related to the way that parameters are parsed to their specific type in the PrimitiveTypeArgument file.

Value = Convert.ChangeType(argumentValues[_index], _parameterInfo.ParameterType);

it looks like adding the CultureInfo.InvariantCulture parameter fixes the issue.

ttutisani commented 6 years ago

That's strange. All tests pass for me...

ttutisani commented 6 years ago

Oh wait, is your machine on different culture than en-us? That can be it.

ttutisani commented 6 years ago

I reviewed your PR. Thanks for fixing it! Can you please add an unit test that ensures that such issue does not come back accidentally?

ChristianPejrup commented 6 years ago

Will do - sorry for the late reply...

ChristianPejrup commented 6 years ago

Hmm, guess its not as easy as i thought. I mistakenly thought that InvariantCulture somehow support all cultures, but there is a reason why there are no easy fixes to Localization.

Basically applying InvariantCulture will limit the code to only working with a limited set of Date formats, which if used should be added to the documentation.

Example. Applying InvariantCulture for (2018-01-30)

The question here is if this is actually a culture issue that the framework should handle or if its a issue with contributors not using same OS Localization?

The alternative to using InvariantCulture could be to configure the tests to define their Culture (this case American) - which would make it work for everyone contributes to the project.

Thread.CurrentThread.CurrentCulture = new CultureInfo("en-US");

I honestly have no idea how other people handle this with their frameworks...so any input is very welcome.

ttutisani commented 6 years ago

Good question and good point.

I think the framework's code should not be fixed. Instead, the tests should be fixed to use the en-US culture.

Framework's code is culture sensitive by default (since built in .net methods for conversion are culture sensitive). So, now that I think about it, I guess there is no need to fix the framework's code for specific culture. Just set the culture in the tests. That will make tests pass for everybody regardless of their machine settings. And the framework code will not care for overriding the culture.

So yes, good thought, let's set the culture in the tests project only.

ttutisani commented 6 years ago

So, is there any action item on this, in your mind? One I can suggest is to set the culture in tests, if they fail for you. Because all those tests (including Gherkin which I wrote for them) assume that current culture is en-us.

ChristianPejrup commented 6 years ago

Hi,

Sorry for the delay in my response.

I have made a ultra simple commit that fixes the culture but only for the tests that were failing because of the culture issue. I was debating if it would be better to just add the culture to all tests but decided that it would probably just confuse more than it would help.

I have added PR with changes

ttutisani commented 6 years ago

No worries about the delay! That is a good enough fix. I wish there was a way to prevent me from breaking it for you and others with non en-us culture, unintentionally.

ChristianPejrup commented 6 years ago

I think it's possible for the Date format by applying yyyy-MM-dd as thats the only format that all countries seem to be able to agree on.

But the other numbering formats that were giving me problems (to the best of my knowlege) do not have a "universal" formatting.

Anyway...thanks for all the cool work on this library 👍