share-argument-exception-extension

[Zombach]

Description

message

Checklist

• Is this feature complete?
  • [x] Yes. Ready to ship.
  • [ ] No. Follow-up changes expected.
• Are you including unit tests for the changes and scenario tests if relevant?
  • [ ] Yes
  • [x] No
• Did you add public API?
  • [ ] Yes
    • If yes, did you have an API Review for it?
      • [ ] Yes
      • [ ] No
    • Did you add <remarks /> and <code /> elements on your triple slash comments?
      • [ ] Yes
      • [ ] No
  • [x] No
• Does the change make any security assumptions or guarantees?
  • [ ] Yes
    • If yes, have you done a threat model and had a security review?
      • [ ] Yes
      • [ ] No
  • [x] No
• Does the change require an update in our Aspire docs?
  • [ ] Yes
    • Is this introducing a breaking change?
      • [ ] Yes
        • Link to aspire-docs issue (please use this breaking-change template):
      • [ ] No
        • Link to aspire-docs issue (please use this doc-idea template):
  • [x] No

[Zombach]

@danmoseley Hello. Please look. Made a check share code

[Zombach]

@danmoseley If anything is ready, you can fill.

[mitchdenny]

This looks good to me. Just need the covering tests for all the other resource types.

[Zombach]

This looks good to me. Just need the covering tests for all the other resource types.

And what exactly do I need to cover the tests in this MR? I did not quite understand. In my previous MR #7575 i have already covered these elements.

[Zombach]

Here I came across several non -critical mistakes in tests. Which depend on the operating system and on the localization of the system

In this screenshot, there is an error in due to '.' <=> ','

Here the problem arises due to the localization of the operating system on which the test is launched. Although the expected values ​​and the result of the test are equal in meaning, but still they are in different languages Launch settings file does not contain 'not-exist' "··· Файл параметров запуска не содержит профиля "not-e"···

Also the problem of localization

I don't know how critical it is. But these are still errors

[danmoseley]

Tests should pass in all locales

[RussKie]

I think there are (at least) two ways of solving this.

1. Tactical - set the culture for each test and unset at it at the end. E.g. something like this pseudocode:

       try
       {
            _originalCulture = Thread.CurrentThread.CurrentCulture;
            _originalUICulture = Thread.CurrentThread.CurrentUICulture;
   
            CultureInfo.DefaultThreadCurrentCulture = Culture;
            Thread.CurrentThread.CurrentCulture = "en-US";
            Thread.CurrentThread.CurrentUICulture = "en-US"
   
           // ... body of the test
       }
       finally
       {
            Thread.CurrentThread.CurrentCulture = _originalCulture;
            Thread.CurrentThread.CurrentUICulture = _originalUICulture;
       }
   

2. Pass the culture as a theory argument. This would allow asserting the messages/formatting are correct across different locales.
3. Strategic - add an attribute which can be applies to a test/fixture. Something like https://github.com/dotnet/winforms/commit/9a26e660c6f7f3663dac601a4bba64961e046fa1 (though it'd be simpler since there are no unmanaged resources here).

[Zombach]

I think there are (at least) two ways of solving this.

1. Tactical - set the culture for each test and unset at it at the end. E.g. something like this pseudocode:

       try
       {
            _originalCulture = Thread.CurrentThread.CurrentCulture;
            _originalUICulture = Thread.CurrentThread.CurrentUICulture;
   
            CultureInfo.DefaultThreadCurrentCulture = Culture;
            Thread.CurrentThread.CurrentCulture = "en-US";
            Thread.CurrentThread.CurrentUICulture = "en-US"
   
           // ... body of the test
       }
       finally
       {
            Thread.CurrentThread.CurrentCulture = _originalCulture;
            Thread.CurrentThread.CurrentUICulture = _originalUICulture;
       }
   

2. Pass the culture as a theory argument. This would allow asserting the messages/formatting are correct across different locales.
3. Strategic - add an attribute which can be applies to a test/fixture. Something like dotnet/winforms@9a26e66 (though it'd be simpler since there are no unmanaged resources here).

I chose the 3rd point. But this did not help this test, here is the problem with ''. ' => ',' which depends on OSPlatform >

You need to understand what to do with it. Also due to static resources when using [UsedEfaultxunitculture] This test breaks even more.

[danmoseley]

@Zombach not about this PR directly, but chatting offline, I think we want to hold off on any future large "argument validation" PR's without agreement first. Yes, code changes should validate arguments. A PR fixing a particular instance is probably fine. But any more large scale ones don't make sense right now as we're biasing for speed, and it brings the risk of regressions.

[Zombach]

@Zombach not about this PR directly, but chatting offline, I think we want to hold off on any future large "argument validation" PR's without agreement first. Yes, code changes should validate arguments. A PR fixing a particular instance is probably fine. But any more large scale ones don't make sense right now as we're biasing for speed, and it brings the risk of regressions.

Yes, I will make Culture fix in a separate PR. And i will roll back this one to the previous state (before Culture fix)

[danmoseley]

Let's merge this after the 9.3 snap

[danmoseley]

@Zombach we are now open for 9.4 changes. can you resolve conflicts - is this ready to go?

[dotnet-policy-service[bot]]

This submission has been automatically marked as stale because it has been marked as requiring author action but has not had any activity for 14 days. It will be closed if no further activity occurs within 7 days of this comment.