Skip to content

Conversation

@marcpopMSFT
Copy link
Member

@marcpopMSFT marcpopMSFT commented Jun 12, 2023

To enable this required a variety of changes:

  • Undo the lock file changes that were done for CG. Since it's test only, we can exclude those requirements
  • upgrade tests to the current tfm
  • pass through the required runtime pack flag to the ProcessFrameworkReference task
  • Update the mock to correctly pull existing objects
  • Update a bunch of strings to match our naming and comment requirements
  • Add error code exclusions to fix those tests
  • Disable four tests on non-windows as they weren't working there
  • Add environment variable for the assets path and include them in the helix package
  • Set environment variables for two other items required for the unit tests to work correctly and remove those from the CLI arguments

@ghost ghost added Area-Infrastructure untriaged Request triage from a team member labels Jun 12, 2023
@marcpopMSFT
Copy link
Member Author

@sbomer I'm trying to enable these unit tests we haven't had running in a while and they include a test that requires certain naming conventions for various messages. in particular, message either need an error number or need to end in _Info.

I tried to update ILLinkOptimizedAssemblies and ILLinkRunning but it turns out those are consumed by the ILLink.targets that aren't in this repo.

What is the path forward here? Do I need to limit this change to main and make an update in the dotnet/linker repo first? Are there tests in the linker repo that will fail if I do? I also noticed that there's a copy of the linker targets in the runtime repo. Does that also need to be modified or does that get updated automatically through codeflow?

@sbomer
Copy link
Member

sbomer commented Jun 12, 2023

The linker targets have been moving around in the last few releases.

  • .NET 8: dotnet/runtime (no, not updated through code flow - this is the new home for the linker)
  • .NET 7: dotnet/linker
  • earlier: SDK

We moved the targets out of the SDK in dotnet/linker#2837 - we thought the testing situation was an ok compromise, but at the time I didn't catch the dependency on the SDK resource strings.

The targets initially lived in the SDK exactly because SDK already had test infra and localization for targets. Other targets that live in runtime also are not localized, and are tested in SDK. I'd like to find a better long-term path for the localization, and remove the cross-repo dependency on SDK's resource strings.

But for now, to make the change in main, you'd need to update the reference in dotnet/runtime first, and to make it for 7.0 you'd need to update it in dotnet/linker first. AFAIK we don't have tests in linker or runtime that will fail if you do.

@sbomer
Copy link
Member

sbomer commented Jun 12, 2023

I filed dotnet/runtime#87435 about finding a better solution to this.

@marcpopMSFT
Copy link
Member Author

Thanks for the summary. I think for simplicity, I may just enable the unit test project in main and only fix the strings in runtime (instead of trying to change things in 7.0 as well). If I make a change in runtime to change the ID, will it break tests in runtime that rely on the string resources that live here?

I'm not ready yet to enable the unit test project (still working through infrastructure issues) and I could potentially disable this one test for now (and revert the string changes in this PR) but it'd be good to figure out how we would update that in the future. Would it be something where we'd have to have both the old and new string id, update the SDK version in runtime, then change runtime, then remove the old strings here (ie do a staged change)?

…accept it on the command line.

TestContext.cs will prioritize the command line first and then the environment variable.

Fix a test.
@sbomer
Copy link
Member

sbomer commented Jun 13, 2023

I'm not aware of any runtime tests that rely on the string resource IDs (or their values).

As far as I know, runtime's usage of the linker is either via custom targets (that don't use these resources), or the LKG SDK. So I think it would work to update the targets in runtime to use non-existent IDs, and then create the IDs when the change flows into SDK.

If I'm wrong about that, then I think the staged change approach you described would work. I would try changing the IDs in runtime, and if nothing fails in ci it should be fine. We'll just want to make the update quickly in SDK to avoid blocking dependency flow for long.

@marcpopMSFT
Copy link
Member Author

I talked to Daniel and we decided it probably wasn't worth a bunch of effort to just unwind the two linker resources for a test (as there's no functional justification for that afaik). Instead, I'll add an exception to the test for now and revert those. We can decide to change the linker ones in the future if we decide it's a problem.

Add DOTNET_SDK_TEST_MSBUILDSDKRESOLVER_FOLDER to the setup script
Save the test assets directory even if it doesn't exist so I can try to figure out where it should be
Account for 1206
Account for different path separator on non-windows
Disable a set of tests on non-windows as the PackageDependenciesDesignTime ends up returning null (need to discuss this with Daniel)
Add 1207 to the known error list
@marcpopMSFT marcpopMSFT requested a review from dsplaisted June 26, 2023 21:45
@marcpopMSFT
Copy link
Member Author

@dsplaisted, finally got it to pass checks. The most interesting of my changes are the inclusion of assets in the helix payload, setting a flag for that and the disabling of some tests on non-windows, and my reverting of the lock files (which may end up triggering CG which can be dismissed). I think the rest of the changes are pretty straightforward. Happy to walk you through it.

@marcpopMSFT marcpopMSFT merged commit f92e0ae into release/7.0.4xx Jun 28, 2023
@marcpopMSFT marcpopMSFT deleted the marcpopMSFT-enableunittestproject branch June 28, 2023 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Infrastructure untriaged Request triage from a team member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants