Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

@jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Mar 26, 2019

Redesign tests that needed to modify Core_Root to no longer do so.

  • Loader.FromNativePaths: Use CORE_LIBRARIES instead of COREROOT
  • Loader.AssemblyDependencyResolver: Split out the invalid hosting test that needs hostpolicy to not be preloaded. Have CoreRun and CoreShim preload hostpolicy when a path to a hostpolicy is specified in the MOCK_HOSTPOLICY environment variable.
  • Add a CLRTest.MockHosting.targets file that is imported when a test requires hostpolicy. It adds the reference to the mock hostpolicy and ensures that the MOCK_HOSTPOLICY environment variable is correctly set in the test scripts.

Old description:

For tests that need to modify Core_Root during execution, copy the Core_Root folder to a subfolder named Local_Core_Root and run from there. This is to avoid trying to write to Core_Root (which is read-only on some Helix agents) and avoid the possibility of non-determinism between tests.

Fixes #23429.

cc: @AaronRobinsonMSFT

…re_Root folder to a subfolder named Local_Core_Root and run from there.
@echesakov
Copy link

/azp run coreclr-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkoritzinsky
Copy link
Member Author

/azp run coreclr-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AaronRobinsonMSFT
Copy link
Member

In general I am not opposed to this, but I did some reworking to avoid modifying Core_Root for COM tests. What is the current reason this is needed?

@jkoritzinsky
Copy link
Member Author

The hostpolicy mock needs to be placed next to System.Private.CoreLib as I understand it so System.Private.CoreLib can P/Invoke into it (since we limit its search paths to its directory and the system search paths). If that's not the case and hostpolicy can live next to the test app, then we can do that instead.

@AaronRobinsonMSFT
Copy link
Member

since we limit its search paths to its directory

See https://github.com/dotnet/coreclr/tree/master/tests/src/Interop/COM/Activator for an example of a test using the mocked out hostpolicy in the test directory. This could also be due to the work I did in CoreShim, which should be applied to CoreRun if that is also needed there.

@AaronRobinsonMSFT
Copy link
Member

The gist here is we shouldn't be modifying Core_Root in any way because it will break any chance we have for the possibility of running tests in parallel. Making a copy is a reasonable compromise, but that should be an absolute last resort.

@jkoritzinsky
Copy link
Member Author

So based on @AaronRobinsonMSFT's feedback, we don't need a local Core_Root for the COM tests.

However, I don't believe we can unconditionally enable CoreRun to load hostpolicy.dll if present for a few reasons:

  • I don't know if Unix shared object loading works the same way as Windows, where you can effectively intercept a DLL load by pre-loading a dll with the same name. If they don't have the same behavior, then we have no way to correctly pre-load a hostpoilcy since it needs to live in the right location. Although, we might be able to use LD_PRELOAD to forcibly load the correct .so beforehand.

  • The AssemblyDependencyResolverTests suite includes a test for the behavior for when hostpolicy isn't found. As a result, we can't preload hostpolicy since it would be found when the test is assuming that it won't be. If we remove this test verifying the missing-hostpolicy behavior, then we could pre-load hostpolicy here if we wanted.

  • The FromNativePaths test verifies that a native library can be loaded from a path given by the "NATIVE_DLL_SEARCH_DIRECTORIES" configuration parameter passed to the runtime, which CoreRun sets to its current directory and also the directory pointed to by the CORE_LIBRARIES environment variable. It does not use hostpolicy, so pre-loading hostpolicy won't help here. We could intercept the CORE_LIBRARIES environment variable and modify it in the script before starting the test.

@jkoritzinsky jkoritzinsky changed the title Copy Core_Root locally for tests that modify Core_Root Change tests to not modify Core_Root Mar 29, 2019
@jkoritzinsky
Copy link
Member Author

I've updated the tests to not need to modify Core_Root and load hostpolicy differently to ensure we do everything correct.

@jkoritzinsky
Copy link
Member Author

/azp run coreclr-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkoritzinsky
Copy link
Member Author

jkoritzinsky commented Mar 29, 2019

Test failures are due to #23534 as far as I can tell.

@jkoritzinsky
Copy link
Member Author

/azp run coreclr-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkoritzinsky
Copy link
Member Author

/azp run coreclr-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@echesakov
Copy link

/azp run coreclr-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkoritzinsky
Copy link
Member Author

@echesakovMSFT test failures in Pri1 Windows x86 look unrelated to this PR and it passed on Linux arm64.

@jkoritzinsky jkoritzinsky merged commit 2516a53 into dotnet:master Apr 2, 2019
@jkoritzinsky jkoritzinsky deleted the local-core-root branch April 2, 2019 23:39
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Redesign tests that needed to modify Core_Root to no longer do so.

- Loader.FromNativePaths: Use `CORE_LIBRARIES` instead of `COREROOT`
- Loader.AssemblyDependencyResolver: Split out the invalid hosting test that needs hostpolicy to not be preloaded. Have `CoreRun` and `CoreShim` preload hostpolicy when a path to a hostpolicy is specified in the `MOCK_HOSTPOLICY` environment variable.
- Add a `CLRTest.MockHosting.targets` file that is imported when a test requires hostpolicy. It adds the reference to the mock hostpolicy and ensures that the `MOCK_HOSTPOLICY` environment variable is correctly set in the test scripts.

Fixes dotnet/coreclr#23429.



Commit migrated from dotnet/coreclr@2516a53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants