DevToys icon indicating copy to clipboard operation
DevToys copied to clipboard

GirCore: Update to 0.5.0

Open badcel opened this issue 9 months ago • 1 comments

Pull request type

Please check the type of change your PR introduces:

  • [ ] Bugfix
  • [ ] New feature or enhancement
  • [ ] UI change (please include screenshot!)
  • [ ] Code style update (formatting, renaming)
  • [ ] Refactoring (no functional changes, no api changes)
  • [ ] Build related changes
  • [ ] Documentation content changes
  • [ ] Internationalization and localization
  • [x] Other (please describe): Update of GirCore dependency to 0.5.0 without a functional change.

What is the current behavior?

Issue Number: N/A

What is the new behavior?

  • No functional change

Other information

Quality check

Before creating this PR:

  • [x] Did you follow the code style guideline as described in CONTRIBUTING.md
  • [x] Did you build the app and test your changes?
  • [ ] Did you check for accessibility? On Windows, you can use Accessibility Insights for this.
  • [ ] Did you verify that the change work in Release build configuration
  • [ ] Did you verify that all unit tests pass
  • [x] If necessary and if possible, did you verify your changes on:
    • [ ] Windows
    • [ ] macOS
    • [x] Linux

badcel avatar May 13 '24 19:05 badcel

The CI error is a bit irritating for me. I executed the unit tests with linux and there were no errors. The tests run fine on the linux runner, too. For me it looks like the macos build is not using GirCore at all. So I wonder why there is an exception in the macos build if GirCore was only added to the DevToys.Linux project.

The stack trace looks like:

 01:53:30 [DBG]   Failed DevToys.UnitTests.Core.AppHelperTests.CheckForUpdate_Failed_Async [18 ms]
  01:53:30 [DBG]   Error Message:
  01:53:30 [DBG]    System.ArgumentNullException : Value cannot be null. (Parameter 'factory')
  01:53:30 [DBG]   Stack Trace:
  01:53:30 [DBG]      at System.ThrowHelper.Throw(String paramName)
  01:53:30 [DBG]    at System.ThrowHelper.ThrowIfNull(Object argument, String paramName)
  01:53:30 [DBG]    at Microsoft.Extensions.Logging.LoggerFactoryExtensions.CreateLogger(ILoggerFactory factory, Type type)
  01:53:30 [DBG]    at DevToys.Api.LoggingExtensions.Log(Type forType) in /Users/runner/work/DevToys/DevToys/src/app/dev/DevToys.Api/Core/LoggingExtensions.cs:line 31
  01:53:30 [DBG]    at DevToys.Core.AppHelper.CheckForUpdateAsync(IWebClientService webClientService, IVersionService versionService, CancellationToken cancellationToken) in /Users/runner/work/DevToys/DevToys/src/app/dev/DevToys.Core/AppHelper.cs:line 138
  01:53:30 [DBG]    at DevToys.UnitTests.Core.AppHelperTests.CheckForUpdate_Failed_Async() in /Users/runner/work/DevToys/DevToys/src/app/tests/DevToys.UnitTests/Core/AppHelperTests.cs:line 182
  01:53:30 [DBG] --- End of stack trace from previous location ---

This messages seems to indicate that the LoggingExtensions.LoggerFactory property is not initialized. Checking it's references it gets initialized in the linux project here.

A corresponding call is missing for the macos version. Could this be a bug in general?

Funny thing is if I execute the failed test AppHelperTests.CheckForUpdate_Failed_Async via Rider it fails for linux, too as the factory is not set in the unit tests either. Another potential bug?

Executing all tests works with rider. This looks like the tests are not really reproduceable, especially as async code is used. I had problems with XUnit and async code before. But this probably requires a deeper look into the root cause of the problem. I don't think my change is related to it.

Do you have any suggestions on how to proceed?

badcel avatar May 14 '24 19:05 badcel

Thank you @badcel ! I didn't have time to test it yet. I see your concern with the CI. I'm aware there's a race condition impacting only MacOS build and haven't found out why yet (I don't reproduce it locally). I re-run the CI and it looks all good now :-)

veler avatar May 17 '24 16:05 veler

No problem take your time I'm in no hurry.

badcel avatar May 17 '24 18:05 badcel

Thank for offering this change, it works like a charm! 😍

veler avatar May 19 '24 04:05 veler

You are welcome 👍. I will slowly create more PRs and keep them in small logical units to make it easy for you to follow along.

badcel avatar May 19 '24 04:05 badcel