THRIFT-5794: Add tests

[SvenRoederer]

Based on Thrift5794 I worked on improving CI-testing for netstd. As sideeffect I found, that the thrift-exe is not found on windows, which is fixed by commit

• "make MSBuild ignore errors when thrift-binary is not found and continue search"
• "include .exe extension, when looking for compiler"

To address Thrift5794 this PR switches to use default C# languagelevel for the Thrift.PublicInterfaces.Compile.Tests (dropping "net8" option from calling the compiler). To keep testing with "net8 / C#12" a 2nd project Thrift.PublicInterfaces.Compile-C#12.Tests is added, which keeps using the "net8" option.
The Thrift-files are shared between both projects, to reduce amount of duplicated code.

---

Alternative ways I tried/had in mind:

• generating code for all language-levels inside one project (generate to different folders inside the project): will not work, as it causes double definitions of generated symbols
• having the test-job changing the code to run for all language-levels: feels ugly and hard to follow. Github tests can for sure do this way, but might be pain when testing locally. (something like "for {default|net6|net8} in LANG: sed /var/$LANG/ *.cssproj")

[SvenRoederer]

When using these changes it turns out that the netstd-test failing after removing the "net8" option from calling the compiler

I can see this when runnig localy and a also in GithubActions (using #2998)

Test run for /home/runner/work/thrift/thrift/lib/netstd/Tests/Thrift.Tests/bin/Debug/net8.0/Thrift.Tests.dll (.NETCoreApp,Version=v8.0)
Microsoft (R) Test Execution Command Line Tool Version 17.10.0 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
  Failed Test_Complex_DeepCopy [16 ms]
  Error Message:
   Assert.IsNotNull failed. 
  Stack Trace:
     at Thrift.Tests.DataModel.DeepCopyTests.InitializeInstance(RaceDetails instance, Int32 nesting) in /home/runner/work/thrift/thrift/lib/netstd/Tests/Thrift.Tests/DataModel/DeepCopy.cs:line [90](https://github.com/SvenRoederer/thrift/actions/runs/9959985460/job/27518257532#step:10:91)
   at Thrift.Tests.DataModel.DeepCopyTests.Test_Complex_DeepCopy() in /home/runner/work/thrift/thrift/lib/netstd/Tests/Thrift.Tests/DataModel/DeepCopy.cs:line 36
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)


Failed!  - Failed:     1, Passed:    30, Skipped:     0, Total:    31, Duration: 113 ms - Thrift.Tests.dll (net8.0)

I'm not that deep in C# and test-code at all, to debug this/fix. Maybe this fail should become a new Issue (to be treated by anyone with the power/access to Jira).

[Jens-G]

The idea was not to rewrite the whole thing. The net8 option is very deliberately placed there.

[Jens-G]

To keep testing with "net8 / C#12"

I occasionally think about making the last release the default (currently net8) and putting older versions under a suffix instead. But especially for netstd 2 this would be sort of awkward.

[SvenRoederer]

The idea was not to rewrite the whole thing.

I initially started with a separate thrift-file for that case (f7cda29, 4f252f4), but realized that the Thrift-5794 also applies to the existing sourcefiles. For that reason I came up with that "large change" version. Idea behind it was to use default compiler option in favor of specific ones. And use explicit options only where needed (like the net8/C#12 tests-folder).

I'm aware, that this solution adds more files that need to be maintained, slightly increases complexity but also gives some better test-coverage.

As both version for the test are around for review and @Jens-G prefers the "separate tests-file" version for the project, I'm fine. For that reason I made small commits to allow cherry-picking. I hope you was able to make use of it for PR #3000.

The net8 option is very deliberately placed there.

Is that the reason, which causes the "Test_Complex_DeepCopy" to fail with non net8/net6?

I occasionally think about making the last release the default (currently net8) ...

C#12 by default would break our (legacy) project and we would need to have a "legacy" option when compiling.
But even with net8 by default and other levels as option, a test will be needed, which goes back to above "separate tests-files vs. same tests for all language-levels".

[Jens-G]

The net8 option is very deliberately placed there.

Idea behind it was to use default compiler option in favor of specific ones.

Both net6 and net8 are not simply some arbitrary compiler options. These directly affect some important settings and assumptions, e.g. how to treat nullable references to name just one. You cannot simply switch those options blindly and hope all will be fine. The options should match the target environment or you will indeed "find errors" that don't exist.

Is that the reason, which causes the "Test_Complex_DeepCopy" to fail with non net8/net6?

I'd propose to compare the generated code for each option and/or looking at the code generator.

For that reason I came up with that "large change" version [...] use explicit options only where needed (like the net8/C#12 tests-folder).

I'm fine with adding tests. I'm not fine with "improving" working tests to the point where there are broken.

We encourage to contribute patches, but everybody should be aware that - like in any other FOSS project - patches will be reviewed, change-requested, modified and occasionally this involves many hours of back and forth until it gets finally accepted (or rejected). Some patches get accepted, some debated, some rejected. That's how it works. As a rule of thumb, smaller and more concise patches generally have a better chance of getting through - simply because review is easier.

[...] would need to have a "legacy" option [...]

Exactly what I meant by "awkward".

[SvenRoederer]

I'm aware of the principle of code-changes for open projects. There is a change-idea, that gets written down and discussed by project members.
I wrote my change-idea ("use existing test-cases for more language-levels, instead of adding another test-file") as code, that looked working to me and proposed up for discussion. As I had the code changes ready it was easy for me to publish them via this PR and to have something to look at, that just discussing on a pure idea of how to change things. If the change does not fit the projects intention, then it is this way ...

I'd propose to compare the generated code for each option and/or looking at the code generator.

From this comment I see, that it's not surprising that the "Test_Complex_DeepCopy" will fail when not using net8. As @Jens-G is more deep in the project and code, I'm fine with this and consider this seen test-fail as "Works as designed".

[SvenRoederer]

But look like Github PR #3008 / THRIFT-5798 is picking up the idea of this PR with some more large changes, that I tries to avoid initially.