Skip to content

Commit

Permalink
Add to the retrieve-codeowners tool support for returning owners fo…
Browse files Browse the repository at this point in the history
…r all paths matching given glob path. (#5134)

This PR implements for the `retrieve-codeowners` tool the ability to return not only owners for a single path, but a list of all owners for all paths resolved when matching a glob path given as input.

As such, this PR contributes to:
- #5135

This work is necessary to be able to compare and diff the owners of all files in repository before and after the regex matcher is turned on, per this comment:
- #5088 (review)

In other words, this PR contributes to unblocking to the following PR:
- #5088

And as such, also contributes to:
- #2770

This PR will require some upstream changes, which are captured by:
- #5103

### Additional changes

- Removed logger from `CodeOwnersParser`; replaced with `Console.Out` and `Console.Error`. This addresses the following comment:
    - #5063 (comment)
- Prevented the new regex matcher from matching paths that have `*` in them - such paths are malformed anyway (at least on Windows).
- A lot of assorted changes to surrounding production & test code - please see the file diff for details.

### Implementation notes

This PR leverages [`Microsoft.Extensions.FileSystemGlobbing`](https://www.nuget.org/packages/Microsoft.Extensions.FileSystemGlobbing).
Doc: https://learn.microsoft.com/en-us/dotnet/core/extensions/file-globbing
  • Loading branch information
Konrad Jamrozik authored Jan 14, 2023
1 parent b4ae814 commit 4cf0beb
Show file tree
Hide file tree
Showing 22 changed files with 490 additions and 92 deletions.
2 changes: 1 addition & 1 deletion eng/common/scripts/get-codeowners.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ function TestGetCodeOwner([string]$targetDirectory, [string]$codeOwnerFileLocati
}

if($Test) {
$testFile = (Resolve-Path $PSScriptRoot/../../../tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CODEOWNERS)
$testFile = (Resolve-Path $PSScriptRoot/../../../tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/simple_path_CODEOWNERS)
TestGetCodeOwner -targetDirectory "sdk" -codeOwnerFileLocation $testFile -includeNonUserAliases $true -expectReturn @("person1", "person2")
TestGetCodeOwner -targetDirectory "sdk/noPath" -codeOwnerFileLocation $testFile -includeNonUserAliases $true -expectReturn @("person1", "person2")
TestGetCodeOwner -targetDirectory "/sdk/azconfig" -codeOwnerFileLocation $testFile -includeNonUserAliases $true -expectReturn @("person3", "person4")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ public class CodeownersFileTests
new( "/a]b" , "a]b" , true , false ),
new( "/a?b" , "a?b" , true , false ),
new( "/a?b" , "axb" , false , false ),
new( "/a" , "*" , false , false ),
new( "/*" , "*" , true , false ),
new( "/*" , "a" , false , true ),
new( "/*" , "a/" , false , false ),
new( "/*" , "a/b" , false , false ),
Expand Down Expand Up @@ -152,20 +154,20 @@ public void TestGetMatchingCodeownersEntry(TestCase testCase)
List<CodeownersEntry>? codeownersEntries =
CodeownersFile.GetCodeownersEntries(testCase.CodeownersPath + "@owner");

VerifyGetMatchingCodeownersEntry(testCase, codeownersEntries, useNewImpl: false, testCase.ExpectedLegacyMatch);
VerifyGetMatchingCodeownersEntry(testCase, codeownersEntries, useNewImpl: true, testCase.ExpectedNewMatch);
VerifyGetMatchingCodeownersEntry(testCase, codeownersEntries, useRegexMatcher: false, testCase.ExpectedLegacyMatch);
VerifyGetMatchingCodeownersEntry(testCase, codeownersEntries, useRegexMatcher: true, testCase.ExpectedNewMatch);
}

private static void VerifyGetMatchingCodeownersEntry(
TestCase testCase,
List<CodeownersEntry> codeownersEntries,
bool useNewImpl,
bool useRegexMatcher,
bool expectedMatch)
{
CodeownersEntry? entryLegacy =
// Act
CodeownersFile.GetMatchingCodeownersEntry(testCase.TargetPath,
codeownersEntries, useNewImpl: useNewImpl);
codeownersEntries, useRegexMatcher);

Assert.That(entryLegacy.Owners, Has.Count.EqualTo(expectedMatch ? 1 : 0));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

<PropertyGroup>
<TargetFramework>net6.0</TargetFramework>
<LangVersion>10.0</LangVersion>
<Nullable>enable</Nullable>
<WarningsAsErrors>Nullable</WarningsAsErrors>
<IsPackable>false</IsPackable>
Expand All @@ -20,8 +21,15 @@
</ItemGroup>

<ItemGroup>
<None Update="CODEOWNERS">
<None Update="TestData\simple_path_CODEOWNERS">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</None>
<None Update="TestData/**">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</None>
</ItemGroup>

<ItemGroup>
<Folder Include="TestData\InputDir\foo\bar\" />
</ItemGroup>
</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -4,40 +4,53 @@
namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests
{
/// <summary>
/// The class is to redirect console output to string writer.
/// The class is to redirect console STDOUT and STDERR to string writer.
/// </summary>
public class ConsoleOutput : IDisposable
{
private readonly StringWriter stringWriter;
private readonly TextWriter originalOutput;
private readonly StringWriter stdoutWriter, stderrWriter;
private readonly TextWriter originalStdout, originalStderr;

/// <summary>
/// The constructor is where we take in the console output and output to string writer.
/// </summary>
public ConsoleOutput()
{
this.stringWriter = new StringWriter();
this.originalOutput = Console.Out;
Console.SetOut(this.stringWriter);
this.stdoutWriter = new StringWriter();
this.stderrWriter = new StringWriter();
this.originalStdout = Console.Out;
this.originalStderr = Console.Error;
Console.SetOut(this.stdoutWriter);
Console.SetError(this.stderrWriter);
}

/// <summary>
/// Writes the text representation of a string builder to the string.
/// </summary>
/// <returns>The string from console output.</returns>
public string GetOutput()
{
return this.stringWriter.ToString();
}
public string GetStdout()
=> this.stdoutWriter.ToString();

public string[] GetStdoutLines()
=> this.stdoutWriter.ToString().Split(Environment.NewLine);

public string GetStderr()
=> this.stderrWriter.ToString();

public string[] GetStderrLines()
=> this.stderrWriter.ToString().Split(Environment.NewLine);

/// <summary>
/// Releases all resources used by the originalOutput and stringWriter object.
/// </summary>
public void Dispose()
{
Console.SetOut(this.originalOutput);
this.stringWriter.Dispose();
this.originalOutput.Dispose();
Console.SetOut(this.originalStdout);
Console.SetError(this.originalStderr);
this.stdoutWriter.Dispose();
this.stderrWriter.Dispose();
this.originalStdout.Dispose();
this.originalStderr.Dispose();
// https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1816
GC.SuppressFinalize(this);
}
Expand All @@ -47,8 +60,10 @@ public void Dispose()
/// </summary>
public void Close()
{
this.stringWriter.Close();
this.originalOutput.Close();
this.stdoutWriter.Close();
this.stderrWriter.Close();
this.originalStderr.Close();
this.originalStdout.Close();
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.Json;
using Azure.Sdk.Tools.CodeOwnersParser;
using NUnit.Framework;

namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests;

/// <summary>
/// Test class for Azure.Sdk.Tools.RetrieveCodeOwners.Program.Main(),
/// for scenario in which targetPath is a glob path, i.e.
/// targetPath.IsGlobPath() returns true.
/// </summary>
[TestFixture]
public class ProgramGlobPathTests
{
/// <summary>
/// Given:
/// <br/><br/>
/// codeownersFilePathOrUrl contents of:
/// <code>
///
/// /** @2star
/// /* @star
/// /foo/**/a.txt @foo_2star_a
/// /foo/*/a.txt @foo_star_a_1 @foo_star_a_2
///
/// </code>
///
/// targetDir contents of:
///
/// <code>
///
/// /a.txt
/// /b.txt
/// /foo/a.txt
/// /foo/b.txt
/// /foo/bar/a.txt
/// /foo/bar/b.txt
///
/// </code>
///
/// targetPath of:
///
/// <code>
///
/// /foo/**
///
/// </code>
///
/// excludeNonUserAliases set to false and useRegexMatcher set to true.
/// <br/><br/>
/// When:
/// The retrieve-codeowners tool is executed on these inputs.
/// <br/><br/>
/// Then:
/// The tool should return on STDOUT owners matched in following way:
///
/// <code>
/// /a.txt @star
/// /b.txt @star
/// /foo/a.txt @foo_2star_a
/// /foo/b.txt @2star
/// /foo/bar/a.txt @foo_star_a_1 @foo_star_a_2
/// /foo/bar/b.txt @2star
/// </code>
/// </summary>
[Test]
public void OutputsCodeownersForGlobPath()
{
const string targetDir = "./TestData/InputDir";
const string targetPath = "/**";
const string codeownersFilePathOrUrl = "./TestData/glob_path_CODEOWNERS";
const bool excludeNonUserAliases = false;
const bool useRegexMatcher = true;

var expectedEntries = new Dictionary<string, CodeownersEntry>
{
// @formatter:off
["a.txt"] = new CodeownersEntry("/*", new List<string> { "star" }),
["b.txt"] = new CodeownersEntry("/*", new List<string> { "star" }),
["foo/a.txt"] = new CodeownersEntry("/foo/**/a.txt", new List<string> { "foo_2star_a" }),
["foo/b.txt"] = new CodeownersEntry("/**", new List<string> { "2star" }),
["foo/bar/a.txt"] = new CodeownersEntry("/foo/*/a.txt", new List<string> { "foo_star_a_1", "foo_star_a_2" }),
["foo/bar/b.txt"] = new CodeownersEntry("/**", new List<string> { "2star" }),
// @formatter:on
};

string actualOutput, actualErr;
int returnCode;
using (var consoleOutput = new ConsoleOutput())
{
// Act
returnCode = Program.Main(
targetPath,
codeownersFilePathOrUrl,
excludeNonUserAliases,
targetDir,
useRegexMatcher);

actualOutput = consoleOutput.GetStdout();
actualErr = consoleOutput.GetStderr();
}

var actualEntries = TryDeserializeActualEntries(actualOutput, actualErr);

Assert.Multiple(() =>
{
AssertEntries(actualEntries, expectedEntries);
Assert.That(returnCode, Is.EqualTo(0));
Assert.That(actualErr, Is.EqualTo(string.Empty));
});
}

private static Dictionary<string, CodeownersEntry> TryDeserializeActualEntries(
string actualOutput,
string actualErr)
{
Dictionary<string, CodeownersEntry> actualEntries;
try
{
actualEntries =
JsonSerializer.Deserialize<Dictionary<string, CodeownersEntry>>(actualOutput)!;
}
catch (Exception e)
{
Console.WriteLine(e);
Console.WriteLine("actualOutput: " + actualOutput);
Console.WriteLine("actualErr: " + actualErr);
throw;
}

return actualEntries;
}

private static void AssertEntries(
Dictionary<string, CodeownersEntry> actualEntries,
Dictionary<string, CodeownersEntry> expectedEntries)
{
foreach (KeyValuePair<string, CodeownersEntry> kvp in actualEntries)
{
string path = kvp.Key;
CodeownersEntry actualEntry = kvp.Value;
Assert.That(expectedEntries[path], Is.EqualTo(actualEntry), $"path: {path}");
}

Assert.That(actualEntries.Count, Is.EqualTo(expectedEntries.Count));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,14 @@
namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests
{
/// <summary>
/// Test class for Azure.Sdk.Tools.RetrieveCodeOwners.Program.
/// Test class for Azure.Sdk.Tools.RetrieveCodeOwners.Program.Main(),
/// for scenario in which targetPath is a simple path, i.e.
/// targetPath.IsGlobPath() returns false.
/// </summary>
[TestFixture]
public class ProgramTests
public class ProgramSimplePathTests
{
private const string CodeownersFilePath = "CODEOWNERS";
private const string CodeownersFilePath = "./TestData/simple_path_CODEOWNERS";

private static readonly object[] sourceLists =
{
Expand All @@ -25,21 +27,24 @@ public class ProgramTests
};

[TestCaseSource(nameof(sourceLists))]
public void TestOnNormalOutput(string targetPath, bool excludeNonUserAliases, List<string> expectedOwners)
public void OutputsCodeownersForSimplePath(
string targetPath,
bool excludeNonUserAliases,
List<string> expectedOwners)
{
using var consoleOutput = new ConsoleOutput();

// Act
Program.Main(targetPath, CodeownersFilePath, excludeNonUserAliases);

string actualOutput = consoleOutput.GetOutput();
string actualOutput = consoleOutput.GetStdout();
AssertOwners(actualOutput, expectedOwners);
}

[TestCase("PathNotExist")]
[TestCase("http://testLink")]
[TestCase("https://testLink")]
public void TestOnError(string codeownersPath)
public void ErrorsOutOnInvalidInputs(string codeownersPath)
{
Assert.That(Program.Main("sdk", codeownersPath), Is.EqualTo(1));
}
Expand Down
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests.TestData
{
class b
{
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests.TestData.foo
{
class a
{
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests.TestData.foo
{
class b
{
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# This file is a test resoure for the test:
#
# Azure.Sdk.Tools.RetrieveCodeOwners.Tests.WildcardTests.TestWildcard
#

/** @2star
/* @star
/foo/**/a.txt @foo_2star_a
/foo/*/a.txt @foo_star_a_1 @foo_star_a_2
Loading

0 comments on commit 4cf0beb

Please sign in to comment.