Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't copy runner to test output during regular builds #96826

Merged
merged 4 commits into from
Jan 22, 2024

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Jan 11, 2024

Fixes #94183

The test runner is not required for local test execution, but is needed for helix (today). In the future we should try to change helix to not expect the runner in the output directory, but instead as a correlation payload.

The test runner is not required for local test execution, but is needed
for helix (today).  In the future we should try to change helix to not
expect the runner in the output directory, but instead as a correlation
payload.
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jan 11, 2024
@ghost ghost assigned ericstj Jan 11, 2024
<None Remove="$(Pkgxunit_runner_visualstudio)\build\netcoreapp2.1\xunit.runner.reporters.netcoreapp10.dll" />
<ItemGroup>
<None Remove="$(Pkgxunit_runner_visualstudio)\build\net462\xunit.abstractions.dll" />
<None Remove="$(Pkgxunit_runner_visualstudio)\build\net6.0\xunit.abstractions.dll" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes every other year and unfortunately msbuild doesn't emit a warning/error when an item remove operation fails. Please add a target that errors out when one of those items aren't found.

Copy link
Member Author

@ericstj ericstj Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this is about to go away anyhow since I see you worked to get the original issue fixed. I refinef this workaround a bit (turns out it is only needed on netfx the netcore props never copied it) until we can pick up a new release of xunit. IMO the bigger issue to fix here is the dotnet test - I just noticed the double write problem was not fixed and corrected it while I was here - I'd like to merge this rather than wait for the new Xunit.

@am11 am11 added area-Infrastructure-libraries and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jan 11, 2024
@ghost
Copy link

ghost commented Jan 11, 2024

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #94183

The test runner is not required for local test execution, but is needed for helix (today). In the future we should try to change helix to not expect the runner in the output directory, but instead as a correlation payload.

Author: ericstj
Assignees: ericstj
Labels:

area-Infrastructure-libraries

Milestone: -

Co-authored-by: Carlos Sánchez López <1175054+carlossanlop@users.noreply.github.com>
@ericstj
Copy link
Member Author

ericstj commented Jan 18, 2024

Note that this PR (as with any double-write fix) will require two consecutive incremental builds or a git clean to get the fix. Sharing that here in case folks see funny behavior after this fix. It had me scratching my head briefly until I realized what was going on - incremental clean was deleting files that I deduplicated.

@ViktorHofer ViktorHofer merged commit 3133abb into dotnet:main Jan 22, 2024
172 of 180 checks passed
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
* Don't copy runner to test output during regular builds

The test runner is not required for local test execution, but is needed
for helix (today).  In the future we should try to change helix to not
expect the runner in the output directory, but instead as a correlation
payload.

* Refine xunit 1651 workaround until we can pick up the fix

* Update eng/testing/xunit/xunit.console.targets

Co-authored-by: Carlos Sánchez López <1175054+carlossanlop@users.noreply.github.com>

* Update xunit.console.targets

---------

Co-authored-by: Carlos Sánchez López <1175054+carlossanlop@users.noreply.github.com>
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Feb 22, 2024
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.

dotnet test is not running tests for .NETFramework targets
4 participants