You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The recent PR #94868 has broken local running of libraries tests. After each test suite is completed, the following is printed to console and the test suite is reported as failed:
XUnitLogChecker.dll does not exist in the expected location: \XUnitLogChecker.dll
Looking at the eng/testing/RunnerTemplate.cmd, I can see that it builds the XUnitLogChecker.dll path like this: set XUNITLOGCHECKER_DLL=%HELIX_CORRELATION_PAYLOAD%\XUnitLogChecker.dll
My guess is the problem is that the HELIX_CORRELATION_PAYLOAD is not set for local testing.
@carlossanlop I ran into this today (also in local testing).
Do you really need to have a variable __IsXUnitLogCheckerSupported? You could presumably skip all this stuff if HELIX_CORRELATION_PAYLOAD is not defined, as Jan suggested above.
Also, why does the test script RunTests.cmd need set __TestArchitecture=x86? Who uses this information? The RunTests.cmd script should be independent of processor architecture. The problem now is that I use the same RunTests.cmd script for both x86 and x64 test runs (they take a -r parameter to the Core_Root that will be used, and I can pass either an x86 or x64 Core_Root. But now there is an architecture hard-coded into the RunTests.cmd script).
The problem now is that I use the same RunTests.cmd script for both x86 and x64 test runs (they take a -r parameter to the Core_Root that will be used, and I can pass either an x86 or x64 Core_Root. But now there is an architecture hard-coded into the RunTests.cmd script).
Well, the value is not hardcoded. It is set in these 3 places:
__TestArchitecture should not be set in the generated RunTests.cmd. If I build x86, then build x64, the generated RunTests.cmd will be overwritten with a new test architecture setting. This is incorrect.
Note that in the CoreCLR tests, the __TestArchitecture variable is only set on Helix. It is not set in any generated wrapper file used by developers on local machines. Libraries testing should do the same.
Similarly, set __IsXUnitLogCheckerSupported=1 should not be added to the generated RunTests.cmd, even if you do add code to skip it if the HELIX variable is not set.
Note that __TestArchitecture is only used to choose a version of cdb.exe to use for dump file processing. This isn't going to work on local machines anyway, since it depends on hard-coded cdb.exe install path (and I, for instance, don't have one at that location). It seems like a hack to set an environment variable to figure this out. Couldn't the wrapper script detect the architecture run type? @jkoritzinsky ?
(The IsXUnitLogCheckerSupported variable might need to be passed through more layers, and maybe there are additional conditions that need to be checked)
Thanks for submitting a PR. I suppose the main problem reported by this issue has been fixed, so we can close it. We can just track the additional change in the new PR directly.
Activity
carlossanlop commentedon Dec 7, 2023
Thanks for reporting. Can you please tell me the command you were using to run the tests?
janvorli commentedon Dec 7, 2023
As a repro, you can use e.g. these steps locally. The last step runs the tests and you'll see the suites reported as failing:
.\build.cmd clr+libs -c Release -rc Checked.\build.cmd libs.tests -c Release -rc Checked.\build.cmd libs.tests -c Release -rc Checked -test -testnobuildBruceForstall commentedon Jan 2, 2024
@carlossanlop I ran into this today (also in local testing).
Do you really need to have a variable
__IsXUnitLogCheckerSupported? You could presumably skip all this stuff ifHELIX_CORRELATION_PAYLOADis not defined, as Jan suggested above.Also, why does the test script
RunTests.cmdneedset __TestArchitecture=x86? Who uses this information? TheRunTests.cmdscript should be independent of processor architecture. The problem now is that I use the same RunTests.cmd script for both x86 and x64 test runs (they take a-rparameter to the Core_Root that will be used, and I can pass either an x86 or x64 Core_Root. But now there is an architecture hard-coded into the RunTests.cmd script).ericstj commentedon Jan 2, 2024
@carlossanlop I just hit this today as well. My workflow was:
carlossanlop commentedon Jan 8, 2024
Looking at this today.
carlossanlop commentedon Jan 8, 2024
@BruceForstall The debuggers fail to run if the environment variable is not set. It is expected here:
runtime/src/tests/Common/Coreclr.TestWrapper/CoreclrTestWrapperLib.cs
Line 243 in 499e288
Well, the value is not hardcoded. It is set in these 3 places:
CoreCLR tests:
runtime/src/tests/Common/helixpublishwitharcade.proj
Line 712 in 499e288
runtime/src/tests/Common/helixpublishwitharcade.proj
Line 712 in 499e288
Libraries tests:
runtime/eng/testing/tests.targets
Line 78 in 499e288
But with the fix I just submitted, these env vars should not be needed at all in local runs.
BruceForstall commentedon Jan 9, 2024
__TestArchitectureshould not be set in the generatedRunTests.cmd. If I build x86, then build x64, the generated RunTests.cmd will be overwritten with a new test architecture setting. This is incorrect.Note that in the CoreCLR tests, the
__TestArchitecturevariable is only set on Helix. It is not set in any generated wrapper file used by developers on local machines. Libraries testing should do the same.Similarly,
set __IsXUnitLogCheckerSupported=1should not be added to the generated RunTests.cmd, even if you do add code to skip it if the HELIX variable is not set.Note that
__TestArchitectureis only used to choose a version of cdb.exe to use for dump file processing. This isn't going to work on local machines anyway, since it depends on hard-coded cdb.exe install path (and I, for instance, don't have one at that location). It seems like a hack to set an environment variable to figure this out. Couldn't the wrapper script detect the architecture run type? @jkoritzinsky ?carlossanlop commentedon Jan 9, 2024
I merged the PR to unblock folks testing locally. Reopening as there is still some discussion to address related to the OS.
BruceForstall commentedon Jan 9, 2024
@carlossanlop I'm suggesting something like this:
#96713
(The
IsXUnitLogCheckerSupportedvariable might need to be passed through more layers, and maybe there are additional conditions that need to be checked)carlossanlop commentedon Jan 10, 2024
Thanks for submitting a PR. I suppose the main problem reported by this issue has been fixed, so we can close it. We can just track the additional change in the new PR directly.