Set fixed culture for CookieContainer blob sanity check #23285
Conversation
Why is that prohibitive? |
| @@ -42,6 +42,10 @@ public static IEnumerable<object[]> SerializableEqualityComparers_MemberData() | |||
| /// </summary> | |||
| public static IEnumerable<object[]> SerializableObjects_MemberData() | |||
| { | |||
| // Save old culture and set a fixed culture for object instantiation | |||
| CultureInfo oldCulture = CultureInfo.CurrentCulture; | |||
| CultureInfo.CurrentCulture = new CultureInfo("en-US"); | |||
There was a problem hiding this comment.
As this is an iterator, control returns to the caller at each yield point, and we don't know what xunit does or doesn't do between each yield. That means we're changing the culture on a thread that will then potentially be used for something else. If you want to do it this way, we should change SerializableObject_MemberData to not be an iterator, e.g. just have it return and return an object[][] populated with all of these entries, rather than yielding them. Then you can set the culture before, and unset it after in a finally block, and no one else will use the thread in the interim.
That said, I was under the impression this setting of culture like this was a no-no on uapaot, as it currently has process-wide impact, which is why we use RemoteInvoke for these. Has that been fixed, or if not, why is this ok in this case?
cc: @tarekgh
There was a problem hiding this comment.
I would recommend any test setting current culture to be executed remotely even if the test not UAP specific test. This will make the test more predictable and reliable.
by the way, we have a request (and tracking issue) to enable the way in uap to use current culture as it is used in the desktop apps.
There was a problem hiding this comment.
@stephentoub I will follow your suggestion and change the method from an iterator to simply return the object[][] array and inside of it use a try - finally block. Thanks for the tip, that's better than my current solution.
I don't want to change the test roundtripping test to use RemoteExecutor as some downsides come with it:
a) Test execution time increases dramatically
b) Refactoring would needed to be done so that all the object instantiations and checks would happen inside the RemoteExecutor. That would mean that we can't use the MemberData xunit pattern and everything would execute in one test instead of 300 separate tests. For mc logging I don't really want to go that way, mainly, because I already had it like that before and even with extensive logging the test behavior and possible exceptions was hard to understand.
Because of these reasons I want to avoid using an RemoteExecutor here. If other tests in other classes rely on the culture I could still put the xunit argument (forgot its name) onto the class to not execute both at the same time.
|
other than @stephentoub comment, LGTM. |
ViktorHofer
force-pushed
the
OSXSerialization
branch
2 times, most recently
from
199ff77 to
d97c4e0
Compare
ViktorHofer
force-pushed
the
OSXSerialization
branch
from
d97c4e0 to
95f998b
Compare
|
@stephentoub PTAL |
* Set fixed culture for CookieContainer blob sanity check * PR feedback
* Set fixed culture for CookieContainer blob sanity check * PR feedback
…x#23285) * Set fixed culture for CookieContainer blob sanity check * PR feedback Commit migrated from dotnet/corefx@04c1a2c
Fixes https://github.com/dotnet/corefx/issues/21720
Not defining a fixed culture causes various test cases to fail sometimes as the at runtime instantiated object isn't the same as the serialized stored one.
RemoteExecutor isn't possible in this case as we can't let every test case (~300) invoke a new RemoteExecutor instance. Try-Finally pattern is also not possible as we are using yield return statements. For this method I think this is the best solution.