Skip to content

Bump xunit and related fixes - #350

Merged
sungam3r merged 10 commits into
masterfrom
bump-xunit
Oct 12, 2023
Merged

sungam3r merged 10 commits into
masterfrom
bump-xunit

Conversation

@sungam3r

@sungam3r sungam3r commented Oct 11, 2023 •

Copy link
Copy Markdown
Member

I suggest to say goodbye for netcoreapp3.1 and net5.0 since the-latest-and-greatest xunit does not support these TFMs, see #344 and #345.

изображение

@sungam3r
sungam3r requested a review from Shane32 October 11, 2023 06:56
@sungam3r sungam3r self-assigned this Oct 11, 2023
@github-actions github-actions Bot added the test Pull request that adds new or changes existing tests label Oct 11, 2023
Comment thread src/GraphQLParser.Tests/Visitors/ASTVisitorTests.cs
Comment thread src/GraphQLParser.Tests/Visitors/CountVisitorTests.cs
@Shane32

Shane32 commented Oct 11, 2023

Copy link
Copy Markdown
Member

I suggest to say goodbye for netcoreapp3.1 and net5.0 since the-latest-and-greatest xunit does not support these TFMs, see #344 and #345.

изображение

I still use older TFMs in various applications and it does not hurt us to test for them. If you want to bump xunit, just make the dependency conditional on the tfm and use the older version for 3.1/5.0.

@Shane32

Shane32 commented Oct 11, 2023

Copy link
Copy Markdown
Member

This project doesn't target .NET 3.1 or .NET 5. So I'm fine with merging this PR as-is and dropping testing against those platforms.

@Shane32

Shane32 commented Oct 11, 2023

Copy link
Copy Markdown
Member

We should drop 3.1/5.0 from the GitHub Action scripts, however.

@Shane32 Shane32 added the CI CI configuration issue or pull request label Oct 11, 2023
@Shane32

Shane32 commented Oct 11, 2023 •

Copy link
Copy Markdown
Member

Note that if we do drop .NET 3.1/5.0 testing, we implicitly drop .NET Standard 2.1 testing. 👎 Only the net6 TFM gets tested

@Shane32

Shane32 commented Oct 11, 2023

Copy link
Copy Markdown
Member

And it seems that we don't test against the .NET Framework currently (a supported TFM), which means that we are not testing the .NET Standard 2.0 build. 👎

@sungam3r

Copy link
Copy Markdown
Member Author

True. Is it worth to?

@sungam3r

Copy link
Copy Markdown
Member Author

We should drop 3.1/5.0 from the GitHub Action scripts, however.

Missed it. Removed. Regarding .NET Framework / netstandard2.0 tests - I'm 50/50.

@Shane32

Shane32 commented Oct 11, 2023

Copy link
Copy Markdown
Member

If we have have different builds for .NET Standard 2.0/2.1 we should test against it. It's not like it costs any dev time and CI testing is free. It would be nice if we could test each of the 3 builds against .NET 6, but that would likely be quite a bit more complicated than simply testing .NET 4.8, .NET 3.1 and .NET 6.0.

I certainly have GraphQL applications that run on .NET Framework under a fully supported scenario, as I expect others do...

@Shane32

Shane32 commented Oct 11, 2023

Copy link
Copy Markdown
Member

Arguably we could remove the .NET Standard 2.1 TFM from the codebase, using .NET Standard 2.0 as a fallback for older TFMs. I'm not prepared to do so at this time.

@sungam3r

Copy link
Copy Markdown
Member Author

OK, I'll deal with this tomorrow making dependency conditional.

@Shane32

Shane32 commented Oct 11, 2023

Copy link
Copy Markdown
Member

Arguably we could remove the .NET Standard 2.1 TFM from the codebase, using .NET Standard 2.0 as a fallback for older TFMs. I'm not prepared to do so at this time.

I'll also review this. Perhaps there is little practical benefit to having a .NET Standard 2.1 TFM.

@Shane32

Shane32 commented Oct 11, 2023

Copy link
Copy Markdown
Member

Looking at the codebase, there isn't any code that wouldn't be exercised if we tested on both .NET Framework 4.8 and .NET 6, omitting .NET 3.1. Again, I'd probably just run it on 3.1 anyway, but if we do eliminate it, at least all the code paths get tested.

I wouldn't eliminate the .NET Standard 2.1 TFM. For one thing, it eliminates some unnecessary dependencies.

@Shane32

Shane32 commented Oct 11, 2023

Copy link
Copy Markdown
Member

We don't need to test against .NET 5. It isn't a LTM release, and .NET 3.1 testing will cover the .NET Standard 2.1 binary.

# Conflicts:
#	src/GraphQLParser/Visitors/SDLPrinterExtensions.cs
Comment thread src/GraphQLParser.Tests/Visitors/SDLPrinterFromParsedTextTests.cs
Comment thread src/GraphQLParser/Visitors/SDLPrinterExtensions.cs Outdated
Comment thread src/GraphQLParser/GraphQLParser.csproj Outdated
@sungam3r sungam3r added the bugfix Pull request that fixes a bug label Oct 12, 2023
@sungam3r

Copy link
Copy Markdown
Member Author

Hah.

Test run for /home/runner/work/parser/parser/src/GraphQLParser.Tests/bin/Debug/net462/GraphQLParser.Tests.dll (.NETFramework,Version=v4.6.2)
Microsoft (R) Test Execution Command Line Tool Version 17.7.1 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:02.58] GraphQLParser.Tests: Catastrophic failure: System.TypeLoadException: Could not load type of field 'Xunit.Runner.VisualStudio.VsExecutionSink:recorder' (4) due to: Could not load file or assembly 'Microsoft.VisualStudio.TestPlatform.ObjectModel, Version=15.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies.

@sungam3r

Copy link
Copy Markdown
Member Author

microsoft/vstest#2469

@sungam3r
sungam3r merged commit b11c759 into master Oct 12, 2023
@sungam3r
sungam3r deleted the bump-xunit branch October 12, 2023 16:09
@sungam3r

Copy link
Copy Markdown
Member Author

9.2.1 ?

@Shane32

Shane32 commented Oct 12, 2023

Copy link
Copy Markdown
Member

No, 8.4.1 from master. You can merge master to develop and then release 9.2.1 from develop.

@Shane32

Shane32 commented Oct 12, 2023

Copy link
Copy Markdown
Member

Most people use the version referenced by GraphQL, which is still on v8. After release we should bump the parser dependency to 8.4.1 in GraphQL's master branch.

@Shane32

Shane32 commented Oct 12, 2023

Copy link
Copy Markdown
Member

Most all of the changes to 9.x I've backported to 8.x. The only real difference now is that 9.x has removed a bunch of constructors. Current versions of GraphQL.NET don't use those obsolete constructors anymore, so it is compatible with Parser 9, but maintains a dependency to 8.x so there are no binary breaking changes within 7.x (either by GraphQL.NET or GraphQL.NET Parser, its dependency).

@sungam3r

Copy link
Copy Markdown
Member Author

I'm confused a bit. I just merged master into develop (actually fast-forwarded develop) and now master and develop both point to the same commit. What version to release - 8.4.1 or 9.2.1 ?

@sungam3r

Copy link
Copy Markdown
Member Author

I'm done for today. You may go on and make release if you wish.

@Shane32

Shane32 commented Oct 12, 2023

Copy link
Copy Markdown
Member

Oh. Sorry, we are using the v8 branch for v8 and the master branch for v9

@Shane32

Shane32 commented Oct 12, 2023

Copy link
Copy Markdown
Member

(It's been that way since you released v9.) No matter. I'll cherry pick the commits to the v8 branch and issue releases.

Shane32 pushed a commit that referenced this pull request Oct 12, 2023
Shane32 pushed a commit that referenced this pull request Oct 12, 2023
@Shane32

Shane32 commented Oct 12, 2023

Copy link
Copy Markdown
Member

I changed the default branch to v8 so we don't make this mistake again.

@sungam3r

Copy link
Copy Markdown
Member Author

Ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Pull request that fixes a bug CI CI configuration issue or pull request test Pull request that adds new or changes existing tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants