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

Breaking API between 2.6.6 and 2.7.0 #2896

Closed
mattleibow opened this issue Mar 13, 2024 · 7 comments
Closed

Breaking API between 2.6.6 and 2.7.0 #2896

mattleibow opened this issue Mar 13, 2024 · 7 comments

Comments

@mattleibow
Copy link

This PR added a new default parameter to the ConfigReader API, so now it technically is different:

5ef7b75#diff-54b338f43759528f47a9544e61b807273f8442e7ec4d16a6df237dddf4bce126R43

Not sure if this was intentional or should have been an overload?

@bradwilson
Copy link
Member

We don't consider this to be an extensible library, so binary breaking changes aren't something we generally care about. Upgrading packages and getting compiler errors (or compatible compilations that call methods with different signatures) are to be expected.

What is the scenario where you've discovered this?

@mattleibow
Copy link
Author

This appeared in both my fork of the devices.xunit as well as the xharness project. It was build against pre-2.7.0 and is basically an on-device test runner. To do this, we need to read the config options for assemblies - just like you would for the IDE or something.

https://github.com/mattleibow/DeviceRunners/blob/b75240601afefcb72ed216c021d750b538c02da5/src/DeviceRunners.VisualRunners.Xunit/XunitTestDiscoverer.cs#L71-L78

So basically, if people are using my library and update to a new xunit, this will throw. But, I suppose since this is quite a non-user thing I can also just update. But I just hit the issue where the xharness I am using is pre-2.7, this means I need to wait for them to update before updating.

But since we are both very close to hte internals, this is maybe OK. I just wanted to mention and see if this was known change.

@bradwilson
Copy link
Member

if people are using my library and update to a new xunit, this will throw

This is in xunit.runner.utility which end users will not be updating.

The architecture of xUnit.net is such that there is a very strong line drawn between test authors (who bring in xunit.core) and runner authors (who bring in xunit.runner.utility). The only common assembly between these is xunit.abstractions. Under no circumstances should a test author updating xunit.core (typically because they update xunit) have any impact on xunit.runner.utility or any third party runners.

The only exception would be you, who probably use use xunit to write tests against your xunit.runner.utility-dependent code. But you're not forced to upgrade xunit.runner.utility and xunit at the same time. Their code is 100% independent.

@bradwilson
Copy link
Member

(Also, I will add the old signatures back, with [Obsolete].)

@bradwilson
Copy link
Member

I was able to add the APIs back and hide them from Intellisense which should stop the breaking backward compatibility. 4b2b9fa

Available in 2.7.1-pre.8 https://xunit.net/docs/using-ci-builds

@bradwilson
Copy link
Member

bradwilson commented Mar 15, 2024

BTW, this is a huge red flag:

https://github.com/mattleibow/DeviceRunners/blob/b75240601afefcb72ed216c021d750b538c02da5/src/DeviceRunners.VisualRunners.Xunit/DeviceRunners.VisualRunners.Xunit.csproj#L10-L11

    <PackageReference Include="xunit" Version="2.6.6" />
    <PackageReference Include="xunit.runner.utility" Version="2.6.6" />

Nothing that ships should ever link against both of these things. You're merging two worlds that aren't supposed to be merged. I understand now why your users are having issues. I don't know anything about what you're doing, but IMO you should undo this architecture immediately.

@bradwilson
Copy link
Member

Shipping in 2.7.1 later today.

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

No branches or pull requests

2 participants