From f88cf0091b9d6281f6ac5a686eaf74c532c3739a Mon Sep 17 00:00:00 2001 From: Charlie Poole Date: Tue, 10 Dec 2024 21:12:12 -0800 Subject: [PATCH] Refactoring and new tests --- .../Extensibility/ExtensionAssemblyTests.cs | 13 -- .../Internal/ExtensionAssemblyTrackerTests.cs | 79 ++++++++++ .../Extensibility/ExtensionAssembly.cs | 27 ++-- .../Internal/ExtensionAssemblyTracker.cs | 25 +++ .../Services/ExtensionManager.cs | 146 ++++++++---------- 5 files changed, 181 insertions(+), 109 deletions(-) create mode 100644 src/NUnitEngine/nunit.engine.core.tests/Internal/ExtensionAssemblyTrackerTests.cs create mode 100644 src/NUnitEngine/nunit.engine.core/Internal/ExtensionAssemblyTracker.cs diff --git a/src/NUnitEngine/nunit.engine.core.tests/Extensibility/ExtensionAssemblyTests.cs b/src/NUnitEngine/nunit.engine.core.tests/Extensibility/ExtensionAssemblyTests.cs index 87a16bf9f..a90c47ae0 100644 --- a/src/NUnitEngine/nunit.engine.core.tests/Extensibility/ExtensionAssemblyTests.cs +++ b/src/NUnitEngine/nunit.engine.core.tests/Extensibility/ExtensionAssemblyTests.cs @@ -11,7 +11,6 @@ public class ExtensionAssemblyTests { private static readonly Assembly THIS_ASSEMBLY = Assembly.GetExecutingAssembly(); private static readonly string THIS_ASSEMBLY_PATH = THIS_ASSEMBLY.Location; - private static readonly string THIS_ASSEMBLY_FULL_NAME = THIS_ASSEMBLY.GetName().FullName; private static readonly string THIS_ASSEMBLY_NAME = THIS_ASSEMBLY.GetName().Name; private static readonly Version THIS_ASSEMBLY_VERSION = THIS_ASSEMBLY.GetName().Version; @@ -23,18 +22,6 @@ public void CreateExtensionAssemblies() _ea = new ExtensionAssembly(THIS_ASSEMBLY_PATH, false); } - [Test] - public void AssemblyDefinition() - { - Assert.That(_ea.Assembly.FullName, Is.EqualTo(THIS_ASSEMBLY_FULL_NAME)); - } - - [Test] - public void MainModule() - { - Assert.That(_ea.MainModule.Assembly.FullName, Is.EqualTo(THIS_ASSEMBLY_FULL_NAME)); - } - [Test] public void AssemblyName() { diff --git a/src/NUnitEngine/nunit.engine.core.tests/Internal/ExtensionAssemblyTrackerTests.cs b/src/NUnitEngine/nunit.engine.core.tests/Internal/ExtensionAssemblyTrackerTests.cs new file mode 100644 index 000000000..dc1e6bd18 --- /dev/null +++ b/src/NUnitEngine/nunit.engine.core.tests/Internal/ExtensionAssemblyTrackerTests.cs @@ -0,0 +1,79 @@ +// Copyright (c) Charlie Poole, Rob Prouse and Contributors. MIT License - see LICENSE.txt + +using System; +using System.ComponentModel; +using System.Reflection; +using NUnit.Engine.Extensibility; +using NUnit.Framework; + +namespace NUnit.Engine.Internal.Tests +{ + public class ExtensionAssemblyTrackerTests + { + private static readonly Assembly THIS_ASSEMBLY = typeof(ExtensionAssemblyTrackerTests).Assembly; + private static readonly string THIS_ASSEMBLY_PATH = THIS_ASSEMBLY.Location; + private static readonly string THIS_ASSEMBLY_NAME = THIS_ASSEMBLY.GetName().Name; + private static readonly Version THIS_ASSEMBLY_VERSION = THIS_ASSEMBLY.GetName().Version; + private static readonly ExtensionAssembly TEST_EXTENSION_ASSEMBLY = + new ExtensionAssembly(THIS_ASSEMBLY_PATH, false, THIS_ASSEMBLY_NAME, THIS_ASSEMBLY_VERSION); + + private ExtensionAssemblyTracker _tracker; + + [SetUp] + public void CreateTracker() + { + _tracker = new ExtensionAssemblyTracker(); + } + + [Test] + public void AddToList() + { + _tracker.Add(TEST_EXTENSION_ASSEMBLY); + + Assert.That(_tracker.Count, Is.EqualTo(1)); + Assert.That(_tracker[0].FilePath, Is.EqualTo(THIS_ASSEMBLY_PATH)); + Assert.That(_tracker[0].AssemblyName, Is.EqualTo(THIS_ASSEMBLY_NAME)); + Assert.That(_tracker[0].AssemblyVersion, Is.EqualTo(THIS_ASSEMBLY_VERSION)); + } + + [Test] + public void AddUpdatesNameIndex() + { + _tracker.Add(TEST_EXTENSION_ASSEMBLY); + + Assert.That(_tracker.ByName.ContainsKey(THIS_ASSEMBLY_NAME)); + Assert.That(_tracker.ByName[THIS_ASSEMBLY_NAME].AssemblyName, Is.EqualTo(THIS_ASSEMBLY_NAME)); + Assert.That(_tracker.ByName[THIS_ASSEMBLY_NAME].FilePath, Is.EqualTo(THIS_ASSEMBLY_PATH)); + Assert.That(_tracker.ByName[THIS_ASSEMBLY_NAME].AssemblyVersion, Is.EqualTo(THIS_ASSEMBLY_VERSION)); + } + [Test] + public void AddUpdatesPathIndex() + { + _tracker.Add(TEST_EXTENSION_ASSEMBLY); + + Assert.That(_tracker.ByPath.ContainsKey(THIS_ASSEMBLY_PATH)); + Assert.That(_tracker.ByPath[THIS_ASSEMBLY_PATH].AssemblyName, Is.EqualTo(THIS_ASSEMBLY_NAME)); + Assert.That(_tracker.ByPath[THIS_ASSEMBLY_PATH].FilePath, Is.EqualTo(THIS_ASSEMBLY_PATH)); + Assert.That(_tracker.ByPath[THIS_ASSEMBLY_PATH].AssemblyVersion, Is.EqualTo(THIS_ASSEMBLY_VERSION)); + } + + [Test] + public void AddDuplicatePathThrowsArgumentException() + { + _tracker.Add(TEST_EXTENSION_ASSEMBLY); + + Assert.That(() => + _tracker.Add(TEST_EXTENSION_ASSEMBLY), + Throws.TypeOf()); + } + + [Test] + public void AddDuplicateAssemblyNameThrowsArgumentException() + { + _tracker.Add(TEST_EXTENSION_ASSEMBLY); + + Assert.That(() => _tracker.Add(new ExtensionAssembly("Some/Other/Path", false, THIS_ASSEMBLY_NAME, THIS_ASSEMBLY_VERSION)), + Throws.TypeOf()); + } + } +} diff --git a/src/NUnitEngine/nunit.engine.core/Extensibility/ExtensionAssembly.cs b/src/NUnitEngine/nunit.engine.core/Extensibility/ExtensionAssembly.cs index 0cf2e3747..774f10da5 100644 --- a/src/NUnitEngine/nunit.engine.core/Extensibility/ExtensionAssembly.cs +++ b/src/NUnitEngine/nunit.engine.core/Extensibility/ExtensionAssembly.cs @@ -1,6 +1,7 @@ // Copyright (c) Charlie Poole, Rob Prouse and Contributors. MIT License - see LICENSE.txt using System; +using System.Collections.Generic; using System.IO; using TestCentric.Metadata; using NUnit.Engine.Internal; @@ -14,26 +15,26 @@ public ExtensionAssembly(string filePath, bool fromWildCard) FilePath = filePath; FromWildCard = fromWildCard; Assembly = GetAssemblyDefinition(); + AssemblyName = Assembly.Name.Name; + AssemblyVersion = Assembly.Name.Version; + } + + // Internal constructor used for certain tests. AssemblyDefinition is not initialized. + internal ExtensionAssembly(string filePath, bool fromWildCard, string assemblyName, Version version) + { + FilePath = filePath; + FromWildCard = fromWildCard; + AssemblyName = assemblyName; + AssemblyVersion = version; } public string FilePath { get; } public bool FromWildCard { get; } public AssemblyDefinition Assembly { get; } - public string AssemblyName - { - get { return Assembly.Name.Name; } - } - - public Version AssemblyVersion - { - get { return Assembly.Name.Version; } - } + public string AssemblyName { get; } - public ModuleDefinition MainModule - { - get { return Assembly.MainModule; } - } + public Version AssemblyVersion { get; } #if NETFRAMEWORK public RuntimeFramework TargetFramework diff --git a/src/NUnitEngine/nunit.engine.core/Internal/ExtensionAssemblyTracker.cs b/src/NUnitEngine/nunit.engine.core/Internal/ExtensionAssemblyTracker.cs new file mode 100644 index 000000000..96b3c153d --- /dev/null +++ b/src/NUnitEngine/nunit.engine.core/Internal/ExtensionAssemblyTracker.cs @@ -0,0 +1,25 @@ +// Copyright (c) Charlie Poole, Rob Prouse and Contributors. MIT License - see LICENSE.txt + +using NUnit.Engine.Extensibility; +using System.Collections.Generic; + +namespace NUnit.Engine.Internal +{ + /// + /// This is a simple utility class used by the ExtensionManager to keep track of ExtensionAssemblies. + /// It is a List of ExtensionAssemblies and also provides indices by file path and assembly name. + /// It allows writing tests to show that no duplicate extension assemblies are loaded. + /// + internal class ExtensionAssemblyTracker : List + { + public Dictionary ByPath = new Dictionary(); + public Dictionary ByName = new Dictionary(); + + public new void Add(ExtensionAssembly assembly) + { + base.Add(assembly); + ByPath.Add(assembly.FilePath, assembly); + ByName.Add(assembly.AssemblyName, assembly); + } + } +} diff --git a/src/NUnitEngine/nunit.engine.core/Services/ExtensionManager.cs b/src/NUnitEngine/nunit.engine.core/Services/ExtensionManager.cs index b033414cd..f8c1d8784 100644 --- a/src/NUnitEngine/nunit.engine.core/Services/ExtensionManager.cs +++ b/src/NUnitEngine/nunit.engine.core/Services/ExtensionManager.cs @@ -10,6 +10,7 @@ using NUnit.Engine.Internal.FileSystemAccess; using NUnit.Engine.Internal.FileSystemAccess.Default; using System.Linq; +using System.Diagnostics; namespace NUnit.Engine.Services { @@ -27,26 +28,21 @@ public class ExtensionManager private readonly List _extensionPoints = new List(); // Index to ExtensionPoints based on the Path - private readonly Dictionary _pathIndex = new Dictionary(); + private readonly Dictionary _extensionPointIndex = new Dictionary(); // List of ExtensionNodes for all extensions discovered. private List _extensions = new List(); private bool _extensionsAreLoaded; - // List of candidate ExtensionAssemblies, which should be examined for extensions. - // This list must be completely built before we examine any of the assemblies, since - // it's possible that the same assembly may be found in multiple versions. - private readonly List _candidateAssemblies = new List(); - + // AssemblyTracker is a List of candidate ExtensionAssemblies, with built-in indexing + // by file path and assembly name, eliminating the need to update indices separately. + private readonly ExtensionAssemblyTracker _assemblies = new ExtensionAssemblyTracker(); + // List of all extensionDirectories specified on command-line or in environment, - // used to ignore duplicate calls to FindExtensions. + // used to ignore duplicate calls to FindExtensionAssemblies. private readonly List _extensionDirectories = new List(); - // Dictionary containing all directory paths and assembly paths already processed, - // used to prevent processing a directory or assembly more than once. - private readonly Dictionary _visited = new Dictionary(); - public ExtensionManager() : this(new FileSystem()) { @@ -97,7 +93,7 @@ public virtual void FindExtensionPoints(params Assembly[] targetAssemblies) foreach (ExtensionPointAttribute attr in assembly.GetCustomAttributes(typeof(ExtensionPointAttribute), false)) { - if (_pathIndex.ContainsKey(attr.Path)) + if (_extensionPointIndex.ContainsKey(attr.Path)) { string msg = string.Format( "The Path {0} is already in use for another extension point.", @@ -111,7 +107,7 @@ public virtual void FindExtensionPoints(params Assembly[] targetAssemblies) }; _extensionPoints.Add(ep); - _pathIndex.Add(ep.Path, ep); + _extensionPointIndex.Add(ep.Path, ep); log.Info(" Found ExtensionPoint: Path={0}, Type={1}", ep.Path, ep.TypeName); } @@ -122,7 +118,7 @@ public virtual void FindExtensionPoints(params Assembly[] targetAssemblies) { string path = attr.Path ?? "/NUnit/Engine/TypeExtensions/" + type.Name; - if (_pathIndex.ContainsKey(path)) + if (_extensionPointIndex.ContainsKey(path)) { string msg = string.Format( "The Path {0} is already in use for another extension point.", @@ -136,7 +132,7 @@ public virtual void FindExtensionPoints(params Assembly[] targetAssemblies) }; _extensionPoints.Add(ep); - _pathIndex.Add(path, ep); + _extensionPointIndex.Add(path, ep); log.Info(" Found ExtensionPoint: Path={0}, Type={1}", ep.Path, ep.TypeName); } @@ -179,22 +175,16 @@ public void FindExtensionAssemblies(Assembly hostAssembly) ? new[] { "nunit-extension-*/**/tools/", "nunit-extension-*/**/tools/*/" } : new[] { "NUnit.Extension.*/**/tools/", "NUnit.Extension.*/**/tools/*/" }; - FindExtensionAssemblies(hostAssembly, extensionPatterns); - } - /// - /// We can only load extensions after all candidate assemblies are identified. - /// This method is called internally to ensure the load happens before any - /// extensions are used. - /// - private void EnsureExtensionsAreLoaded() - { - if (!_extensionsAreLoaded) + IDirectory startDir = _fileSystem.GetDirectory(AssemblyHelper.GetDirectoryName(hostAssembly)); + + while (startDir != null) { - _extensionsAreLoaded = true; + foreach (var pattern in extensionPatterns) + foreach (var dir in _directoryFinder.GetDirectories(startDir, pattern)) + ProcessDirectory(dir, true); - foreach (var candidate in _candidateAssemblies) - FindExtensionsInAssembly(candidate); + startDir = startDir.Parent; } } @@ -203,7 +193,7 @@ private void EnsureExtensionsAreLoaded() /// public IExtensionPoint GetExtensionPoint(string path) { - return _pathIndex.ContainsKey(path) ? _pathIndex[path] : null; + return _extensionPointIndex.ContainsKey(path) ? _extensionPointIndex[path] : null; } /// @@ -323,23 +313,18 @@ private ExtensionPoint DeduceExtensionPointFromType(TypeReference typeRef) } /// - /// Find candidate extension assemblies for a given host assembly, - /// using the provided list of patterns to direct the search using - /// a built-in algorithm. + /// We can only load extensions after all candidate assemblies are identified. + /// This method is called internally to ensure the load happens before any + /// extensions are used. /// - /// An assembly that supports extensions - /// A list of patterns used to identify potential candidates. - private void FindExtensionAssemblies(Assembly hostAssembly, string[] patterns) + private void EnsureExtensionsAreLoaded() { - IDirectory startDir = _fileSystem.GetDirectory(AssemblyHelper.GetDirectoryName(hostAssembly)); - - while (startDir != null) + if (!_extensionsAreLoaded) { - foreach (var pattern in patterns) - foreach (var dir in _directoryFinder.GetDirectories(startDir, pattern)) - ProcessDirectory(dir, true); + _extensionsAreLoaded = true; - startDir = startDir.Parent; + foreach (var candidate in _assemblies) + FindExtensionsInAssembly(candidate); } } @@ -354,20 +339,15 @@ private void ProcessDirectory(IDirectory startDir, bool fromWildCard) if (WasVisited(directoryName, fromWildCard)) { log.Warning($"Skipping directory '{directoryName}' because it was already visited."); + return; } - else - { - log.Info($"Scanning directory '{directoryName}' for extensions."); - MarkAsVisited(directoryName, fromWildCard); - if (ProcessAddinsFiles(startDir, fromWildCard) == 0) - { - foreach (var file in startDir.GetFiles("*.dll")) - { - ProcessCandidateAssembly(file.FullName, true); - } - } - } + log.Info($"Scanning directory '{directoryName}' for extensions."); + Visit(directoryName, fromWildCard); + + if (ProcessAddinsFiles(startDir, fromWildCard) == 0) + foreach (var file in startDir.GetFiles("*.dll")) + ProcessCandidateAssembly(file.FullName, true); } /// @@ -432,32 +412,29 @@ private void ProcessAddinsFile(IFile addinsFile, bool fromWildCard) private void ProcessCandidateAssembly(string filePath, bool fromWildCard) { - if (WasVisited(filePath, fromWildCard)) + // Did we already process this file? + if (_assemblies.ByPath.ContainsKey(filePath)) return; - MarkAsVisited(filePath, fromWildCard); - try { - var candidate = new ExtensionAssembly(filePath, fromWildCard); + var candidateAssembly = new ExtensionAssembly(filePath, fromWildCard); + string assemblyName = candidateAssembly.AssemblyName; - if (!CanLoadTargetFramework(Assembly.GetEntryAssembly(), candidate)) + // We never add assemblies unless the host can load them + if (!CanLoadTargetFramework(Assembly.GetEntryAssembly(), candidateAssembly)) return; - - for (var i = 0; i < _candidateAssemblies.Count; i++) + + // Do we already have a copy of the same assembly at a diffrent path? + if (_assemblies.ByName.ContainsKey(assemblyName)) { - var assembly = _candidateAssemblies[i]; + if (candidateAssembly.IsBetterVersionOf(_assemblies.ByName[assemblyName])) + _assemblies.ByName[assemblyName] = candidateAssembly; - if (candidate.IsDuplicateOf(assembly)) - { - if (candidate.IsBetterVersionOf(assembly)) - _candidateAssemblies[i] = candidate; - - return; - } + return; } - _candidateAssemblies.Add(candidate); + _assemblies.Add(candidateAssembly); } catch (BadImageFormatException e) { @@ -471,12 +448,15 @@ private void ProcessCandidateAssembly(string filePath, bool fromWildCard) } } + // Dictionary containing all directory paths already visited. + private readonly Dictionary _visited = new Dictionary(); + private bool WasVisited(string path, bool fromWildcard) { return _visited.ContainsKey($"path={path}_visited={fromWildcard}"); } - private void MarkAsVisited(string path, bool fromWildcard) + private void Visit(string path, bool fromWildcard) { _visited.Add($"path={path}_visited={fromWildcard}", null); } @@ -486,35 +466,35 @@ private void MarkAsVisited(string path, bool fromWildcard) /// For each extension, create an ExtensionNode and link it to the /// correct ExtensionPoint. Public for testing. /// - internal void FindExtensionsInAssembly(ExtensionAssembly assembly) + internal void FindExtensionsInAssembly(ExtensionAssembly extensionAssembly) { - log.Info($"Scanning {assembly.FilePath} for Extensions"); + log.Info($"Scanning {extensionAssembly.FilePath} for Extensions"); - if (!CanLoadTargetFramework(Assembly.GetEntryAssembly(), assembly)) + if (!CanLoadTargetFramework(Assembly.GetEntryAssembly(), extensionAssembly)) { - log.Info($"{assembly.FilePath} cannot be loaded on this runtime"); + log.Info($"{extensionAssembly.FilePath} cannot be loaded on this runtime"); return; } IRuntimeFramework assemblyTargetFramework = null; #if NETFRAMEWORK var currentFramework = RuntimeFramework.CurrentFramework; - assemblyTargetFramework = assembly.TargetFramework; + assemblyTargetFramework = extensionAssembly.TargetFramework; if (!currentFramework.CanLoad(assemblyTargetFramework)) { - if (!assembly.FromWildCard) + if (!extensionAssembly.FromWildCard) { - throw new NUnitEngineException($"Extension {assembly.FilePath} targets {assemblyTargetFramework.DisplayName}, which is not available."); + throw new NUnitEngineException($"Extension {extensionAssembly.FilePath} targets {assemblyTargetFramework.DisplayName}, which is not available."); } else { - log.Info($"Assembly {assembly.FilePath} targets {assemblyTargetFramework.DisplayName}, which is not available. Assembly found via wildcard."); + log.Info($"Assembly {extensionAssembly.FilePath} targets {assemblyTargetFramework.DisplayName}, which is not available. Assembly found via wildcard."); return; } } #endif - foreach (var extensionType in assembly.MainModule.GetTypes()) + foreach (var extensionType in extensionAssembly.Assembly.MainModule.GetTypes()) { CustomAttribute extensionAttr = extensionType.GetAttribute("NUnit.Engine.Extensibility.ExtensionAttribute"); @@ -534,7 +514,7 @@ internal void FindExtensionsInAssembly(ExtensionAssembly assembly) } } - var node = new ExtensionNode(assembly.FilePath, assembly.AssemblyVersion, extensionType.FullName, assemblyTargetFramework) + var node = new ExtensionNode(extensionAssembly.FilePath, extensionAssembly.AssemblyVersion, extensionType.FullName, assemblyTargetFramework) { Path = extensionAttr.GetNamedArgument("Path") as string, Description = extensionAttr.GetNamedArgument("Description") as string @@ -679,9 +659,9 @@ private System.Runtime.Versioning.FrameworkName GetTargetRuntime(string filePath public void Dispose() { // Make sure all assemblies release the underlying file streams. - foreach (var assembly in _candidateAssemblies) + foreach (var candidate in _assemblies) { - assembly.Dispose(); + candidate.Dispose(); } } }