-
Notifications
You must be signed in to change notification settings - Fork 50
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
feat: breaking change: revive cleanup projects PR, target .NET 6.0 #313
Conversation
* Update test projects from .NET Core 3.1 (out of support) to .NET 6 (LTS) * Remove `<Service Include="{82a7f48d-3b50-4b1e-b82e-3ada8210c358}" />` (see microsoft/vstest#472 (comment)) * Remove `<LangVersion>7.2</LangVersion>` on test projects * Fix `Run_Specifies_Cancellation_Token` which (rightfully) throws when running on .NET 6
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labled with |
4661572 has a test "fix" for a serialization issue that occurs when updating .NET versions. Essentially, the expected string query string in tests looks like (note the
and the actual string looks like (note the
These are the "same" content, but fail string comparisons. I'm not sure how to monkey with our string comparison library or our query generation to get these to be truly identical. @StanleyGoldman, since you originally introduced this comparison mechanism years ago, I'd be very interested to know if you have thoughts! In the meantime, my hacky "fix" is a find/replace that looks for instances of "new{" in the expected string and replaces them with "newobject{" to match the actual strings. I don't love this approach, hence I'm leaving this PR in draft for the time being. @nickfloyd and @0xced, I'm also interested to hear if you have comments on this. |
Assert.Equal(StripWhitespace(expectedString), StripWhitespace(actualString)); | ||
// hacky fix for anonymous types: actual strings give "new" and expected strings give "new object" | ||
expectedString = StripWhitespace(expectedString); | ||
expectedString = expectedString.Replace("new{", "newobject{"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this feels like something is off with serialization. I'm not sure what the proper call would be here. Let me pull this down today and see if I can get a better picture of what might be going on.
Closing in favor of: #321 |
BREAKING CHANGE: target .NET 6.0.
Builds the project on .NET 8. Also makes the following changes of @0xced's from #304:
<Service Include="{82a7f48d-3b50-4b1e-b82e-3ada8210c358}" />
(see Test projects end up with this GUID microsoft/vstest#472 (comment))<LangVersion>7.2</LangVersion>
on test projectsRun_Specifies_Cancellation_Token
which (rightfully) throws when running on .NET 6