Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[WIP] add localized unit names #974
[WIP] add localized unit names #974
Changes from 4 commits
414ed57
b54f9b8
476d89e
038dce9
f6121f2
77e7f9c
f14edf9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
BOB, is that a placeholder? Should we maybe use null or empty string instead?
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.
As mentioned, I believe you can revert this entire file. Windows Runtime Component build target is no longer receiving new features.
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 only reason we use
CollectionDefininition
from before is to disable parallelization of tests that manipulate static values like CurrentCulture, so maybe we don't need multiple of these.In another project, we have been using
Maybe we can rename
UnitAbbreviationsCacheFixture
toNotThreadSafe
instead and copy this xmldoc there?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.
CollectionDefininition itself is used to shared a context (data for example between a collection) of tests . The parallelization is just an additionnal option.
I think that:
1°) you should add a getter in UnitLocalizedNamesCacheFixture to get the Culture instance
2°) you should modify the constructors of all your TUs to inject the fixtue each time you need to get the culture
Links:
https://xunit.net/docs/shared-context
https://blog.somewhatabstract.com/tag/collectiondefinition/
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.
Interesting, could you give an example of how this could be used to test
ToString()
andParse()
, where no culture is given as parameter, but we want a consistent way to test specific cultures in the current thread? I'm very interested to learn if there is a better way to do this.