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

Revert "Updating TestWindow.Interfaces assembly redirection" #1730

Closed
wants to merge 4 commits into from

Conversation

singhsarab
Copy link
Contributor

Reverts #1718

@singhsarab singhsarab requested a review from AbhitejJohn August 13, 2018 15:06
@@ -19,7 +19,7 @@
</dependentAssembly>
<dependentAssembly>
<assemblyIdentity name="Microsoft.VisualStudio.TestWindow.Interfaces" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
<bindingRedirect oldVersion="11.0.0.0-15.0.0.0" newVersion="16.0.0.0"/>
<bindingRedirect oldVersion="11.0.0.0-14.0.0.0" newVersion="15.0.0.0"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this redirection at all? Can't we remove this entirely?

Copy link
Contributor

Choose a reason for hiding this comment

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

So certain extensions seem to be extending both TestWindow related extensibility points(for which they need TW.Interfaces) and TP in the same assembly which sometimes ends up triggering a load of TW.Interfaces in the testhost process. That starts bombing test discovery/execution. Now devenv.exe has a similar redirection so the TW extension points as such work perfectly fine but discovery/execution through the platform would bomb out without this.

Copy link
Contributor Author

@singhsarab singhsarab Aug 15, 2018

Choose a reason for hiding this comment

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

How do these extensions work in portable scenarios, when VS isn't present?

These extensions should either have this interface shipped along with them or not have this dependency in the adapter least.

In Dev16, I wish to stop supporting these extensions and would like them to react to this.
/cc: @smadala @cltshivash

Copy link
Contributor

Choose a reason for hiding this comment

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

Well they do not support portable scenarios. I've seen C++ extensions - Google Test, Cmake etc having such dependencies. That should be ok as long as we have documentation on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do want to stop doing this, we might want to communicate with the extension authors sooner because these scenarios are blocked in the latest Dev16 bits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you point to an existing bug for this ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@singhsarab
Copy link
Contributor Author

@AbhitejJohn FYI

@singhsarab
Copy link
Contributor Author

singhsarab commented Aug 15, 2018

This already happened with #1731
@AbhitejJohn Can you please reply on Mayank's comment ?

@singhsarab singhsarab closed this Aug 15, 2018
@singhsarab singhsarab deleted the revert-1718-twredirections branch August 15, 2018 18:55
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.

3 participants