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

Disable test parallelization in installer tests #58604

Closed
wants to merge 1 commit into from

Conversation

agocke
Copy link
Member

@agocke agocke commented Sep 3, 2021

Should help #53587

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-HostModel Microsoft.NET.HostModel issues label Sep 3, 2021
@ghost
Copy link

ghost commented Sep 3, 2021

Tagging subscribers to this area: @vitek-karas, @agocke
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: agocke
Assignees: -
Labels:

area-HostModel

Milestone: -

@agocke agocke marked this pull request as ready for review September 3, 2021 18:27
@agocke agocke requested a review from mateoatr September 4, 2021 05:51
private TestProject CopyTestProject(TestProject sourceTestProject)
{
// This can race with the filesystem. If we're running this code from two threads, fail fast
Copy link
Member

Choose a reason for hiding this comment

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

Even with test parallelization disabled, does xunit guarantee it'll always run tests on the same single thread? I'd have expected it'd be valid for it to run tests on different threads, just serialized.

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't guarantee it as far as I can tell, but it happens to work for now.

I'm not particularly pleased with this solution, but it's the closest thing I can find to a workaround that will scream if something goes wrong.

Long term plan needs to be to make these tests thread safe

Copy link
Member Author

Choose a reason for hiding this comment

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

@stephentoub Any suggestions? If this starts failing I can remove the lock

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this fails when xunit is allowed to run concurrently and whether the tests really touch the same files.

Copy link
Member

Choose a reason for hiding this comment

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

I am concerned that we are not dealing with concurrent use of the same file issue. If we do not realy have that, then change may have effect, but only because it makes tests to run slower or smth and would only reduce failures, not eliminate them.

Copy link
Member

Choose a reason for hiding this comment

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

What I suspect is the root cause is that in rare cases the file system has not finished writing the executable and we already try executing it. I do not know though how to prove this or how to prevent this from happening.

Copy link
Member

Choose a reason for hiding this comment

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

What I find suspicious is that bundle tests are not the only tests which write executables and then try to run them immediately after. Lot of the hosting tests do this - they either build the app and immediately run it, or in some cases we take a built app, patch the apphost and then run it (patching it to enable some test features in the product).

The only main difference between the hosting tests and the bundle tests is the size of the executable. Hosting tests use the standard apphost (150KB), while the bundle tests create large single-file executables (60MB+).

So it is definitely possible that the file system didn't really finish writing it yet. Or there's some other thing (like AV) taking longer due to the size of the file. It's also interesting that pretty much all the failures are on Linux.

@agocke agocke requested a review from VSadov September 8, 2021 19:33
@agocke
Copy link
Member Author

agocke commented Oct 4, 2021

Decided to not go forward with this, found other fixes instead.

@agocke agocke closed this Oct 4, 2021
@agocke agocke deleted the keep-test-files branch October 4, 2021 20:10
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-HostModel Microsoft.NET.HostModel issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants