Skip to content

Try synchronizing dispose and output #20341

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 1, 2020
Merged

Try synchronizing dispose and output #20341

merged 2 commits into from
Apr 1, 2020

Conversation

JunTaoLuo
Copy link
Contributor

Try resolving #20336 with more synchronization.

JunTaoLuo
JunTaoLuo commented Mar 31, 2020
View reviewed changes
src/Grpc/test/InteropTests/InteropTests.cs Outdated
@@ -27,6 +27,7 @@ public InteropTests(ITestOutputHelper output)
}

[Theory]
[Repeat]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Runs 10 times which should take an extra 20 mins.

Copy link
Member

Choose a reason for hiding this comment

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

You'll run the PR build a few times with this and then remove it before checking in, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the plan.

@davidfowl
Copy link
Member

Didn't we have a PR that fixed this by booting up the server per test.?

@JunTaoLuo
Copy link
Contributor Author

Yea last time I saw

Attachments:
  /private/tmp/helix/working/AEED0942/w/A8390952/e/TestResults/5d20590f-dca0-40a5-9110-acab59d28ae2/Sequence_47afb6e8ca4d447884f4db3f416ebf35.xml
Test Run Aborted.
Total tests: Unknown
     Passed: 13
 Total time: 1.6884 Minutes

So I thought the server was logging in between tests, which I still think occured, which is why I converted to booting and shutting down a server per test. However, today @Tratcher pointed to a build that still failed and after looking at it, it seems like the server was logging after shutting down which can also cause this error. I've also added Repeat so if this is still flaky, we'll know sooner.

TL:DR my fix was insufficient. Hopefully this makes it more reliable.

@JunTaoLuo JunTaoLuo marked this pull request as ready for review March 31, 2020 03:20
@JamesNK
Copy link
Member

JamesNK commented Mar 31, 2020

What about explicitly unregistering test output logging from the processes in dispose?

@Pilchie Pilchie added the area-grpc Includes: GRPC wire-up, templates label Mar 31, 2020
@Tratcher
Copy link
Member

What about explicitly unregistering test output logging from the processes in dispose?

ProcessEx already does that, but still seems to be hitting a race.

@Tratcher Tratcher removed their assignment Mar 31, 2020
@Tratcher Tratcher self-requested a review March 31, 2020 17:19
Tratcher
Tratcher approved these changes Mar 31, 2020
View reviewed changes
src/Grpc/test/InteropTests/InteropTests.cs Outdated
@@ -27,6 +27,7 @@ public InteropTests(ITestOutputHelper output)
}

[Theory]
[Repeat]
Copy link
Member

Choose a reason for hiding this comment

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

You'll run the PR build a few times with this and then remove it before checking in, right?

@JunTaoLuo
Copy link
Contributor Author

/azp run Tests: Helix x64

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@JunTaoLuo
Copy link
Contributor Author

/azp run aspnetcore-ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JunTaoLuo
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@JunTaoLuo
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@JunTaoLuo
Copy link
Contributor Author

40 runs no failure, I think that's sufficient to declare this battle won.

@JunTaoLuo JunTaoLuo merged commit 71327c0 into master Apr 1, 2020
@JunTaoLuo JunTaoLuo deleted the johluo/racy-interop-tests branch April 1, 2020 17:03
@JunTaoLuo
Copy link
Contributor Author

@anurse should test flakiness improvements like this go into preview3?

@JunTaoLuo JunTaoLuo mentioned this pull request Apr 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tratcher Tratcher Tratcher approved these changes

Assignees
No one assigned
Labels
area-grpc Includes: GRPC wire-up, templates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JunTaoLuo @davidfowl @JamesNK @Tratcher @Pilchie