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

Merge v1.7 for main #124

Merged
merged 15 commits into from
Dec 31, 2024
Merged

Merge v1.7 for main #124

merged 15 commits into from
Dec 31, 2024

Conversation

AArnott
Copy link
Owner

@AArnott AArnott commented Dec 30, 2024

No description provided.

@AArnott
Copy link
Owner Author

AArnott commented Dec 30, 2024

@siewers, your recent #99 added tests that fail when run against main (which targets xunit v3). At least one reason I see is that the TheoryData<T> class from xunit v3 no longer implements IEnumerable<object[]>.

Would you be willing to investigate and push a branch that fixes this?

@siewers
Copy link
Contributor

siewers commented Dec 30, 2024

@AArnott Sure, I'll get right on it. I'm a bit confused, though. The project is now targeting .NET 9, but the tests still target .NET 8, is that on purpose or a mistake? In Rider, I'm getting warnings about it, so I'm not sure if I should update the tests to target .NET 9 as well?
Nevermind, I think it was just a caching issue in Rider.

@AArnott
Copy link
Owner Author

AArnott commented Dec 30, 2024

Thanks!

@siewers
Copy link
Contributor

siewers commented Dec 30, 2024

I see that xUnit 3 was only released 14 days ago. I haven't worked with version 3 yet, but many things have changed.
Is the plan to break compatibility with xUnit 2? If so, would it make more sense to bump the major version of Xunit.Combinatorial and let #99 merge into version 1.6 that targets xUnit 2?
I can then rework the implementation to target xUnit 3 in a separate branch.

@AArnott
Copy link
Owner Author

AArnott commented Dec 30, 2024

Yes, xunit 3 is new and quite the breaking change.
The main branch, which is the only one that targets xunit 3, already has had a major version bump to 2.0.
Your change went into the v1.7 branch, which targets xunit 2 where it works today.

Please consider this branch (the source branch for this PR) as the one where you can rework your change. Just send a PR that targets v1.7_for_main and I can complete it to fix this PR.

@siewers
Copy link
Contributor

siewers commented Dec 30, 2024

Ah, okay, I think I understand now :)

I'm considering making the implementation only work with TheoryData as that's also the recommended way of working with data sources in xUnit.

That would force the user to derive from TheoryData, but I don't think that's a bad idea.

However, it would change a lot of tests. What do you think about that?

@AArnott
Copy link
Owner Author

AArnott commented Dec 30, 2024

Xunit v3 already puts some breaking change expense on the user. In principle then, I'm OK with requiring source changes for users that upgrade to the main branch of this repo.
And getting in step with the recommendations of xunit v3 is also generally a good thing.
We should definitely support the recommended path.

That said, if we can continue to support the old way, so that the upgrade path for users is easier, that also seems worth doing if it isn't too much trouble. And at the moment, in main, that works. So I'm optimistic we can achieve it.

@siewers
Copy link
Contributor

siewers commented Dec 31, 2024

@AArnott, please review #125
I managed to make the existing behavior work as before. However, it's slightly more challenging to check and get the data from the new TheoryData implementation in xUnit 3 since all the implementations derive from a generic base class.
You may have some ideas on how it can be improved.

In any case, I only changed a single test to report a slightly more informative exception message; all other tests work as before.

siewers and others added 2 commits December 31, 2024 03:39
* Avoid the need for the null forgiveness operator.
* Use `internal` for members of `internal` types.
@AArnott AArnott enabled auto-merge December 31, 2024 03:46
@AArnott AArnott merged commit a05eb25 into main Dec 31, 2024
5 of 6 checks passed
@AArnott AArnott deleted the v1.7_for_main branch December 31, 2024 03:50
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