-
Notifications
You must be signed in to change notification settings - Fork 141
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
Replace Moq with NSubstitute #1903
Conversation
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.
Some adjustments and rework of the Protected
trick
Tests/SonarScanner.MSBuild.PreProcessor.Test/AnalysisConfigGeneratorTests.cs
Show resolved
Hide resolved
Tests/SonarScanner.MSBuild.PreProcessor.Test/SonarCloudWebServerTest.cs
Outdated
Show resolved
Hide resolved
Tests/SonarScanner.MSBuild.PreProcessor.Test/SonarCloudWebServerTest.cs
Outdated
Show resolved
Hide resolved
Tests/SonarScanner.MSBuild.PreProcessor.Test/SonarCloudWebServerTest.cs
Outdated
Show resolved
Hide resolved
Tests/SonarScanner.MSBuild.PreProcessor.Test/SonarCloudWebServerTest.cs
Outdated
Show resolved
Hide resolved
Tests/SonarScanner.MSBuild.PreProcessor.Test/TargetsInstallerTests.cs
Outdated
Show resolved
Hide resolved
Tests/SonarScanner.MSBuild.PreProcessor.Test/TargetsInstallerTests.cs
Outdated
Show resolved
Hide resolved
Tests/SonarScanner.MSBuild.PreProcessor.Test/TargetsInstallerTests.cs
Outdated
Show resolved
Hide resolved
Tests/SonarScanner.MSBuild.Shim.Test/PropertiesFileGeneratorTests.cs
Outdated
Show resolved
Hide resolved
Tests/SonarScanner.MSBuild.TFS.Test/CoverageReportProcessorTests.cs
Outdated
Show resolved
Hide resolved
83d8555
to
1f0a4a8
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s), but failed to run 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s), but failed to run 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s), but failed to run 1 pipeline(s). |
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.
One more round on the handler mock
Tests/SonarScanner.MSBuild.PreProcessor.Test/Infrastructure/HttpMessageHandlerMock.cs
Outdated
Show resolved
Hide resolved
Tests/SonarScanner.MSBuild.PreProcessor.Test/Infrastructure/HttpMessageHandlerMock.cs
Outdated
Show resolved
Hide resolved
Tests/SonarScanner.MSBuild.PreProcessor.Test/Infrastructure/HttpMessageHandlerMock.cs
Outdated
Show resolved
Hide resolved
Tests/SonarScanner.MSBuild.PreProcessor.Test/Infrastructure/HttpMessageHandlerMock.cs
Outdated
Show resolved
Hide resolved
new(async (request, cancellationToken) => request.RequestUri switch | ||
{ | ||
var url when url == new Uri(cacheFullUrl) => new HttpResponseMessage | ||
{ |
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.
What about making there a fuent API like
new HttpMessageHandlerMock()
.AddResponse(URL, new StringContent(...)),
.AddResponse(URL, ....)
and then, the method would just iterate a list of responses and return 404 otherwise.
A token could be a ctor parameter.
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.
I think it's a bit of an over-engineered solution for the usage that we require, open to discussing it
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.
We're building framework that will be reused in the future. Just the next sprint about bootstrapper will use this again. The problem I see is that the mock is too hard to use and produces code that is too hard to read (constructor inside ternary inside lambda inside constructor).
Please create a follow up PR that will change this.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s), but failed to run 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s), but failed to run 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s), but failed to run 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s), but failed to run 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s), but failed to run 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s), but failed to run 1 pipeline(s). |
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.
I don't know where the regex came from
Tests/SonarScanner.MSBuild.PreProcessor.Test/Infrastructure/HttpMessageHandlerMock.cs
Outdated
Show resolved
Hide resolved
/azp run |
Azure Pipelines successfully started running 1 pipeline(s), but failed to run 1 pipeline(s). |
Quality Gate passedIssues Measures |
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.
LGTM
Reviewer: each commit is replacing moq in a single project