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.

Sign up for GitHub

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

Jump to bottom

Add logging when environment variable parsing fails #1251

Merged
merged 10 commits into from
Jun 15, 2022
Merged

Add logging when environment variable parsing fails #1251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5f08c0f
Pass the logger param
andrei-epure-sonarsource Jun 8, 2022
60e948a
Propagate logger interface changes to TFS code
andrei-epure-sonarsource Jun 8, 2022
0f8848b
Improve error reporting
andrei-epure-sonarsource Jun 8, 2022
cd6ead4
Add IT
andrei-epure-sonarsource Jun 8, 2022
edd6865
treat 2 code smells
andrei-epure-sonarsource Jun 9, 2022
a5f5eb7
Add TestInitialize
andrei-epure-sonarsource Jun 9, 2022
e9733cc
Improve readability
andrei-epure-sonarsource Jun 9, 2022
3eedf6d
Only log warning
andrei-epure-sonarsource Jun 13, 2022
9e61c6c
Revert "Only log warning"
andrei-epure-sonarsource Jun 14, 2022
643e370
Revert "treat 2 code smells"
andrei-epure-sonarsource Jun 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 26 additions & 10 deletions Tests/SonarScanner.MSBuild.Common.Test/AnalysisConfig/AnalysisConfigExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,22 @@ public void GetConfigValue_WhenSettingIdIsWhitespace_ThrowsArgumentNullException
public void GetAnalysisSettings_WhenConfigIsNull_ThrowsArgumentNullException()
{
csaba-sagi-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
// Arrange
Action action = () => ConfigSettingsExtensions.GetAnalysisSettings(null, false);
Action action = () => ConfigSettingsExtensions.GetAnalysisSettings(null, false, new TestLogger());

// Act & Assert
action.Should().ThrowExactly<ArgumentNullException>().And.ParamName.Should().Be("config");
}

[TestMethod]
public void GetAnalysisSettings_WhenLoggerIsNull_ThrowsArgumentNullException()
{
// Arrange
Action action = () => ConfigSettingsExtensions.GetAnalysisSettings(new AnalysisConfig(), false, null);

// Act & Assert
action.Should().ThrowExactly<ArgumentNullException>().And.ParamName.Should().Be("logger");
}

[TestMethod]
public void SetSettingsFilePath_WhenConfigIsNull_ThrowsArgumentNullException()
{
Expand Down Expand Up @@ -409,12 +419,16 @@ public void ConfigExt_FindServerVersion_ValidVersion_ReturnsVersion()
public void ConfigExt_GetSettingOrDefault_InvalidArgs_Throw()
{
// 1. Null config
Action action = () => ConfigSettingsExtensions.GetSettingOrDefault(null, "anySetting", true, "defaultValue");
Action action = () => ConfigSettingsExtensions.GetSettingOrDefault(null, "anySetting", true, "defaultValue", new TestLogger());
action.Should().ThrowExactly<ArgumentNullException>().And.ParamName.Should().Be("config");

// 2. Null setting name
action = () => ConfigSettingsExtensions.GetSettingOrDefault(new AnalysisConfig(), null, true, "defaultValue");
action = () => ConfigSettingsExtensions.GetSettingOrDefault(new AnalysisConfig(), null, true, "defaultValue", new TestLogger());
action.Should().ThrowExactly<ArgumentNullException>().And.ParamName.Should().Be("settingName");

// 3. Null logger
action = () => ConfigSettingsExtensions.GetSettingOrDefault(new AnalysisConfig(), "foo", true, "defaultValue", null);
action.Should().ThrowExactly<ArgumentNullException>().And.ParamName.Should().Be("logger");
}

[TestMethod]
Expand All @@ -427,21 +441,22 @@ public void ConfigExt_GetSettingOrDefault_NoSetting_DefaultIsReturned()
new Property{ Id = "foo", Value = "value" }
}
};
var testLogger = new TestLogger();

// 1. Non-null default
var result = ConfigSettingsExtensions.GetSettingOrDefault(config, "id1", true, "default1");
var result = ConfigSettingsExtensions.GetSettingOrDefault(config, "id1", true, "default1", testLogger);
result.Should().Be("default1");

// 2. Null default is ok
result = ConfigSettingsExtensions.GetSettingOrDefault(config, "id1", true, null);
result = ConfigSettingsExtensions.GetSettingOrDefault(config, "id1", true, null, testLogger);
result.Should().BeNull();

// 3. Case-sensitive
result = ConfigSettingsExtensions.GetSettingOrDefault(config, "FOO", true, "not found");
result = ConfigSettingsExtensions.GetSettingOrDefault(config, "FOO", true, "not found", testLogger);
result.Should().Be("not found");

// 4. Not including server settings -> missing
result = ConfigSettingsExtensions.GetSettingOrDefault(config, "foo", false, "default1");
result = ConfigSettingsExtensions.GetSettingOrDefault(config, "foo", false, "default1", testLogger);
result.Should().Be("default1");
}

Expand All @@ -459,9 +474,10 @@ public void ConfigExt_GetSettingOrDefault_SettingExists_ValueIsReturned()
new Property { Id = "id1", Value = "local value" }
}
};
var testLogger = new TestLogger();

// 1. Local value should take precedence
var result = ConfigSettingsExtensions.GetSettingOrDefault(config, "id1", true, "local value");
var result = ConfigSettingsExtensions.GetSettingOrDefault(config, "id1", true, "local value", testLogger);
result.Should().Be("local value");
}

Expand All @@ -470,15 +486,15 @@ public void ConfigExt_GetSettingOrDefault_SettingExists_ValueIsReturned()
private static IAnalysisPropertyProvider GetAnalysisSettingsIsolatedFromEnvironment(AnalysisConfig config, bool includeServerSettings)
{
IAnalysisPropertyProvider provider = null;

var testLogger = new TestLogger();
// Make sure the test isn't affected by the hosting environment
// The SonarCloud VSTS extension sets additional properties in an environment variable that
// would affect the test
using (var scope = new EnvironmentVariableScope())
{
scope.SetVariable(EnvScannerPropertiesProvider.ENV_VAR_KEY, null);

provider = ConfigSettingsExtensions.GetAnalysisSettings(config, includeServerSettings);
provider = ConfigSettingsExtensions.GetAnalysisSettings(config, includeServerSettings, testLogger);
}

return provider;
Expand Down
13 changes: 12 additions & 1 deletion Tests/SonarScanner.MSBuild.Common.Test/EnvScannerPropertiesProviderTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using System;
using System.Linq;
using FluentAssertions;
using Microsoft.VisualStudio.TestTools.UnitTesting;
Expand All @@ -37,6 +38,16 @@ public void ParseValidJson()
provider.GetAllProperties().First().Value.Should().Be("http://myhost");
}

[TestMethod]
public void TryCreateProvider_WithNullLogger_Throws()
{
// Arrange
Action action = () => EnvScannerPropertiesProvider.TryCreateProvider(null, out var provider);

// Act & Assert
action.Should().ThrowExactly<ArgumentNullException>().And.ParamName.Should().Be("logger");
}

[TestMethod]
public void ParseInvalidJson()
{
Expand All @@ -51,7 +62,7 @@ public void ParseInvalidJson()
scope.SetVariable("SONARQUBE_SCANNER_PARAMS", "trash");
var result = EnvScannerPropertiesProvider.TryCreateProvider(logger, out var provider);
result.Should().BeFalse();
logger.AssertErrorLogged("Failed to parse properties from the environment variable 'SONARQUBE_SCANNER_PARAMS'");
logger.AssertErrorLogged("Failed to parse properties from the environment variable 'SONARQUBE_SCANNER_PARAMS' because 'Error parsing boolean value. Path '', line 1, position 2.'.");
}
}

Expand Down
14 changes: 7 additions & 7 deletions Tests/SonarScanner.MSBuild.TFS.Test/BuildVNextCoverageReportProcessorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,13 +143,13 @@ public void ProcessCoverageReports_VsCoverageXmlPathProvided_NotCoverageXmlFileA
testSubject.Initialise(analysisConfig, settings, testDir + "\\sonar-project.properties");

// Act
var result = testSubject.ProcessCoverageReports();
var result = testSubject.ProcessCoverageReports(testLogger);

// Assert
result.Should().BeTrue();
converter.AssertExpectedNumberOfConversions(1);

Assert.AreEqual(analysisConfig.GetSettingOrDefault(SonarProperties.VsCoverageXmlReportsPaths, true, null), coveragePathValue);
Assert.AreEqual(analysisConfig.GetSettingOrDefault(SonarProperties.VsCoverageXmlReportsPaths, true, null, testLogger), coveragePathValue);
}

[TestMethod]
Expand All @@ -175,7 +175,7 @@ public void ProcessCoverageReports_VsTestReportsPathsProvided_ShouldSkipSearchin
testSubject.Initialise(analysisConfig, settings, testDir + "\\sonar-project.properties");

// Act
var result = testSubject.ProcessCoverageReports();
var result = testSubject.ProcessCoverageReports(testLogger);

// Assert
// 1) Property vstestreportPaths provided, we skip the search for trx files.
Expand Down Expand Up @@ -218,7 +218,7 @@ public void ProcessCoverageReports_VsCoverageXmlPathProvided_CoverageXmlFileAlre
try
{
// Act
var result = testSubject.ProcessCoverageReports();
var result = testSubject.ProcessCoverageReports(testLogger);

// Assert
result.Should().BeTrue();
Expand Down Expand Up @@ -268,7 +268,7 @@ public void ProcessCoverageReports_NotVsCoverageXmlPathProvided_CoverageXmlFileA

using (new AssertIgnoreScope())
{
var result = testSubject.ProcessCoverageReports();
var result = testSubject.ProcessCoverageReports(testLogger);

// Assert
result.Should().BeTrue();
Expand Down Expand Up @@ -311,7 +311,7 @@ public void ProcessCoverageReports_NotVsCoverageXmlPathProvided_NotCoverageXmlFi
testSubject.Initialise(analysisConfig, settings, testDir + "\\sonar-project.properties");

// Act
var result = testSubject.ProcessCoverageReports();
var result = testSubject.ProcessCoverageReports(testLogger);

// Assert
result.Should().BeTrue();
Expand Down Expand Up @@ -350,7 +350,7 @@ public void ProcessCoverageReports_NotVsCoverageXmlPathProvided_NotCoverageXmlFi
testSubject.Initialise(analysisConfig, settings, testDir + "\\sonar-project.properties");

// Act
var result = testSubject.ProcessCoverageReports();
var result = testSubject.ProcessCoverageReports(testLogger);

// Assert
result.Should().BeTrue();
Expand Down
8 changes: 4 additions & 4 deletions Tests/SonarScanner.MSBuild.TFS.Test/Classic/TfsLegacyCoverageReportProcessorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ public void ReportProcessor_NoUrlsFound()
// Act
var initResult = processor.Initialise(context, settings, testDir + "\\sonar-project.properties");
initResult.Should().BeTrue("Expecting true: processor should have been initialized successfully");
var result = processor.ProcessCoverageReports();
var result = processor.ProcessCoverageReports(logger);

// Assert
urlProvider.AssertGetUrlsCalled();
Expand Down Expand Up @@ -130,7 +130,7 @@ public void ReportProcessor_MultipleUrlsFound()
// Act
var initResult = processor.Initialise(context, settings, testDir + "\\sonar-project.properties");
initResult.Should().BeTrue("Expecting true: processor should have been initialized successfully");
var result = processor.ProcessCoverageReports();
var result = processor.ProcessCoverageReports(logger);

// Assert
urlProvider.AssertGetUrlsCalled();
Expand Down Expand Up @@ -159,7 +159,7 @@ public void ReportProcessor_SingleUrlFound_NotDownloaded()
// Act
var initResult = processor.Initialise(context, settings, string.Empty);
initResult.Should().BeTrue("Expecting true: processor should have been initialized successfully");
var result = processor.ProcessCoverageReports();
var result = processor.ProcessCoverageReports(logger);

// Assert
urlProvider.AssertGetUrlsCalled();
Expand Down Expand Up @@ -196,7 +196,7 @@ public void ReportProcessor_SingleUrlFound_DownloadedOk()
// Act
var initResult = processor.Initialise(context, settings, testDir + "\\sonar-project.properties");
initResult.Should().BeTrue("Expecting true: processor should have been initialized successfully");
var result = processor.ProcessCoverageReports();
var result = processor.ProcessCoverageReports(logger);

// Assert
urlProvider.AssertGetUrlsCalled();
Expand Down
10 changes: 5 additions & 5 deletions Tests/SonarScanner.MSBuild.TFS.Test/CoverageReportProcessorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public void ProcessCoverageReports_LegacyTeamBuild_SkipCoverageIsFalse_WhenProce
// Set up the factory to return a processor that returns success
var processorMock = new Mock<ICoverageReportProcessor>();
processorMock.Setup(x => x.Initialise(It.IsAny<AnalysisConfig>(), It.IsAny<ITeamBuildSettings>(), It.IsAny<String>())).Returns(true);
processorMock.Setup(x => x.ProcessCoverageReports()).Returns(true);
processorMock.Setup(x => x.ProcessCoverageReports(logger)).Returns(true);
legacyFactoryMock.Setup(x => x.BuildTfsLegacyCoverageReportProcessor()).Returns(processorMock.Object);

using (var scope = new EnvironmentVariableScope())
Expand All @@ -106,13 +106,13 @@ public void ProcessCoverageReports_LegacyTeamBuild_SkipCoverageIsFalse_WhenProce
testSubject.Initialise(analysisConfig, settingsMock, String.Empty);

// Act
var result = testSubject.ProcessCoverageReports();
var result = testSubject.ProcessCoverageReports(logger);

// Assert
result.Should().BeTrue();
legacyFactoryMock.Verify(x => x.BuildTfsLegacyCoverageReportProcessor(), Times.Once);
processorMock.Verify(x => x.Initialise(It.IsAny<AnalysisConfig>(), It.IsAny<ITeamBuildSettings>(), It.IsAny<String>()), Times.Once);
processorMock.Verify(x => x.ProcessCoverageReports(), Times.Once);
processorMock.Verify(x => x.ProcessCoverageReports(logger), Times.Once);
}
}

Expand All @@ -135,7 +135,7 @@ public void ProcessCoverageReports_LegacyTeamBuild_SkipCoverageIsTrue_WhenProces
var result = false;
using (new AssertIgnoreScope())
{
result = testSubject.ProcessCoverageReports();
result = testSubject.ProcessCoverageReports(logger);
}

// Assert
Expand All @@ -159,7 +159,7 @@ public void ProcessCoverageReports_Standalone_WhenProcess_ReturnsTrue()
var result = false;
using (new AssertIgnoreScope())
{
result = testSubject.ProcessCoverageReports();
result = testSubject.ProcessCoverageReports(logger);
}

// Assert
Expand Down
2 changes: 1 addition & 1 deletion Tests/SonarScanner.MSBuild.TFS.Test/Infrastructure/MockCodeCoverageProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public bool Initialise(AnalysisConfig config, ITeamBuildSettings settings, strin
return InitialiseValueToReturn;
}

public bool ProcessCoverageReports()
public bool ProcessCoverageReports(ILogger logger)
{
processCoverageMethodCalled.Should().BeFalse("Expecting ProcessCoverageReports to be called only once");
initalisedCalled.Should().BeTrue("Expecting Initialize to be called first");
Expand Down
2 changes: 1 addition & 1 deletion Tests/SonarScanner.MSBuild.TFS.Test/Infrastructure/MockSummaryReportBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ internal class MockSummaryReportBuilder : ISummaryReportBuilder

#region ISummaryReportBuilder interface

public void GenerateReports(ITeamBuildSettings settings, AnalysisConfig config, bool ranToCompletion, string fullPropertiesFilePath)
public void GenerateReports(ITeamBuildSettings settings, AnalysisConfig config, bool ranToCompletion, string fullPropertiesFilePath, ILogger logger)
{
methodCalled.Should().BeFalse("Generate reports has already been called");

Expand Down
Loading