improve comment parsing; more combinatorial testing

dotnet · Sep 8, 2023 · ed81852 · ed81852
1 parent 86c8724
commit ed81852
Show file tree

Hide file tree

Showing 13 changed files with 189 additions and 191 deletions.
diff --git a/src/Microsoft.DotNet.Interactive.Documents/CodeSubmission.cs b/src/Microsoft.DotNet.Interactive.Documents/CodeSubmission.cs
@@ -53,7 +53,7 @@ public static InteractiveDocument Parse(
 
                 metadata = JsonSerializer.Deserialize<Dictionary<string, object>>(metadataString, InteractiveDocument.JsonSerializerOptions);
 
-                if (InteractiveDocument.TryGetKernelInfoFromMetadata(metadata, out var kernelInfoFromMetadata))
+                if (InteractiveDocument.TryGetKernelInfosFromMetadata(metadata, out var kernelInfoFromMetadata))
                 {
                     InteractiveDocument.MergeKernelInfos(kernelInfos, kernelInfoFromMetadata);
                     document.Metadata["kernelInfo"] = kernelInfoFromMetadata;

diff --git a/src/Microsoft.DotNet.Interactive.Documents/InteractiveDocument.cs b/src/Microsoft.DotNet.Interactive.Documents/InteractiveDocument.cs
@@ -65,7 +65,7 @@ public async IAsyncEnumerable<InteractiveDocument> GetImportsAsync(bool recursiv
     {
         EnsureImportFieldParserIsInitialized();
 
-        if (!TryGetKernelInfoFromMetadata(Metadata, out var kernelInfos))
+        if (!TryGetKernelInfosFromMetadata(Metadata, out var kernelInfos))
         {
             kernelInfos = new();
         }
@@ -198,7 +198,8 @@ public static async Task<InteractiveDocument> LoadAsync(
 
     public string? GetDefaultKernelName()
     {
-        if (TryGetKernelInfoFromMetadata(Metadata, out var kernelInfo))
+        // FIX: (GetDefaultKernelName) remove from public API
+        if (TryGetKernelInfosFromMetadata(Metadata, out var kernelInfo))
         {
             return kernelInfo.DefaultKernelName;
         }
@@ -208,7 +209,7 @@ public static async Task<InteractiveDocument> LoadAsync(
 
     internal string? GetDefaultKernelName(KernelInfoCollection kernelInfos)
     {
-        if (TryGetKernelInfoFromMetadata(Metadata, out var kernelInfoCollection))
+        if (TryGetKernelInfosFromMetadata(Metadata, out var kernelInfoCollection))
         {
             return kernelInfoCollection.DefaultKernelName;
         }
@@ -243,7 +244,7 @@ public static async Task<InteractiveDocument> LoadAsync(
 
     internal static void MergeKernelInfos(InteractiveDocument document, KernelInfoCollection kernelInfos)
     {
-        if (TryGetKernelInfoFromMetadata(document.Metadata, out var kernelInfoCollection))
+        if (TryGetKernelInfosFromMetadata(document.Metadata, out var kernelInfoCollection))
         {
             MergeKernelInfos(kernelInfoCollection, kernelInfos);
         }
@@ -264,7 +265,7 @@ internal static void MergeKernelInfos(KernelInfoCollection destination, KernelIn
         destination.AddRange(source.Where(ki => added.Add(ki.Name)));
     }
 
-    internal static bool TryGetKernelInfoFromMetadata(
+    internal static bool TryGetKernelInfosFromMetadata(
         IDictionary<string, object>? metadata,
         [NotNullWhen(true)] out KernelInfoCollection? kernelInfo)
     {

diff --git a/src/Microsoft.DotNet.Interactive.Documents/InteractiveDocumentElement.cs b/src/Microsoft.DotNet.Interactive.Documents/InteractiveDocumentElement.cs
@@ -6,7 +6,7 @@
 
 namespace Microsoft.DotNet.Interactive.Documents;
 
-public class InteractiveDocumentElement
+public sealed class InteractiveDocumentElement
 {
     [JsonConstructor]
     public InteractiveDocumentElement()

diff --git a/src/Microsoft.DotNet.Interactive.Formatting/PlainTextSummaryFormatter.cs b/src/Microsoft.DotNet.Interactive.Formatting/PlainTextSummaryFormatter.cs
@@ -8,7 +8,6 @@ namespace Microsoft.DotNet.Interactive.Formatting;
 
 public static class PlainTextSummaryFormatter
 {
-    // FIX: (PlainTextSummaryFormatter) rename this
     public const string MimeType = "text/plain+summary";
 
     public static ITypeFormatter GetPreferredFormatterFor(Type type)

diff --git a/src/Microsoft.DotNet.Interactive.HttpRequest.Tests/ParserTests.Body.cs b/src/Microsoft.DotNet.Interactive.HttpRequest.Tests/ParserTests.Body.cs
@@ -1,7 +1,7 @@
 // Copyright (c) .NET Foundation and contributors. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
-using System.Linq;
+using System;
 using FluentAssertions;
 using Microsoft.DotNet.Interactive.HttpRequest.Tests.Utility;
 using Xunit;
@@ -12,27 +12,6 @@ public partial class ParserTests
 {
     public class Body
     {
-        [Fact]
-        public void body_separator_is_present()
-        {
-            var result = Parse(
-                """
-            POST https://example.com/comments HTTP/1.1
-            Content-Type: application/xml
-            Authorization: token xxx
-
-            <request>
-                <name>sample</name>
-                <time>Wed, 21 Oct 2015 18:27:50 GMT</time>
-            </request>
-            """);
-
-            var requestNode = result.SyntaxTree.RootNode
-                                    .ChildNodes.Should().ContainSingle<HttpRequestNode>().Which;
-
-            requestNode.BodySeparatorNode.ChildTokens.First().Kind.Should().Be(HttpTokenKind.NewLine);
-        }
-
         [Fact]
         public void body_is_parsed_correctly_when_headers_are_not_present()
         {
@@ -87,5 +66,23 @@ public void multiple_new_lines_before_body_are_parsed_correctly()
                 </request>
                 """);
         }
+
+        [Fact]
+        public void Whitespace_after_headers_is_not_parsed_as_body()
+        {
+            var code = """
+
+                hptps://example.com 
+                Accept: */*
+
+
+
+
+                """;
+
+            var result = Parse(code);
+
+            result.SyntaxTree.RootNode.DescendantNodesAndTokens().Should().NotContain(n => n is HttpBodyNode);
+        }
     }
 }
diff --git a/src/Microsoft.DotNet.Interactive.HttpRequest.Tests/ParserTests.Combinatorial.cs b/src/Microsoft.DotNet.Interactive.HttpRequest.Tests/ParserTests.Combinatorial.cs
@@ -5,6 +5,7 @@
 using System.Collections.Generic;
 using System.Linq;
 using FluentAssertions;
+using Microsoft.DotNet.Interactive.Formatting;
 using Microsoft.DotNet.Interactive.HttpRequest.Tests.Utility;
 using Xunit;
 using Xunit.Abstractions;
@@ -13,6 +14,8 @@ namespace Microsoft.DotNet.Interactive.HttpRequest.Tests;
 
 public partial class ParserTests
 {
+    private readonly ITestOutputHelper _output;
+
     public class Combinatorial
     {
         private readonly ITestOutputHelper _output;

diff --git a/src/Microsoft.DotNet.Interactive.HttpRequest.Tests/ParserTests.Headers.cs b/src/Microsoft.DotNet.Interactive.HttpRequest.Tests/ParserTests.Headers.cs
@@ -1,6 +1,7 @@
-// Copyright (c) .NET Foundation and contributors. All rights reserved.
+// Copyright (c) .NET Foundation and contributors. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
+using System;
 using System.Linq;
 using FluentAssertions;
 using Microsoft.DotNet.Interactive.HttpRequest.Tests.Utility;
@@ -74,6 +75,25 @@ public void header_separator_is_parsed()
                   .Which.Text.Should().Be(":");
         }
 
+        [Fact]
+        public void Comments_can_precede_headers()
+        {
+            var code = """
+                POST https://example.com
+                # this is a comment
+                Accept: */*
+                Accept-Encoding: gzip, deflate, br
+                """;
+
+            var result = Parse(code);
+
+            var requestNode = result.SyntaxTree.RootNode.DescendantNodesAndTokens()
+                                    .Should().ContainSingle<HttpRequestNode>().Which;
+
+            requestNode.DescendantNodesAndTokens().Should().ContainSingle<HttpCommentNode>().Which.Text.Should().Be("# this is a comment");
+            requestNode.ChildNodes.Should().ContainSingle<HttpHeadersNode>();
+        }
+
         [Fact]
         public void headers_are_parsed_correctly()
         {

diff --git a/src/Microsoft.DotNet.Interactive.HttpRequest.Tests/ParserTests.Url.cs b/src/Microsoft.DotNet.Interactive.HttpRequest.Tests/ParserTests.Url.cs
@@ -32,9 +32,9 @@ public void newline_is_legal_at_the_after_url()
 
                 """);
 
-            result.SyntaxTree.RootNode
-                  .ChildNodes.Should().ContainSingle<HttpRequestNode>().Which
-                  .ChildTokens.Last().Kind.Should().Be(HttpTokenKind.NewLine);
+            result.SyntaxTree.RootNode.ChildNodes.Should().ContainSingle<HttpRequestNode>();
+
+            result.GetDiagnostics().Should().BeEmpty();
         }
 
         [Theory]

diff --git a/src/Microsoft.DotNet.Interactive.HttpRequest.Tests/ParserTests.cs b/src/Microsoft.DotNet.Interactive.HttpRequest.Tests/ParserTests.cs
@@ -2,17 +2,20 @@
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 using System;
+using System.Linq;
 using FluentAssertions;
 using FluentAssertions.Execution;
+using Xunit.Abstractions;
 
 namespace Microsoft.DotNet.Interactive.HttpRequest.Tests;
 
 public partial class ParserTests : IDisposable
 {
     private readonly AssertionScope _assertionScope;
 
-    public ParserTests()
+    public ParserTests(ITestOutputHelper output)
     {
+        _output = output;
         _assertionScope = new AssertionScope();
     }
 
@@ -24,9 +27,9 @@ public void Dispose()
     private static HttpRequestParseResult Parse(string code)
     {
         var result = HttpRequestParser.Parse(code);
-
-        result.SyntaxTree.RootNode.FullText.Should().Be(code);
 
+        result.SyntaxTree.RootNode.FullText.Should().Be(code);
+
         return result;
     }
 }

diff --git a/src/Microsoft.DotNet.Interactive.HttpRequest.Tests/Utility/HttpBodyNodeSyntaxSpec.cs b/src/Microsoft.DotNet.Interactive.HttpRequest.Tests/Utility/HttpBodyNodeSyntaxSpec.cs
@@ -184,7 +184,6 @@ public override string ToString()
         sb.Append(MaybeWhitespace());
         sb.AppendLine();
         sb.Append(MaybeLineComment()); 
-        sb.Append(MaybeNewLines());
 
         if (HeadersSection is not null)
         {
@@ -201,8 +200,6 @@ public override string ToString()
             sb.Append(MaybeNewLines());
         }
 
-        sb.Append(MaybeLineComment());
-
         return sb.ToString();
     }
 
@@ -227,8 +224,8 @@ private string MaybeNewLines() =>
     private string MaybeLineComment() =>
         ExtraTriviaRandomizer?.NextDouble() switch
         {
-            // FIX: (MaybeLineComment) 
-            < 0 => "# random line comment",
+            < .2 => "# random line comment followed by a LF\n",
+            > .2 and < .4 => "# random line comment followed by a CRLF\r\n",
             _ => ""
         };
 }

diff --git a/src/Microsoft.DotNet.Interactive.HttpRequestParser/Parser/HttpBodySeparatorNode.cs b/src/Microsoft.DotNet.Interactive.HttpRequestParser/Parser/HttpBodySeparatorNode.cs
diff --git a/src/Microsoft.DotNet.Interactive.HttpRequestParser/Parser/HttpRequestNode.cs b/src/Microsoft.DotNet.Interactive.HttpRequestParser/Parser/HttpRequestNode.cs
@@ -27,8 +27,6 @@ internal HttpRequestNode(SourceText sourceText, HttpSyntaxTree? syntaxTree) : ba
 
     public HttpHeadersNode? HeadersNode { get; private set; }
 
-    public HttpBodySeparatorNode? BodySeparatorNode { get; private set; }
-
     public HttpBodyNode? BodyNode { get; private set; }
 
     public void Add(HttpMethodNode node)
@@ -71,16 +69,6 @@ public void Add(HttpHeadersNode node)
         AddInternal(node);
     }
 
-    public void Add(HttpBodySeparatorNode node)
-    {
-        if (BodySeparatorNode is not null)
-        {
-            throw new InvalidOperationException($"{nameof(BodySeparatorNode)} was already added.");
-        }
-        BodySeparatorNode = node;
-        AddInternal(node);
-    }
-
     public void Add(HttpBodyNode node)
     {
         if (BodyNode is not null)