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

ResultsCommandExtension: Do not dispose the VssConnection #3221

Merged

Conversation

thomasdgx
Copy link
Contributor

Fix #3219

Do not dispose the VssConnection as this seems to impact the underlying socket, which leads to sporadic exception.

If I search through the code base, then it seems that in most cases the connection is not disposed.

@thomasdgx
Copy link
Contributor Author

@anatolybolshakov Is there a way to rerun the checks? For me it looks like that your team did an agent deployment and the latest version was broken. The unit test error seems not to be related to my changes.
Could you also forward to some expert for review? I think this bug needs to be solved urgently.

@anatolybolshakov
Copy link
Contributor

Hi @thomasdgx let us take a look at this PR.
Added label to PR - to pass checks

@thomasdgx
Copy link
Contributor Author

@anatolybolshakov All checks passed now. Is there anything I need to do? Will your team accept the changes and merge them? Can you please share some timeline estimation, as I am not sure about your internal procedures. I could imagine that you integrate external changes only once per sprint.

@anatolybolshakov anatolybolshakov requested a review from a team February 5, 2021 15:10
@thomasdgx
Copy link
Contributor Author

@anatolybolshakov Hi Anatoly,
how can I help to get the story done? Anything left I need to do?
Ciao
Thomas

Copy link
Contributor

@mjroghelia mjroghelia left a comment

Choose a reason for hiding this comment

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

@thomasdgx Thanks for digging into this.

@anatolybolshakov I'm going to merge this. The earlier PR @thomasdgx referenced (#2811) was based on static analysis and there were a few of the using statements that had to be reverted after it was merged.

@mjroghelia mjroghelia merged commit 7db4d83 into microsoft:master Feb 22, 2021
@thomasdgx thomasdgx deleted the users/thomasdgx/FixPublishTestResults branch February 23, 2021 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PublishTestResults sporadically fails to publish all results
3 participants