Add sourcelink tests #17309

NikolaMilosavljevic · 2023-09-05T21:03:13Z

For now, we process just the runtime symbols as they are available in source-build artifacts. After PDB work is done, we'll modify the code in GetAllSymbolsPackages() to discover all symbols archives. That work is tracked with: dotnet/source-build#3612

PDB work has the following related issues: dotnet/source-build#3547, dotnet/source-build#3609, dotnet/source-build#3610

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/Utilities.cs

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/SourcelinkTests.cs

mthalman · 2023-09-05T21:19:39Z

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/SourcelinkTests.cs

+        string[] files = Directory.GetFiles(Path.GetDirectoryName(Config.SourceBuiltArtifactsPath), runtimeSymbolsPackageNamePattern, SearchOption.AllDirectories);
+        Assert.True(files.Length > 0, "Did not find runtime symbols archive");
+
+        yield return files[0];


Why is the return type IEnumerable<string> but only ever returning one item?

We'll be updating this code soon, to enumerate all symbols archives.

Is this work still pending?

It is coming up very soon. PR for creating symbols archives will be out today or tomorrow. Update to sourcelink validation to consume those archives will follow few days later, barring any unforeseen issues.

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/SourcelinkTests.cs

…eTests/Utilities.cs Co-authored-by: Matt Thalman <mthalman@microsoft.com>

MichaelSimons · 2023-09-06T13:26:08Z

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/ExecuteHelper.cs

@@ -17,10 +17,14 @@ internal static class ExecuteHelper
        string args,
        ITestOutputHelper outputHelper,
        bool logOutput = false,
+        bool logInfo = true,


Default parameters should be named in such a way that the default value is the type's default value. Therefor this parameter's default value should be false and therefore name should be inverted.

Suggested change

bool logInfo = true,

bool excludeInfo = false,

Thanks - will fix.

Fixed with 72aacfc

MichaelSimons · 2023-09-06T13:26:54Z

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/ExecuteHelper.cs

        Action<Process>? configureCallback = null,
        int millisecondTimeout = -1)
    {
-        outputHelper.WriteLine($"Executing: {fileName} {args}");
+        if (logInfo)


Later on there is some more info logged. Should it be conditioned on this parameter as well?

outputHelper.WriteLine($"Process did not exit. Killing {fileName} {args} after waiting {millisecondTimeout} milliseconds.");

I consider that one an error message, and only wanted to provide an option to skip logging info/status messages.

MichaelSimons · 2023-09-06T13:32:16Z

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/SourcelinkTests.cs

+    /// <summary>
+    /// Verifies that all symbols have valid sourcelinks.
+    /// </summary>
+    [SkippableFact(Config.ExcludeSourcelinkEnv, skipOnTrueEnv: true)]


What is the scenario for excluding these tests? I am trying to understand the need for the environment variable to exclude them.

Time savings, mostly. It shouldn't be an issue today and probably unnecessary?

xunit/dotnet already supports a mechanism to run a subset of tests via the --filter option. That is what I utilize in my dev scenarios. We use ENVs only for CI configuration purposes. I think this should be removed as we intend to run this all of the time.

Fixed with 72aacfc

MichaelSimons · 2023-09-06T14:43:22Z

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/SourcelinkTests.cs

+    /// Extracts the dotnet-sourcelink tool package from PSB arhive.
+    /// </summary>
+    /// <returns>Path to sourcelink tool binary.</returns>
+    private string GetSourcelinkToolPath()


This does a lot more than retrieve the sourcelink tool path. Naming this to something like InitializeSourceLinkTool feels more appropriate.

Yeah, it does a lot more, today. Sourcelink tool might become part of SDK in the future and the method name would still be the same if we modify its contents.

However, I agree that your proposed name is more generic. Will fix.

Fixed with 72aacfc

MichaelSimons · 2023-09-06T14:46:27Z

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/SourcelinkTests.cs

+
+public class SourcelinkTests : SmokeTests
+{
+    private static string SourcelinkRoot { get; } = Path.Combine(Directory.GetCurrentDirectory(), "sourcelink");


Consider using a name that associates the directory directly with the tests that create them.

Suggested change

private static string SourcelinkRoot { get; } = Path.Combine(Directory.GetCurrentDirectory(), "sourcelink");

private static string SourcelinkRoot { get; } = Path.Combine(Directory.GetCurrentDirectory(), nameof(SourcelinkTests));

I've followed the existing patterns in this codebase, i.e.:

installer/src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/OmniSharpTests.cs

Line 24 in 8bdf61a

private string OmniSharpDirectory { get; } = Path.Combine(Directory.GetCurrentDirectory(), "omnisharp");

However, this is an easy fix and will update it.

Ah - yes, I think both instances should be updated - your call.

Fixed with 72aacfc

MichaelSimons · 2023-09-06T14:50:03Z

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/SourcelinkTests.cs

+
+        foreach (string file in failedFiles)
+        {
+            OutputHelper.WriteLine($"Failed sourcelink verification: {file}");


Just a general suggestion that can I find helpful is to emit a header and put the files alone on separate lines. This is helpful for copy/paste specifically.

Sourcelink Verification Failures:
FilePath1
FilePath2
...

Yes - I did think of this option, but never got to implementing it. Will do.

Fixed with 72aacfc

MichaelSimons · 2023-09-06T15:02:24Z

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/SourcelinkTests.cs

+    private string GetSourcelinkToolPath()
+    {
+        const string SourcelinkToolPackageNamePattern = "dotnet-sourcelink*nupkg";
+        const string SourcelinkToolBinaryFilenamePattern = "dotnet-sourcelink.dll";


What makes this a pattern? It appears to be the actual filename.

Yeah, true - copy/paste issue. Will update - thanks!

Fixed with 72aacfc

MichaelSimons · 2023-09-06T15:08:34Z

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/SourcelinkTests.cs

+
+        string toolPackageDir = Directory.CreateDirectory(Path.Combine(SourcelinkRoot, "sourcelink-tool")).FullName;
+        Utilities.ExtractTarball(Config.SourceBuiltArtifactsPath, toolPackageDir, SourcelinkToolPackageNamePattern);
+        string[] files = Directory.GetFiles(toolPackageDir, SourcelinkToolPackageNamePattern, SearchOption.AllDirectories);


This pattern appears to be used three times. Consider making a shared Utility method for it.

Quoted code shows tarball extraction which appears twice. Are you suggesting a Utility method for the pattern that follows the extraction - getting a single file and failing if there are 0 or more than 1?

Sorry my comment was not very clear. I am referring to this pattern:

files = Directory.GetFiles(extractedToolPath, SourcelinkToolBinaryFilenamePattern, SearchOption.AllDirectories); Assert.False(files.Length > 1, $"Unexpected - found more than one sourcelink tool binary with filename pattern: {SourcelinkToolBinaryFilenamePattern}"); Assert.False(files.Length == 0, $"Did not find sourcelink tool binary with expected filename pattern: {SourcelinkToolBinaryFilenamePattern}");

I'm not sure about this. One of the instances is going away in the near future, when we start using Symbols archives. The code gets harder to read, line lengths balloon, or line breaks get introduced that just invite bugs.

For clarity this is what I mean. Usage Utility.GetFile(path, pattern)

public static string GetFile(string path, string pattern)
{
files = Directory.GetFiles(path, pattern, SearchOption.AllDirectories);
Assert.False(files.Length > 1, $"Found multiple files matching the pattern {pattern}: {Enivronment.NewLine}{string.Join(files, Enivronment.NewLine)}");
Assert.False(files.Length == 0, $"Did not find any files matching the pattern {pattern}");

return file[0];

}

To me it actually helps the readability of the code because it simplifies this method.

Yeah, that would be a generic simplification,. I wanted to provide a way to supply, and use, custom messages, which bloated the code in the caller method, and made it almost unreadable.

I'll do the suggested solution.

Fixed with c499278

MichaelSimons · 2023-09-06T15:17:26Z

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/SourcelinkTests.cs

+        return symbolsRoot;
+    }
+
+    private IList<string> ValidateAllFiles(string path, string sourcelinkToolPath)


Renaming this in terms of symbols would help the readability IMO.

Suggested change

private IList<string> ValidateAllFiles(string path, string sourcelinkToolPath)

private IList<string> ValidateSymbols(string path, string sourcelinkToolPath)

Thanks - will update this.

Fixed with 72aacfc

MichaelSimons · 2023-09-06T15:17:50Z

src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.SmokeTests/SourcelinkTests.cs

+    /// Extracts packages to subdirectories of the common symbols root directory.
+    /// </summary>
+    /// <returns>Path to common symbols root directory.</returns>
+    private string ExtractPackages(IEnumerable<string> packages)


Renaming this in terms of symbols would help the readability and be more consistent with GetAllSymbolsPackages IMO.

Suggested change

private string ExtractPackages(IEnumerable<string> packages)

private string ExtractSymbolPackages(IEnumerable<string> packages)

Thanks - will update this.

Fixed with 72aacfc

Add sourcelink tests

9cfe4b8

NikolaMilosavljevic requested a review from a team as a code owner September 5, 2023 21:03

mthalman reviewed Sep 5, 2023

View reviewed changes

NikolaMilosavljevic and others added 3 commits September 5, 2023 15:22

Update src/SourceBuild/content/test/Microsoft.DotNet.SourceBuild.Smok…

90a42ed

…eTests/Utilities.cs Co-authored-by: Matt Thalman <mthalman@microsoft.com>

Addressing review comments

9ff1e02

Merge branch 'release/8.0.1xx' into sourcelink.tests

b64d812

MichaelSimons reviewed Sep 6, 2023

View reviewed changes

NikolaMilosavljevic added 3 commits September 6, 2023 18:22

Addressed more review comments

72aacfc

Merge branch 'release/8.0.1xx' into sourcelink.tests

f1c557e

Added utility method for finding files

c499278

MichaelSimons approved these changes Sep 6, 2023

View reviewed changes

NikolaMilosavljevic enabled auto-merge (squash) September 6, 2023 21:41

NikolaMilosavljevic merged commit 9a21558 into dotnet:release/8.0.1xx Sep 6, 2023

	private static string SourcelinkRoot { get; } = Path.Combine(Directory.GetCurrentDirectory(), "sourcelink");
	private static string SourcelinkRoot { get; } = Path.Combine(Directory.GetCurrentDirectory(), nameof(SourcelinkTests));

	private IList<string> ValidateAllFiles(string path, string sourcelinkToolPath)
	private IList<string> ValidateSymbols(string path, string sourcelinkToolPath)

	private string ExtractPackages(IEnumerable<string> packages)
	private string ExtractSymbolPackages(IEnumerable<string> packages)

Add sourcelink tests #17309

Add sourcelink tests #17309

Conversation

NikolaMilosavljevic commented Sep 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikolaMilosavljevic Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikolaMilosavljevic Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikolaMilosavljevic Sep 6, 2023 •

edited

Loading

NikolaMilosavljevic Sep 6, 2023 •

edited

Loading