-
Notifications
You must be signed in to change notification settings - Fork 0
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
test: fix tests #6
Conversation
@@ -191,7 +191,7 @@ internal EnvironmentDetails(BrowserSpecs specs, BrowserStats stats) | |||
DeviceName = !string.IsNullOrWhiteSpace(specs.DeviceName) ? specs.DeviceName : null; ; | |||
Locale = specs.Locale; | |||
OSVersion = !string.IsNullOrWhiteSpace(specs.UAHints?.CalculatedOSVersion) ? specs.UAHints.CalculatedOSVersion : specs.CalculatedOSVersion; | |||
Platform = specs.UAHints.CalculatedPlatform ?? specs.Platform; | |||
Platform = specs.UAHints?.CalculatedPlatform ?? specs.Platform; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IDE also displayed a warning here saying that UAHints
could be null. One of the tests failed due to it being null too.
using Microsoft.Extensions.DependencyInjection; | ||
using Microsoft.VisualStudio.TestTools.UnitTesting; | ||
|
||
namespace Raygun4Net.Tests.Blazor.WebAssembly | ||
namespace Raygun.NetCore.Tests.Blazor.WebAssembly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a recommendation by the IDE, to match the namespace definition with the folder name.
@@ -1,4 +1,5 @@ | |||
{ | |||
"Raygun": { | |||
"ApiKey": "APIKEY" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests failed to initialize because the appsettings.json
was missing this key and so the init method threw an exception.
@@ -36,7 +36,7 @@ internal record RequestDetails | |||
/// The HTTP method used to request the URL (GET, POST, PUT, etc). | |||
/// </summary> | |||
[JsonInclude] | |||
public HttpMethod HttpMethod { get; set; } | |||
public string HttpMethod { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a bit strange, not sure if it was a typo or on purpose. The JSON parser failed trying to convert this. The rest of the code did not complain after the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a high level the changes make sense to me, but GH shows a bunch of build warnings toward the end of the files view.
The warnings are fixed on a different PR. #12 |
* ci: dotnet formatting * simplify job * test break formatting * fix format * dotnet format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me this PR looks overall good and doesn't raise any obvious concern to me (high level review, mind you)
Test were broken for several reasons, some fixes required changing the client code, some fixing the test setup.
JSInterop
mocks was missing, which caused tests to fail.Besides, I need to check how to fix the code formatting with some rules, so it doesn't change each time I modify a file.