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 test session issues #4075

Merged
merged 4 commits into from
Oct 19, 2022

Conversation

cvpoienaru
Copy link
Member

Description

This PR fixes a couple of issues related to test sessions:

  • Spawning a test session when either the runsettings or the test sources are incompatible with test session criteria (e.g.: the use of Fakes or any other data collector);
  • Aborting test discovery/run operations because of an unhandled exception when returning a non-existent proxy, caused by an earlier incompatibility between the session criteria when the session was spawned and the current operation criteria. This scenario was taken into account before and the solution was to spawn an on-demand testhost that meets the discovery/run criteria, but it wasn't properly tested;
  • During the abort scenario described above, a testhost Close call is badly handled and has the effect of leaving on-demand testhosts hanging indefinitely. This issue has been addressed by the current PR, too;

@cvpoienaru cvpoienaru enabled auto-merge (squash) October 17, 2022 16:19
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.

A few comments and nits. I am not asking for tests as you told me that you looked into it and it's not easy to add them because of current architecture.

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.

LGTM, might be worth asking for a second opinion.

@cvpoienaru
Copy link
Member Author

To be honest, I'd rather just merge since the PR has been up for review since Monday and nobody else weighed in.

@cvpoienaru cvpoienaru merged commit ca6e3a8 into microsoft:main Oct 19, 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