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

Fix net462 tests and some async improvements #756

Merged
merged 4 commits into from
Aug 7, 2023

Conversation

borland
Copy link
Contributor

@borland borland commented Aug 4, 2023

Background

We noticed that some of the async/await code in the async BasicRepository was inconsistent.
As part of routine maintenance, I cleaned this up.

In doing so, I discovered that the tests had not been running on the net462 target for quite some time, due to a bug in the NUnit test runner that occurred when the .NET 7 SDK was present on the host machine.
This issue was fixed in September 2022, and upon upgrading the NUnit runner I discovered that a few breakages/incompatibilities had crept into the codebase.

Results

  • Enables Microsoft analyzers CS4014, CA1849, CA2012, CA2016 which help ensure we are handling async/await correctly.
  • Installs the AsyncFixer third-party analyzer for additional safety nets.
  • Updates LangVersion to 8 for Octopus.Client.csproj
    • Note: This should have no effect at runtime for people who simply consume the binary DLL via a Nuget package, it's just at the compiler level
  • Tweaks a couple of places in the code to handle platform differences between .NET Framework and .NET 6
  • Consistently applies ConfigureAwait(false) and async methods in BasicRepository.cs, MixedScopeBaseRepository.cs and UserInvitesRepository

@borland borland changed the title Orion/async improvements Fix net462 tests and some async improvements Aug 4, 2023
@borland borland marked this pull request as ready for review August 4, 2023 04:14
@borland borland requested a review from a team August 4, 2023 04:23
@borland borland self-assigned this Aug 4, 2023
Copy link
Contributor

@adam-mccoy adam-mccoy left a comment

Choose a reason for hiding this comment

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

LGTM. Just a couple of questions.

Comment on lines +11 to +15
#if NETFRAMEWORK
var codeBase = assembly.CodeBase ?? throw new NotSupportedException($"Cannot get codebase for assembly {assembly}");
#else
var codeBase = assembly.Location;
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just use Assembly.Location for both frameworks?

Copy link
Contributor Author

@borland borland Aug 7, 2023

Choose a reason for hiding this comment

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

Probably, but not definitely.

Microsoft's documentation contains this:

Remarks
In .NET 5 and later versions, for bundled assemblies, the value returned is an empty string.

.NET Framework only: If the loaded file was shadow-copied, the location is that of the file after being shadow-copied. To get the location before the file has been shadow-copied, use the CodeBase property.

Out of caution, I chose to leave the code untouched for NETFRAMEWORK.

<PackageReference Include="NSubstitute" Version="4.4.0" />
<PackageReference Include="NUnit" Version="3.13.3" />
<PackageReference Include="NUnit3TestAdapter" Version="4.5.0" />
<PackageReference Include="System.IO.Compression" Version="4.3.0" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the dependency on System.IO.Compression added intentionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the newer version of NUnit3TestAdapter takes a dependency on it, and it's not built into the BCL in NETFRAMEWORK

@borland borland merged commit 3e668bd into master Aug 7, 2023
31 checks passed
@borland borland deleted the orion/async-improvements branch August 7, 2023 00:48
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.

2 participants