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

Possible solution for culture invariant tests using UseCultureAttribute #1005

Merged
merged 7 commits into from Mar 9, 2019

Conversation

Projects
None yet
3 participants
@lg2de
Copy link
Contributor

lg2de commented Mar 3, 2019

In #1003 I listed several options to run the Fluent Assertions tests on all Windows locales.
In this PR I want to show a possible by running culture related test explicitely on the expected culture.

@dennisdoomen
Copy link
Member

dennisdoomen left a comment

Good idea!

@jnyrup
Copy link
Collaborator

jnyrup left a comment

This PR modifies global static variables between parallel runs of unit tests.
I'm no fan of that approach as that introduces potential concurrency issues.

I agree that no unit tests in FA should be affected by the locale.
To me that clearly indicates that the tests asserts on more than necessary.
See tip 10 of https://www.continuousimprover.com/2015/11/12-tips-to-write-unit-tests-that-dont.html

Instead
When_asserting_collections_not_to_be_equal_but_expected_collection_is_null_it_should_throw could be rewritten as

act.Should().Throw<ArgumentNullException>()
    .WithMessage("Cannot compare collection with <null>.*")
    .And.ParamName.Should().Be("unexpected");

or maybe even

act.Should().Throw<ArgumentNullException>()
    .Which.ParamName.Should().Be("unexpected");
@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Mar 4, 2019

This PR modifies global static variables between parallel runs of unit tests.

For the .NET versions that support it, it will change it only for the length of a test. However, an alternative would be to set the culture for the entire test suite.

We could be more liberal in what we expect, but we have no way to test if this works. AppVeyor agent always use an English version of Windows.

@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Mar 4, 2019

@dennisdoomen I'm not sure I understand you regarding the length of a test?
When tests are run in parallel, setting a static variable has nothing to do with the lifetime of a single test.

I would rather set the culture at one single point, which is ensured to be run before any tests, than setting the culture on individual tests.
I don't know how to do that in xunit, or if it even possible without making each class inherit from some base class, which I'm not the biggest fan of.

I was thinking of trying to run all your tests in all locales, to check if we have more tests fragile to different locales.

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Mar 4, 2019

Yeah, you're right. Even when it only changes the culture of the current thread temporarily, thread jumping may happen in async tests. We could try to force the entire test to the invariant culture, but XUnit does not have a nice way for that other than using collection fixtures on each test class.

@lg2de

This comment has been minimized.

Copy link
Contributor Author

lg2de commented Mar 4, 2019

You are right. This is not the best solution. Do you have another idea to MAKE the tests culture invariant?
I tried setting the culture within build.ps1, but I did not succeed.

This solution helps ME to get successful completed tests. It will not affect the CI build environment because it is already english locale. The modified tests are NOT using async.
I would like to provide PR for another issue(s). Do I need to switch my PC to english locale to contribute to FluentAssertions?
There are other test failing due to timing issues. Maybe we can live with this too?

@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Mar 4, 2019

It seems to be possible in xUnit.net: https://github.com/xunit/samples.xunit/blob/master/AssemblyFixtureExample/Samples.cs

Such culture fixing should not be be set in ps1/cake code.
Running tests from VS or via CLI should yield identical results.

@lg2de

This comment has been minimized.

Copy link
Contributor Author

lg2de commented Mar 5, 2019

Hi @jnyrup, I did not use AssemblyFixture, because this is not a build-in feature of Xunit.
Instead I introduced CollectionFixture on the classes with "localized" tests.

Lukas Grützmacher
@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Mar 7, 2019

@lg2de bummer...
I'm still no fan of this approach.
How do you decide when a class should be decorated with [Collection("Invariant Culture Collection")]?

I liked how #726 handled culture problems.

@lg2de

This comment has been minimized.

Copy link
Contributor Author

lg2de commented Mar 7, 2019

Mmh, it looke quite difficult to match your expectations...
The approach of mentioned PR does not match this problem because I cannot control how ArgumentNullException builds its text. It is just depending on the locale.

I think we need to change the tests to NOT check the locale dependent tests...

@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Mar 8, 2019

@lg2de

I cannot control how ArgumentNullException builds its text

Exactly, and that's why the test shouldn't assert on that part of the exception message.
What we are interested in, is that the property ParamName should be unexpected.

That's what I proposed in the my first examples of rewriting the test.

@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Mar 9, 2019

I'm not sure if you're trying to alter the tests as little as possible?
As the framework has evolved, some tests may have suffered from not being rewritten with newer features.
I think the usage of .Where(ex => ...) in ObjectAssertionSpecs is such an example.
In my opinion those assertions are more clearly expression through WithMessage().

Here are some examples on how I would probably write those tests.

When_an_object_is_binary_serializable_but_not_deserializable_it_should_fail

act.Should().Throw<XunitException>()
    .WithMessage("*to be serializable, but serialization failed with:**BinarySerializableClassMissingDeserializationConstructor*");

When_an_object_is_not_data_contract_serializable_it_should_fail

act.Should().Throw<XunitException>()
    .WithMessage("*we need to store it on disk*EnumMemberAttribute*");

I agree that the ObjectAssertionSpecs specs are not the easiest to write robust and clear tests for.

When_asserting_collections_not_to_be_equal_but_expected_collection_is_null_it_should_throw

act.Should().Throw<ArgumentNullException>()
    .WithMessage("Cannot compare collection with <null>*")
    .And.ParamName.Should().Be("unexpected");

Instead of relying on what the message contains, it not relies on the type system.

@lg2de

This comment has been minimized.

Copy link
Contributor Author

lg2de commented Mar 9, 2019

Dear @jnyrup, this was my third attempt to fulfil your expectations. Step by step I loss motivation to continue...
For sure, I tried to change as less as possible. I replaced ONLY (!!!) the checks of german texts. These texts are comparing texts from .NET framework and should not be checked by FluentAssertions tests.

I've updated When_an_object_is_not_data_contract_serializable_it_should_fail according to your suggestion. The other test you may change later. I think, I was following existing patterns of other tests.

My main goal was to get the master branch teastable on a German PC (like my one) to contribute another feature(s).

@lg2de

This comment has been minimized.

Copy link
Contributor Author

lg2de commented Mar 9, 2019

... and your recommendation When_an_object_is_binary_serializable_but_not_deserializable_it_should_fail will not work: "to be serializable, but serialization failed with" is part of the localized text!

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Mar 9, 2019

@jnyrup Let's just merge this one. If it fixes the problem @lg2de describes, it's fine by me. Feel free to apply a better fix as a reference example after that.

@jnyrup

jnyrup approved these changes Mar 9, 2019

Copy link
Collaborator

jnyrup left a comment

@lg2de I'm sorry for pushing my quest for reference examples on you.
The current PR looks good and is indeed an improvement 👍

I was unclear in my note about changing as little as possible, you did change as little as possible, which is perfectly fine.
It was supposed to mean, that you didn't have to change as little as possible if you wanted to improve the assertions further.

Out of curiosity, I hope my example When_an_object_is_binary_serializable_but_not_deserializable_it_should_fail does work for you.
I changed my windows language to German to test my examples before posting them here.
"to be serializable, but serialization failed with" comes from
https://github.com/fluentassertions/fluentassertions/blob/master/Src/FluentAssertions/ObjectAssertionsExtensions.cs#L61 so it shouldn't have been localized in any way.

@jnyrup jnyrup merged commit f360725 into fluentassertions:master Mar 9, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@lg2de lg2de deleted the lg2de:UseCultureAttribute branch Mar 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.