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

run unit tests in CI against dotnet 8 #306

Merged
merged 25 commits into from
Aug 29, 2024
Merged

run unit tests in CI against dotnet 8 #306

merged 25 commits into from
Aug 29, 2024

Conversation

hahn-kev
Copy link
Contributor

@hahn-kev hahn-kev commented Jul 30, 2024

Our team is planning to use LCM on dotnet 8, so I decided it would be good to run tests against dotnet 8. In doing so I found a few issues with some paths which would cause errors during build or clean.

Prevously RhinoMocks was being used however it does not support modern dotnet, Moq is the suggested replacement and I found a library that works as a shim, so I didn't need to rewrite any of the test code (with one exception).

Lastly I had to make Core and TestHelper target net8.0 also as tests would not get the desired IcuData and other dependancies otherwise.

Actual code changes:

  • FileUtils.AreFilesIdentical now always uses SHA1 as the hash algorithm as modern dotnet throws if you try to delegate to a default algorithm and SHA1 is what was being used in framework
  • AssertValidFilePath and IsFilePathValid had to be rewritten since modern dotnet does not do much if any validation anymore.

There's currently some failing tests due to NHunspell not supporting anything but .Net framework and it's last update was 2015. I suggest we go to https://github.com/aarondandy/WeCantSpell.Hunspell as it's been updated in the last year and supporst .net standard 2.0. However I think that's out of scope for this PR.


This change is Reviewable

…atch a conflict with System.Diagnostics.PerformanceCounter
…ation set so the output path is consistent.
…s that IcuData will be copied to the output directory for dotnet 8 projects that depend on SIL.LCModel.Core
…ncluding Newtonsoft.Json.dll. Use TestContext.Out instead of Console.WriteLine. Fix assertion in CustomIcuFallbackTests.InitIcuDataDir_NoIcuLibrary due to how the error is different between .NET Framework and .NET Core.
Copy link

github-actions bot commented Jul 30, 2024

LCM Tests

    16 files  +    8      16 suites  +8   2m 52s ⏱️ + 1m 17s
 2 826 tests +   13   2 806 ✅ +   13   20 💤 ±  0  0 ❌ ±0 
11 252 runs  +5 626  11 073 ✅ +5 524  179 💤 +102  0 ❌ ±0 

Results for commit f683b1a. ± Comparison against base commit aa682dd.

♻️ This comment has been updated with latest results.

@hahn-kev
Copy link
Contributor Author

@jasonleenaylor CollatorSort_DoesNotThrow is currently throwing in our tests. It looks like it's due to a missmatch between the version of icu.net that SIL.WritingSystems is using and the version we're using. If I were to upgrade SIL.WritingSystems to use icu.net 2.10 would we be able to upgrade to that new WritingSystems package to fix this?

@jasonleenaylor
Copy link
Contributor

@hahn-kev yes, we can do that. I will want to get some other changes merged first so I can delay the corresponding work in FieldWorks until the next sprint.

Copy link
Collaborator

@josephmyers josephmyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good. Thanks for removing the netstandard target for the test projects -- I don't think we have any realistic reason to keep that.

Copy link
Contributor

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 11 files at r1, 3 of 12 files at r2, 7 of 7 files at r3, all commit messages.
Reviewable status: 13 of 23 files reviewed, all discussions resolved


src/SIL.LCModel.Utils/SIL.LCModel.Utils.csproj line 13 at r3 (raw file):

    <PackageReference Include="GitVersion.MsBuild" Version="5.6.10" PrivateAssets="All" />
    <PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.0.0" PrivateAssets="All" />
    <PackageReference Include="SIL.Core" Version="14.2.0-beta0021" />

Here, and elsewhere I'd like to keep the wildcard, I accept the risk of a breaking library change in the same version over constant update maintenance churn. Basically, I trust me/us on our core libraries.


tests/SIL.LCModel.Tests/RhinoMocksToMoq/Expect.cs line 1 at r3 (raw file):

using System;

🤮
We should discuss, this may be the best solution but I prefer to avoid inlining 3rd party code.

@hahn-kev
Copy link
Contributor Author

tests/SIL.LCModel.Tests/RhinoMocksToMoq/Expect.cs line 1 at r3 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

🤮
We should discuss, this may be the best solution but I prefer to avoid inlining 3rd party code.

Yeah I tried using the library but was getting errors due to the Nuget dlls not being signed, so I decided to inline it. I'd be happy to use the package if you know what we should do about that.

@hahn-kev
Copy link
Contributor Author

hahn-kev commented Aug 16, 2024

@jasonleenaylor when I run with the nuget package to sub RhinoMocks for Moq I get this error at runtime in dotnet framework:

  System.IO.FileLoadException : Could not load file or assembly 'RhinoMocksToMoq, Version=0.2.0.0, Culture=neutral, PublicKeyToken=null' or one of its dependencies. A strongly-named assembly is required. (Exception from HRESULT: 0x80131044)

in addition the default configuration of Moq doesn't match the default config of RhinoMocks (automatically setting up all properties) and the nuget replacment does not configure that properly, so we'd have to touch every place that uses MockRepository to get a mock or stub.

If you can figure out the strongly named assembly issue then I'll take the time to get the tests working by making sure setup is called before the test code uses the mock. I've pushed the version that uses the nuget package.

@hahn-kev
Copy link
Contributor Author

@jasonleenaylor I inlined the helper lib for RhinoMocks like we talked about on Friday/Thursday, I'm ready to merge this in. Not sure why that PR check is failing but I don't think it's related to this PR

Copy link
Contributor

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 11 files at r1, 1 of 12 files at r2, 5 of 5 files at r4, 9 of 9 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @hahn-kev)

@hahn-kev hahn-kev merged commit c3b1d64 into master Aug 29, 2024
5 checks passed
@hahn-kev hahn-kev deleted the dotnet8-testing branch August 29, 2024 02:06
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