From 7bee39932de56b35ae33e3588ddea6dd0fe07fe1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Bj=C3=B6rkstr=C3=B6m?= Date: Sat, 12 Aug 2017 23:44:41 +0300 Subject: [PATCH] (GH-1746) ScriptAnalyzer will not throw on error. - ScriptAnalyzer will not throw on error. Instead the result will indicate if analysis succeeded or not. A list of errors is also present in the analyzer result. - Fixes #1746 --- .../Analysis/ScriptAnalyzerResultTests.cs | 27 ++++++++++ .../Scripting/Analysis/ScriptAnalyzerTests.cs | 53 +++++++++++++++++-- .../Analysis/IScriptAnalyzerContext.cs | 6 +++ .../Scripting/Analysis/ScriptAnalyzer.cs | 3 +- .../Analysis/ScriptAnalyzerContext.cs | 16 +++++- .../Scripting/Analysis/ScriptAnalyzerError.cs | 48 +++++++++++++++++ .../Analysis/ScriptAnalyzerResult.cs | 17 +++++- .../Processors/UriDirectiveProcessor.cs | 12 ++++- src/Cake.Core/Scripting/ScriptRunner.cs | 11 ++++ 9 files changed, 183 insertions(+), 10 deletions(-) create mode 100644 src/Cake.Core/Scripting/Analysis/ScriptAnalyzerError.cs diff --git a/src/Cake.Core.Tests/Unit/Scripting/Analysis/ScriptAnalyzerResultTests.cs b/src/Cake.Core.Tests/Unit/Scripting/Analysis/ScriptAnalyzerResultTests.cs index ea61230e49..e8272740ff 100644 --- a/src/Cake.Core.Tests/Unit/Scripting/Analysis/ScriptAnalyzerResultTests.cs +++ b/src/Cake.Core.Tests/Unit/Scripting/Analysis/ScriptAnalyzerResultTests.cs @@ -107,6 +107,33 @@ public void Should_Populate_Tools_From_Provided_Script() Assert.Contains(result.Tools, package => package.OriginalString == "nuget:?package=First.Package"); Assert.Contains(result.Tools, package => package.OriginalString == "nuget:?package=Second.Package"); } + + [Fact] + public void Should_Set_Succeeded_To_False_If_Any_Errors() + { + // Given + var script = new ScriptInformation("./build.cake"); + var error = new ScriptAnalyzerError("./build.cake", 1, "error message"); + + // When + var result = new ScriptAnalyzerResult(script, new List(), new List { error }); + + // Then + Assert.False(result.Succeeded); + } + + [Fact] + public void Should_Set_Succeeded_To_True_If_Not_Any_Errors() + { + // Given + var script = new ScriptInformation("./build.cake"); + + // When + var result = new ScriptAnalyzerResult(script, new List(), new List()); + + // Then + Assert.True(result.Succeeded); + } } } } \ No newline at end of file diff --git a/src/Cake.Core.Tests/Unit/Scripting/Analysis/ScriptAnalyzerTests.cs b/src/Cake.Core.Tests/Unit/Scripting/Analysis/ScriptAnalyzerTests.cs index 12f2fd8319..fd73ee200f 100644 --- a/src/Cake.Core.Tests/Unit/Scripting/Analysis/ScriptAnalyzerTests.cs +++ b/src/Cake.Core.Tests/Unit/Scripting/Analysis/ScriptAnalyzerTests.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System.Linq; +using Cake.Core.Scripting.Processors.Loading; using Cake.Core.Tests.Fixtures; using Xunit; @@ -71,17 +72,18 @@ public void Should_Throw_If_File_Path_Is_Null() } [Fact] - public void Should_Throw_If_Script_Was_Not_Found() + public void Should_Return_Error_If_Script_Was_Not_Found() { // Given var fixture = new ScriptAnalyzerFixture(); // When - var result = Record.Exception(() => fixture.Analyze("/Working/notfound.cake")); + var result = fixture.Analyze("/Working/notfound.cake"); // Then - Assert.IsType(result); - Assert.Equal("Could not find script '/Working/notfound.cake'.", result?.Message); + Assert.False(result.Succeeded); + Assert.Equal(1, result.Errors.Count); + Assert.Equal("Could not find script '/Working/notfound.cake'.", result.Errors[0].Message); } [Fact] @@ -324,6 +326,49 @@ public void Should_Process_Break_Directive() Assert.Equal(result.Lines[0], "#line 1 \"/Working/script.cake\""); Assert.Equal(result.Lines[1], @"if (System.Diagnostics.Debugger.IsAttached) { System.Diagnostics.Debugger.Break(); }"); } + + [Fact] + public void Should_Log_Error_Location() + { + // Given + var fixture = new ScriptAnalyzerFixture(); + fixture.Providers.Add(new FileLoadDirectiveProvider()); + fixture.GivenScriptExist("/Working/script.cake", "#load \"local:?pat\""); + + // When + var result = fixture.Analyze("/Working/script.cake"); + + // Then + Assert.Equal(2, result.Lines.Count); + Assert.Equal("#line 1 \"/Working/script.cake\"", result.Lines[0]); + Assert.Equal("// #load \"local:?pat\"", result.Lines[1]); + Assert.False(result.Succeeded); + Assert.Equal(1, result.Errors.Count); + Assert.Equal("/Working/script.cake", result.Errors[0].File.FullPath); + Assert.Equal(1, result.Errors[0].Line); + Assert.Equal("Query string for #load is missing parameter 'path'.", result.Errors[0].Message); + } + + [Fact] + public void Should_Log_Error_Location_In_Loaded_Script() + { + // Given + var fixture = new ScriptAnalyzerFixture(); + fixture.Providers.Add(new FileLoadDirectiveProvider()); + fixture.GivenScriptExist("/Working/script.cake", "#load \"local:?path=script2.cake\""); + fixture.GivenScriptExist("/Working/script2.cake", "\n#load \"local:?path=1&path=2\""); + + // When + var result = fixture.Analyze("/Working/script.cake"); + + // Then + Assert.Equal(6, result.Lines.Count); + Assert.False(result.Succeeded); + Assert.Equal(1, result.Errors.Count); + Assert.Equal("/Working/script2.cake", result.Errors[0].File.FullPath); + Assert.Equal(2, result.Errors[0].Line); + Assert.Equal("Query string for #load contains more than one parameter 'path'.", result.Errors[0].Message); + } } } } \ No newline at end of file diff --git a/src/Cake.Core/Scripting/Analysis/IScriptAnalyzerContext.cs b/src/Cake.Core/Scripting/Analysis/IScriptAnalyzerContext.cs index 2bcb829a4d..4576f7272f 100644 --- a/src/Cake.Core/Scripting/Analysis/IScriptAnalyzerContext.cs +++ b/src/Cake.Core/Scripting/Analysis/IScriptAnalyzerContext.cs @@ -33,5 +33,11 @@ public interface IScriptAnalyzerContext /// /// The script line to add. void AddScriptLine(string line); + + /// + /// Adds a script error to the result. + /// + /// The script error to add. + void AddScriptError(string error); } } \ No newline at end of file diff --git a/src/Cake.Core/Scripting/Analysis/ScriptAnalyzer.cs b/src/Cake.Core/Scripting/Analysis/ScriptAnalyzer.cs index b2614fdd93..2d311880f4 100644 --- a/src/Cake.Core/Scripting/Analysis/ScriptAnalyzer.cs +++ b/src/Cake.Core/Scripting/Analysis/ScriptAnalyzer.cs @@ -92,7 +92,8 @@ public ScriptAnalyzerResult Analyze(FilePath path) // Create and return the results. return new ScriptAnalyzerResult( context.Current, - context.Lines); + context.Lines, + context.Errors); } [SuppressMessage("ReSharper", "ConvertIfStatementToConditionalTernaryExpression")] diff --git a/src/Cake.Core/Scripting/Analysis/ScriptAnalyzerContext.cs b/src/Cake.Core/Scripting/Analysis/ScriptAnalyzerContext.cs index 1a8bf0c742..717acda090 100644 --- a/src/Cake.Core/Scripting/Analysis/ScriptAnalyzerContext.cs +++ b/src/Cake.Core/Scripting/Analysis/ScriptAnalyzerContext.cs @@ -20,6 +20,7 @@ internal sealed class ScriptAnalyzerContext : IScriptAnalyzerContext private readonly Stack _stack; private readonly List _lines; private readonly HashSet _processedScripts; + private readonly List _errors; private ScriptInformation _current; // ReSharper disable once ConvertToAutoProperty @@ -29,6 +30,8 @@ internal sealed class ScriptAnalyzerContext : IScriptAnalyzerContext public IReadOnlyList Lines => _lines; + public IReadOnlyList Errors => _errors; + public ScriptAnalyzerContext( IFileSystem fileSystem, ICakeEnvironment environment, @@ -44,6 +47,7 @@ public ScriptAnalyzerContext( _processedScripts = new HashSet(new PathComparer(_environment)); _stack = new Stack(); _lines = new List(); + _errors = new List(); } public void Analyze(FilePath path) @@ -60,7 +64,9 @@ public void Analyze(FilePath path) { const string format = "Could not find script '{0}'."; var message = string.Format(CultureInfo.InvariantCulture, format, path.FullPath); - throw new CakeException(message); + _errors.Add(new ScriptAnalyzerError(path, 0, message)); + _current = new ScriptInformation(path); + return; } if (_processedScripts.Contains(path)) @@ -86,6 +92,14 @@ public void AddScriptLine(string line) _current.Lines.Add(line); } + public void AddScriptError(string error) + { + if (error != null) + { + _errors.Add(new ScriptAnalyzerError(_current.Path, _current.Lines.Count + 1, error)); + } + } + public void Push(FilePath path) { var script = new ScriptInformation(path); diff --git a/src/Cake.Core/Scripting/Analysis/ScriptAnalyzerError.cs b/src/Cake.Core/Scripting/Analysis/ScriptAnalyzerError.cs new file mode 100644 index 0000000000..bb0474c575 --- /dev/null +++ b/src/Cake.Core/Scripting/Analysis/ScriptAnalyzerError.cs @@ -0,0 +1,48 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using Cake.Core.IO; + +namespace Cake.Core.Scripting.Analysis +{ + /// + /// Represents a script analysis error. + /// + public sealed class ScriptAnalyzerError + { + /// + /// Gets the file containing the error. + /// + public FilePath File { get; } + + /// + /// Gets the line number for the error. + /// + public int Line { get; } + + /// + /// Gets the error message. + /// + public string Message { get; } + + /// + /// Initializes a new instance of the class. + /// + /// The file containing the error. + /// The line number for the error. + /// The error message. + public ScriptAnalyzerError(FilePath file, int line, string message) + { + if (file == null) + { + throw new ArgumentNullException(nameof(file)); + } + + File = file; + Line = line; + Message = message; + } + } +} \ No newline at end of file diff --git a/src/Cake.Core/Scripting/Analysis/ScriptAnalyzerResult.cs b/src/Cake.Core/Scripting/Analysis/ScriptAnalyzerResult.cs index e8505d4fe4..2b9b5c7387 100644 --- a/src/Cake.Core/Scripting/Analysis/ScriptAnalyzerResult.cs +++ b/src/Cake.Core/Scripting/Analysis/ScriptAnalyzerResult.cs @@ -59,12 +59,23 @@ public sealed class ScriptAnalyzerResult /// The tools. public HashSet Tools { get; } + /// + /// Gets a value indicating whether to analysis succeded without errors. + /// + public bool Succeeded { get; } + + /// + /// Gets the list of analyzer errors. + /// + public IReadOnlyList Errors { get; } + /// /// Initializes a new instance of the class. /// /// The script. /// The merged script lines. - public ScriptAnalyzerResult(IScriptInformation script, IReadOnlyList lines) + /// The analyzer errors. + public ScriptAnalyzerResult(IScriptInformation script, IReadOnlyList lines, IReadOnlyList errors = null) { Script = script; Lines = lines; @@ -73,9 +84,11 @@ public ScriptAnalyzerResult(IScriptInformation script, IReadOnlyList lin _usingAliases = new HashSet(Collect(script, i => i.UsingAliases)); Tools = new HashSet(Collect(script, i => i.Tools)); Addins = new HashSet(Collect(script, i => i.Addins)); + Errors = errors ?? new List(0); + Succeeded = Errors.Count == 0; } - private IEnumerable Collect(IScriptInformation script, Func> collector) + private static IEnumerable Collect(IScriptInformation script, Func> collector) { var stack = new Stack(); stack.Push(script); diff --git a/src/Cake.Core/Scripting/Processors/UriDirectiveProcessor.cs b/src/Cake.Core/Scripting/Processors/UriDirectiveProcessor.cs index de0d5a0965..abdc62f443 100644 --- a/src/Cake.Core/Scripting/Processors/UriDirectiveProcessor.cs +++ b/src/Cake.Core/Scripting/Processors/UriDirectiveProcessor.cs @@ -37,8 +37,16 @@ public sealed override bool Process(IScriptAnalyzerContext context, string line, var uri = ParseUriFromTokens(tokens); if (uri != null) { - // Add the URI to the context. - AddToContext(context, uri); + try + { + // Add the URI to the context. + AddToContext(context, uri); + } + catch (Exception e) + { + // Add any errors to context + context.AddScriptError(e.Message); + } // Return success. return true; diff --git a/src/Cake.Core/Scripting/ScriptRunner.cs b/src/Cake.Core/Scripting/ScriptRunner.cs index dbf4219775..42f37f9a2f 100644 --- a/src/Cake.Core/Scripting/ScriptRunner.cs +++ b/src/Cake.Core/Scripting/ScriptRunner.cs @@ -131,6 +131,17 @@ public void Run(IScriptHost host, FilePath scriptPath, IDictionary