[libraries] Move library tests Feature Switches defaults to Functional tests #53253

mdh1418 · 2021-05-25T21:23:00Z

In the work to reduce library testing infrastructure size, EnableAggressiveTrimming had been added in addition to a few properties from https://docs.microsoft.com/en-us/dotnet/core/deploying/trimming-options#trimming-framework-library-features.

These properties were not conditioned under EnableAggressiveTrimming or made library testing specific, so they probably attributed to #48027, which had been resolved by #48429 using DefaultBlazorWASMFeatureSwitches.

In the work to move WASM AOT test building to helix, some of the properties were modified #48226, probably to get tests buliding and passing.

In a similar vein, Xamarin Android had set switch defaults for runtime libraries also to reduce size dotnet/android#5337, and this work had been extended to dotnet/runtime via
#49005 and #49635 because it was thought that because Browser wasm had them in tests.wasm.targets that they would be needed for Android and iOS.

Taking the above events into account, the properties were originally intended to reduce library testing size when trimming (so when EnableAggressiveTrimming=true) but weren't conditioned as such, and led to the other events.

This PR looks to:

Remove ActiveIssue 52645 attributes
Set the trimming framework features properties directly in the Functional tests project properties and model them after Xamarin Android, Xamarin Mac/iOS, and WASM
Set the trimming framework features properties in tests.mobile.targets to the values previously found in tests.wasm.targets
Remove UseDefault*FeatureSwitches

ghost · 2021-05-25T21:23:07Z

Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

Author:	mdh1418
Assignees:	-
Labels:	`area-Infrastructure-libraries`
Milestone:	-

filipnavara · 2021-05-25T21:42:30Z

Why only for Android and not for iOS? Is there some background on why it was used for tests in the first place?

steveisok · 2021-05-25T22:11:46Z

Why only for Android and not for iOS? Is there some background on why it was used for tests in the first place?

We wanted standard defaults for the functional tests and I think it got mixed with the libraries tests. Fair point re: iOS. I agree.

radical · 2021-05-26T14:57:19Z

We wanted standard defaults for the functional tests and I think it got mixed with the libraries tests.

This should be added to the PR description too.

…esource_keys_from_libraries

filipnavara · 2021-05-26T16:47:42Z

LGTM

radical · 2021-05-26T17:19:22Z

We wanted standard defaults for the functional tests and I think it got mixed with the libraries tests.

Is $(UseDefaultAndroidFeatureSwitches) ever !="true"? Just wondering if we need these properties at all.
If these were added just for functional tests, and got added for libraries by mistake, then can/should the equivalent thing in tests.wasm.targets be removed too?

cc @akoeplinger

filipnavara · 2021-05-26T18:17:16Z

Is $(UseDefaultAndroidFeatureSwitches) ever !="true"? Just wondering if we need these properties at all.

No. Same for iOS.

mdh1418 · 2021-05-26T19:13:53Z

The default switches properties are set to false in the scope of /eng/testing/linker/SupportFiles/Directory.Build.props

runtime/eng/testing/linker/SupportFiles/Directory.Build.props

Lines 4 to 6 in 84b7c67

    
           <UseDefaultBlazorWASMFeatureSwitches>false</UseDefaultBlazorWASMFeatureSwitches> 
        
           <UseDefaultAndroidFeatureSwitches>false</UseDefaultAndroidFeatureSwitches> 
        
           <UseDefaultiOSFeatureSwitches>false</UseDefaultiOSFeatureSwitches>

@MaximLipnin I'm not exactly sure why they're needed, do you know in more detail?

I'll move the Browser wasm equivalent from tests.mobile.targets to src/tests/FunctionalTests/Directory.Build.props

radical · 2021-05-26T19:30:29Z

You can also drop the 3 properties from tests.mobile.targets (https://github.com/mdh1418/runtime/blob/remove_use_system_resource_keys_from_libraries/eng/testing/tests.mobile.targets#L9-L11).

radical · 2021-05-26T19:31:11Z

.. and move them to the functional tests csproj!

mdh1418 · 2021-05-26T19:38:25Z

Ah right! Totally missed that, thanks :)

MaximLipnin · 2021-05-27T07:39:28Z

The default switches properties are set to false in the scope of /eng/testing/linker/SupportFiles/Directory.Build.props

runtime/eng/testing/linker/SupportFiles/Directory.Build.props

Lines 4 to 6 in 84b7c67

<UseDefaultBlazorWASMFeatureSwitches>false</UseDefaultBlazorWASMFeatureSwitches>

<UseDefaultAndroidFeatureSwitches>false</UseDefaultAndroidFeatureSwitches>

<UseDefaultiOSFeatureSwitches>false</UseDefaultiOSFeatureSwitches>

@MaximLipnin I'm not exactly sure why they're needed, do you know in more detail?

I added ios/android props analogously to the browser one. If it's not necessary now we could remove them.

…raries with EnableAggressiveTrimming

mdh1418 · 2021-05-27T20:47:12Z

I think I discovered the series of events that led to the properties as of current, and updated the PR description. Can I get another 👀 over if it makes sense? @filipnavara @radical @MaximLipnin

radical · 2021-05-27T21:00:11Z

I think I discovered the series of events that led to the properties as of current, and updated the PR description. Can I get another 👀 over if it makes sense? @filipnavara @radical @MaximLipnin

Awesome! Thank you for getting that whole history figured out 👍

radical

This does explicitly set some new properties, for the wasm case, which might be fine. Let's wait to see how the builds go.

filipnavara · 2021-05-27T21:52:54Z

Thanks for putting together the links. It really helps to show the context!

For reference, here are the defaults on the Xamarin.Mac/iOS side since the Xamarin.Android ones are already linked in the description:
https://github.com/xamarin/xamarin-macios/blob/aad913db8673637150af01b13e76605966b8f14a/dotnet/targets/Xamarin.Shared.Sdk.targets#L118-L134
Same for the WASM:
https://github.com/dotnet/sdk/blob/c90ef5e103e5c961e40acd6ab1b1622353cc2965/src/BlazorWasmSdk/Targets/Microsoft.NET.Sdk.BlazorWebAssembly.Current.targets#L57-L63

I'm still somewhat torn on what is the intended setup. On one hand keeping some of the switches "on" may reduce the size of the packages that are produced and transferred to Helix and in turn save some time and bandwidth. It also more closely mirrors the defaults that customer will get when targeting net6.0-ios / net6.0-android. On the other hand it causes compatibility errors with some of the tests and the environment they expect and it also overrides runtimeconfig.template.json (#53172 (comment)) which caught me off guard (and perhaps should be tracked as dotnet/sdk issue?).

I'd certainly value some other opinion on what do we really want to test here since there's no clear answer to that. Is the intent for functional tests to mirror the options customers get? Are the unit tests intentionally run in an environment that is unified across platforms but not matching the defaults customers get? Are there actually some tests specifically for these options and whether they are honoured properly?

steveisok · 2021-05-28T00:43:33Z

The intent of the functional tests are to closely mirror what our customers will get w/ xamarin android and ios. I had forgotten we originally brought these defaults up for our wasm AOT testing efforts (where a smaller app really matters).

As for the libraries tests, the problem with applying centralized defaults in tests.mobile.targets is that there are no opportunities for the test project to override them. Also, I would rather not try to deviate from having the test project be in control of its own configuration.

filipnavara · 2021-05-28T06:02:49Z

the problem with applying centralized defaults in tests.mobile.targets is that there are no opportunities for the test project to override them

I started to introduce a mechanism to override them on per-test-project basis by adding the same Conditon="'$(XXX)' == ''" attribute to each of the options as there is in the workloads/SDKs.

I am fine with keeping the defaults different for functional tests (that finally run as part of CI) and the unit tests since it makes some things easier. Just wanted to point out that there are some downsides to it too.

That leaves the last question, is there a test coverage somewhere (dotnet/sdk? linker?) specifically for testing the options themselves? For example, I would expect tests that explicily expect 1) disabled UTF-7 encoding 2) exception messages having the resource key instead of localized string 3) disabled binary serialization.

…defaults for enable aggressive trimming

…ts to Functional tests

mdh1418 · 2021-05-28T19:47:38Z

Thanks for the input!

The intent of the functional tests are to closely mirror what our customers will get w/ xamarin android and ios

Because of this, I decided to add the Xamarin Android, Xamarin Mac/iOS, and Wasm feature defaults into the Functional tests project properties.

I started to introduce a mechanism to override them on per-test-project basis by adding the same Conditon="'$(XXX)' ==

''" attribute to each of the options as there is in the workloads/SDKs.
I copied this scheme for the properties set in tests.mobile.targets

src/tests/FunctionalTests/Directory.Build.props

mdh1418 · 2021-05-28T23:15:55Z

The failing lanes seem to be unrelated.
Android x64 RuntimeTests is a PackageInstallationError
The other lane is related to the installer for CoreCLR on Linux, where this PR is relevant for library tests.

…asm_debugger_and_use_debugger_agent * upstream/main: (597 commits) Fix42292 (dotnet#52463) [mono] Remove some obsolete emscripten flags. (dotnet#53486) Fixed path to projects (dotnet#53435) support ServerCertificateContext in quic (dotnet#53175) Socket: delete unix local endpoint filename on Close (dotnet#52103) [mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364) Refactor MsQuic's native IP address types. (dotnet#53461) Re-enabled optimizations for gtFoldExprConst (dotnet#53347) Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361) Fix issues with virtuals and mdarrays (dotnet#53400) Split Variant marshalling tests (dotnet#53035) Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470) remove WSL checks in tests (dotnet#53475) Always spawn message loop thread for SystemEvents (dotnet#53467) add AcceptAsync cancellation overloads (dotnet#53340) Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425) Add CookieContainer.GetAllCookies (dotnet#53441) Remove DynamicallyAccessedMembers on JsonSerializer (dotnet#53235) [wasm] Re-enable Wasm.Build.Tests (dotnet#53433) [libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253) ...

dotnet-issue-labeler bot added the area-Infrastructure-libraries label May 25, 2021

mdh1418 requested a review from steveisok May 25, 2021 21:23

[libraries] Remove UseSystemResourceKeys true default

8d0a0eb

Mitchell Hwang added 3 commits May 26, 2021 11:10

[libraries] Remove UseSystemResourceKeys for iOS

ab46fbf

[libraries] Resolve WebSockets System resource keys activeissue

309b41f

Merge remote-tracking branch 'upstream/main' into remove_use_system_r…

0199991

…esource_keys_from_libraries

mdh1418 marked this pull request as ready for review May 26, 2021 16:42

mdh1418 requested review from MaximLipnin, filipnavara and radical May 26, 2021 16:42

[tests] Move default feature switches from libraries to functional tests

f01c3b1

Mitchell Hwang added 2 commits May 26, 2021 15:17

[tests] Move Browser wasm default switches into FuntionalTests scope

fa74fe9

Add Browser OS condition

d9f5dce

Move DefaultFeatureSwitches initialization to FunctionalTests project

ee0dc9d

runfoapp bot mentioned this pull request May 27, 2021

FileSystemWatcher_DirectorySymbolicLink_TargetsFile_Fails #53366

Open

Consolidate trimming framework library features and condition for lib…

f84657a

…raries with EnableAggressiveTrimming

mdh1418 requested a review from akoeplinger May 27, 2021 20:45

radical reviewed May 27, 2021

View reviewed changes

Mitchell Hwang added 2 commits May 28, 2021 15:39

[testing][EAT] Revert trimming framework library features to browser …

a66f971

…defaults for enable aggressive trimming

[tests] Add Xamarin Android, Xamarin Mac/iOS, and Wasm feature defaul…

794e273

…ts to Functional tests

radical reviewed May 28, 2021

View reviewed changes

src/tests/FunctionalTests/Directory.Build.props Outdated Show resolved Hide resolved

radical reviewed May 28, 2021

View reviewed changes

src/tests/FunctionalTests/Directory.Build.props Outdated Show resolved Hide resolved

radical reviewed May 28, 2021

View reviewed changes

src/tests/FunctionalTests/Directory.Build.props Outdated Show resolved Hide resolved

Fixup tabs to spaces

6a05abc

radical reviewed May 28, 2021

View reviewed changes

src/tests/FunctionalTests/Directory.Build.props Outdated Show resolved Hide resolved

Mitchell Hwang added 2 commits May 28, 2021 16:14

Add common feature switches, links, and cleanup

24add4d

Cleanup Xamarin Android link

f0a2a18

runfoapp bot mentioned this pull request May 28, 2021

leakwheel failing in CI #53452

Closed

mdh1418 changed the title ~~[libraries] Remove UseSystemResourceKeys true default~~ [libraries] Move library tests Feature Switches defaults to Functional tests May 28, 2021

steveisok approved these changes May 28, 2021

View reviewed changes

radical approved these changes May 28, 2021

View reviewed changes

mdh1418 merged commit c6b5994 into dotnet:main May 29, 2021

mdh1418 deleted the remove_use_system_resource_keys_from_libraries branch May 29, 2021 00:07

ghost locked as resolved and limited conversation to collaborators Jun 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libraries] Move library tests Feature Switches defaults to Functional tests #53253

[libraries] Move library tests Feature Switches defaults to Functional tests #53253

mdh1418 commented May 25, 2021 •

edited

Loading

ghost commented May 25, 2021

filipnavara commented May 25, 2021

steveisok commented May 25, 2021

radical commented May 26, 2021

filipnavara commented May 26, 2021

radical commented May 26, 2021

filipnavara commented May 26, 2021

mdh1418 commented May 26, 2021

radical commented May 26, 2021

radical commented May 26, 2021 •

edited

Loading

mdh1418 commented May 26, 2021

MaximLipnin commented May 27, 2021

mdh1418 commented May 27, 2021

radical commented May 27, 2021

radical left a comment

filipnavara commented May 27, 2021 •

edited

Loading

steveisok commented May 28, 2021

filipnavara commented May 28, 2021 •

edited

Loading

mdh1418 commented May 28, 2021 •

edited

Loading

mdh1418 commented May 28, 2021

[libraries] Move library tests Feature Switches defaults to Functional tests #53253

[libraries] Move library tests Feature Switches defaults to Functional tests #53253

Conversation

mdh1418 commented May 25, 2021 • edited Loading

ghost commented May 25, 2021

filipnavara commented May 25, 2021

steveisok commented May 25, 2021

radical commented May 26, 2021

filipnavara commented May 26, 2021

radical commented May 26, 2021

filipnavara commented May 26, 2021

mdh1418 commented May 26, 2021

radical commented May 26, 2021

radical commented May 26, 2021 • edited Loading

mdh1418 commented May 26, 2021

MaximLipnin commented May 27, 2021

mdh1418 commented May 27, 2021

radical commented May 27, 2021

radical left a comment

Choose a reason for hiding this comment

filipnavara commented May 27, 2021 • edited Loading

steveisok commented May 28, 2021

filipnavara commented May 28, 2021 • edited Loading

mdh1418 commented May 28, 2021 • edited Loading

mdh1418 commented May 28, 2021

mdh1418 commented May 25, 2021 •

edited

Loading

radical commented May 26, 2021 •

edited

Loading

filipnavara commented May 27, 2021 •

edited

Loading

filipnavara commented May 28, 2021 •

edited

Loading

mdh1418 commented May 28, 2021 •

edited

Loading