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

Replace Specflow.Internal.Json with System.Text.Json #373

Merged
merged 9 commits into from
Jan 10, 2025
Merged

Replace Specflow.Internal.Json with System.Text.Json #373

merged 9 commits into from
Jan 10, 2025

Conversation

obligaron
Copy link
Contributor

@obligaron obligaron commented Dec 27, 2024

🤔 What's changed?

Replaced all places where Specflow.Internal.Json is used with System.Text.Json.
Used SourceGenerator when possible to avoid reflection for serialization.

⚡️ What's your motivation?

SpecFlow and with it Specflow.Internal.Json is end-of-life.
With this PR all dependencies to specflow are gone.
Also System.Text.Json supports comments in reqnroll.json.

🏷️ What kind of change is this?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)
  • 🐛 Bug fix (non-breaking change which fixes a defect)

♻️ Anything particular you want feedback on?

Areas where JSON serializer was used:

  • Telemetry messages (AppInsightsEventSerializer)
  • Providing information about bindings to the VS extension (BindingProviderService)
  • Loading reqnroll.json configuration file during runtime (JsonConfigurationLoader)
  • Loading reqnroll.json configuration file during generation (JsonConfigurationLoader)

Notes:

  • SpecFlow.Internal.Json was not 100% JSON compliant, but System.Text.Json is. For example, it could parse boolean "True", but STJ doesn't allow it, or string values are not encoded correctly. This results in a slightly different parsing experience. This probably won't affect (many) users, as these are more edge cases that are unlikely to be used in reqnroll.json.
  • The telemetry logic also used Specflow.Internal.Json. This has also been replaced by STJ. However, there are no unit tests, so additional manual testing is required.
  • The BindingProviderService also used Specflow.Internal.Json. This has also been replaced by STJ. But there aren't many unit tests, and I haven't tested it with the Visual Studio plugin, so additional manual testing is required.

📋 Checklist:

  • I've changed the behaviour of the code
  • Users should know about my change
    • I have added an entry to the "[vNext]" section of the CHANGELOG, linking to this pull request & included my GitHub handle to the release contributors list.
  • manual testing "generation" (compile time) with VS 17.8.3
  • manual testing telemetry

@Code-Grump
Copy link
Contributor

This seems like a wholly good thing: I wouldn't expect users to be affected by expanding the set of supported JSON language features and removing custom libraries in favour of standard components helps our maintainability.

It looks like the specs and system tests are currently failing, including some failing of the code-behind generation.

@gasparnagy
Copy link
Contributor

As far as I remember, in the VS extension we parse back the json with a "standard" json parser, so probably there will be no issues there. But I will double check.

@SabotageAndi
Copy link

I hope for the best that you get this to work.
I had massive problems with .NET Framework MSBuild and the used assemblies there.

As we supported multiple different MSBuild versions back then, the used assembly versions were different for each.

I don't know which VS Versions are supported by Reqnroll, but I would suggest doing a manual test with all of them to see if the MSBuild Task still works for everyone. Don't rely on the automatic tests for this one.

@obligaron
Copy link
Contributor Author

I think additional testing is a good thing for this PR, but I can't do it. On my private machine I don't have the resources to test all supported Visual Studio versions.

Do you have a tip where we could look first? (Compiling in Visual Studio, command line, etc).

On the positive side, since v2.2.0 we already have a transitive dependency on System.Text.Json via Gherkin v30.

It looks like MsBulid in .NET 6 can load System.Text.Json 6.0 without including the assembly. But this caused JIT errors because the SourceGenerator used System.Text.Json 8.0 specific APIs. That's why the SystemTests failed.

So a workaround might be to use only 6.0 specific APIs and avoid the SourceGenerator. But that's not a clean solution, so I tried to provide the correct assemblies in the NuGet package. That way we deploy and run what we specify.

@gasparnagy
Copy link
Contributor

I will do some testing with a VM configured for 17.8. In this list I have found that 17.6 will go out of support in a few days. No info on 17.7, but last time when we had VS problems, people reported issues with 17.8.3 that is one year old, so probably good enough for testing.

@gasparnagy
Copy link
Contributor

Found this: https://endoflife.date/visual-studio
17.7 is end of support, so 17.8 is a good target.

@gasparnagy
Copy link
Contributor

Unfortunately as @SabotageAndi suspected, the things are not that easy. Trying to build on a machine with a clean VS 17.8.3 installation (that installs only .NET 8 by default) I get a compile error:

1>------ Build started: Project: CleanReqnrollProject.Net8, Configuration: Debug Any CPU ------
1>ReqnrollFeatureFiles: Features\Calculator.feature
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error : [Reqnroll] System.IO.FileNotFoundException: Could not load file or assembly 'System.Text.Json, Version=8.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' or one of its dependencies. The system cannot find the file specified.
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error : File name: 'System.Text.Json, Version=8.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error :    at Reqnroll.Configuration.JsonConfig.JsonConfigurationLoader.LoadJson(ReqnrollConfiguration reqnrollConfiguration, String jsonContent)
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error :    at Reqnroll.Configuration.ConfigurationLoader.LoadJson(ReqnrollConfiguration reqnrollConfiguration, String jsonContent)
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error :    at Reqnroll.Configuration.ConfigurationLoader.LoadFromConfigSource(ReqnrollConfiguration reqnrollConfiguration, IReqnrollConfigurationHolder reqnrollConfigurationHolder)
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error :    at Reqnroll.Configuration.ConfigurationLoader.Load(ReqnrollConfiguration reqnrollConfiguration, IReqnrollConfigurationHolder reqnrollConfigurationHolder)
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error :    at Reqnroll.Generator.Configuration.GeneratorConfigurationProvider.LoadConfiguration(ReqnrollConfiguration reqnrollConfiguration, ReqnrollConfigurationHolder reqnrollConfigurationHolder)
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error :    at Reqnroll.Generator.Configuration.GeneratorConfigurationProviderExtensions.LoadConfiguration(IGeneratorConfigurationProvider configurationProvider, ReqnrollConfigurationHolder configurationHolder)
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error :    at Reqnroll.Generator.Project.ProjectReader.ReadReqnrollProject(String projectFilePath, String rootNamespace)
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error : 
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error : WRN: Assembly binding logging is turned OFF.
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error : To enable assembly bind failure logging, set the registry value [HKLM\Software\Microsoft\Fusion!EnableLog] (DWORD) to 1.
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error : Note: There is some performance penalty associated with assembly bind failure logging.
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error : To turn this feature off, remove the registry value [HKLM\Software\Microsoft\Fusion!EnableLog].
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error :
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error : [Reqnroll] System.Exception: Error when reading project file. ---> System.IO.FileNotFoundException: Could not load file or assembly 'System.Text.Json, Version=8.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' or one of its dependencies. The system cannot find the file specified.
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error :    at Reqnroll.Configuration.JsonConfig.JsonConfigurationLoader.LoadJson(ReqnrollConfiguration reqnrollConfiguration, String jsonContent)
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error :    at Reqnroll.Configuration.ConfigurationLoader.LoadJson(ReqnrollConfiguration reqnrollConfiguration, String jsonContent)
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error :    at Reqnroll.Configuration.ConfigurationLoader.LoadFromConfigSource(ReqnrollConfiguration reqnrollConfiguration, IReqnrollConfigurationHolder reqnrollConfigurationHolder)
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error :    at Reqnroll.Configuration.ConfigurationLoader.Load(ReqnrollConfiguration reqnrollConfiguration, IReqnrollConfigurationHolder reqnrollConfigurationHolder)
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error :    at Reqnroll.Generator.Configuration.GeneratorConfigurationProvider.LoadConfiguration(ReqnrollConfiguration reqnrollConfiguration, ReqnrollConfigurationHolder reqnrollConfigurationHolder)
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error :    at Reqnroll.Generator.Configuration.GeneratorConfigurationProviderExtensions.LoadConfiguration(IGeneratorConfigurationProvider configurationProvider, ReqnrollConfigurationHolder configurationHolder)
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error :    at Reqnroll.Generator.Project.ProjectReader.ReadReqnrollProject(String projectFilePath, String rootNamespace)
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error :    --- End of inner exception stack trace ---
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error :    at Reqnroll.Generator.Project.ProjectReader.ReadReqnrollProject(String projectFilePath, String rootNamespace)
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error :    at Reqnroll.Generator.Project.MSBuildProjectReader.LoadReqnrollProjectFromMsBuild(String projectFilePath, String rootNamespace)
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error :    at Reqnroll.Tools.MsBuild.Generation.ReqnrollProjectProvider.GetReqnrollProject()
1>C:\Users\radmin\.nuget\packages\reqnroll.tools.msbuild.generation\2.2.2-local\build\Reqnroll.Tools.MsBuild.Generation.targets(93,5): error :    at Reqnroll.Tools.MsBuild.Generation.GenerateFeatureFileCodeBehindTaskExecutor.Execute()
1>Done building project "CleanReqnrollProject.Net8.csproj" -- FAILED.

looking into it...

@gasparnagy
Copy link
Contributor

gasparnagy commented Jan 9, 2025

with dotnet build (ie gen on .NET 8) it works, but not with msbuild (ie gen on .NET Fw)

once I compile with dotnet build from CLI, the VS extension works fine at least...

@gasparnagy
Copy link
Contributor

It seems that the problem is that the JsonConfigurationLoader is in the Reqnroll project, that is compiled to .NET Standard. In .NET Standard, the assembly version of the System.Text.Json is 8.0.0.0. But then we package the .NET Fw version of the System.Text.Json package where the assembly version is 8.0.0.5. Since .NET Fw makes strict version verification, it tries to load the 8.0.0.0 and can't load 8.0.0.5 instead.

I have an idea to fix it, so hold on.

@gasparnagy
Copy link
Contributor

I have fixed the old VS compilation error with force loading the dependencies from the package tasks/net462 even if the assembly version does not match (basically if the file is there and the framework was not loading it, we load it).
This only affects compilation with .NET Framework (used by VS and "msbuild").
Maybe #392 will make this obsolete, but let's consider that as a separate change.

@gasparnagy gasparnagy merged commit dc1c3e4 into main Jan 10, 2025
5 checks passed
@gasparnagy gasparnagy deleted the json branch January 10, 2025 09: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.

4 participants