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

Fixed CC for in-process console scenarios #4084

Merged
merged 5 commits into from
Oct 25, 2022

Conversation

cvpoienaru
Copy link
Member

@cvpoienaru cvpoienaru commented Oct 21, 2022

In-process vstest.console wrapper didn't implement CC attachment processing methods until now. These methods were implemented based on their counterparts in the out-of-process vstest.console wrapper, as well as by using important bits-and-pieces from the translation layer (which is no longer required since in-proc console removes the need of a translation layer).

Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

Do you have any ticket we could link this PR to?

In term of tests, it would be nice to test:

  • case where token is cancelled
  • case where exception is thrown
  • case where TestRequestManager is null

@Evangelink Evangelink changed the title Fixed CC for in-proces console scenarios Fixed CC for in-process console scenarios Oct 24, 2022
@cvpoienaru
Copy link
Member Author

Do you have any ticket we could link this PR to?

In term of tests, it would be nice to test:

  • case where token is cancelled
  • case where exception is thrown
  • case where TestRequestManager is null

Yes, there's an internal ticket that we can link the PR to, will send it to you offline (and also link the PR).

I agree with the last two test cases, I will write tests for them right away. The first test case though (case where the token is cancelled) is not feasible here, since the token is passed down to the TestRequestManager.ProcessTestRunAttachments and there is the logic that's handling the cancellation (including response propagation). That logic is independently tested in other tests, so we are covered.

@cvpoienaru cvpoienaru enabled auto-merge (squash) October 24, 2022 16:54
Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

Thank you for the nice work @cvpoienaru! :shipit:

@cvpoienaru cvpoienaru merged commit f229b1a into microsoft:main Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants