Expand combinatorial testing of HTTP parser by jonsequitur · Pull Request #3166 · dotnet/interactive

jonsequitur · 2023-09-08T20:29:53Z

This PR adds more combinatorial testing and fixes a number of bugs discovered in the process, with scenario-specific tests added in each case. I've removed the HttpBodySeparatorNode concept.

It also contains a few small unrelated changes from API review.

shyamnamboodiripad · 2023-09-08T20:40:54Z

    public string? GetDefaultKernelName()
    {
-        if (TryGetKernelInfoFromMetadata(Metadata, out var kernelInfo))
+        // FIX: (GetDefaultKernelName) remove from public API


Did you intend to leave this change in the current PR?

shyamnamboodiripad · 2023-09-08T20:54:26Z


                if (ParseRequestSeparator() is { } separatorNode)
                {
+                    // FIX: (Parse) uncovered


Is this something that will be fixed in a future PR?

It's uncovered but removing it causes tests to fail, which points to a test gap but not to the code being superfluous. We can look into these examples during a group session. (Test-first approaches usually avoid this kind of uncertainty.)

shyamnamboodiripad · 2023-09-08T20:55:36Z

        }

-        private IEnumerable<HttpVariableDeclarationAndAssignmentNode>? ParseVariableDeclarations()
+        private static void AppendCommentsIfAny(List<HttpCommentNode> commentsToAppend, HttpSyntaxNode prependToNode)


prependToNode -> appendToNode (Or perhaps just change it to node in both functions).

shyamnamboodiripad · 2023-09-08T20:58:45Z

+            commentsToAppend.Clear();
+        }
+
+        private static void PrependCommentsIfAny(List<HttpCommentNode> commentsToPrepend, HttpSyntaxNode prependToNode)


The method looks identical to the one above except for the addBefore argument. Consider creating a single AddCommentsIfAny() method with an addBefore parameter.

The method has been removed.

shyamnamboodiripad · 2023-09-08T21:14:51Z

            node.Add(ParseExpression());
-            node.Add(ParseExpressionEnd());
+            var expressionEnd = ParseExpressionEnd();
+            if (expressionEnd is not null)


Curious why is there a null check for the end of an expression but not for the expression itself?

I found a bug via testing that drove this specific change.

shyamnamboodiripad · 2023-09-08T21:17:16Z

-
            if (IsComment())
            {
+                // FIX: (ParseRequest) this looks suspect... test with a comment at the start of the request node?


Is this for a future PR?

Potentially, yes. There's no need to hold a PR to fix unrelated pre-existing issues.

shyamnamboodiripad · 2023-09-11T07:47:10Z

                ConsumeCurrentTokenInto(node);
            }

+            ParseTrailingWhitespace(node);


consider: return PassTRailingWhitespace(node);

I was considering removing the return values from these methods as it makes it ambiguous as to whether they return the original node.

shyamnamboodiripad · 2023-09-11T07:47:20Z

-            return ParseTrailingTrivia(node);
+            ParseTrailingWhitespace(node);
+
+            return node;


consider: return PassTRailingWhitespace(node);

shyamnamboodiripad · 2023-09-11T07:49:45Z

@@ -759,15 +772,14 @@ private bool IsComment()
                    return false;


minor: This line seems redundant since we already return false on line 775 below.

shyamnamboodiripad · 2023-09-11T07:55:28Z

+            if (MoreTokens() && !IsRequestSeparator())
            {
-                if (CurrentToken is { Kind: HttpTokenKind.Punctuation } and { Text: "#" })
+                if (CurrentToken is { Text: "#" })


This was also a question before the changes in the current PR, but it is unclear whether comments are allowed to start at the end of an existing line e.g.

myHeaderName: myHeaderValue // a comment
or
get https://www.host.com HTTP/1.1 # a comment

I think we disallow this at the moment. But it would be great to add (negative) tests for the above cases. We should probably also report errors like unexpected token "/" at position (*, *) in such cases.

shyamnamboodiripad · 2023-09-11T07:59:06Z

+            }
+
+            var commentsToPrepend = new List<HttpCommentNode>();
+            if (ParseComment() is { } comment)


Looks like we would also fail to parse the comment if there is any whitespace before the # token (since ParseComment expects the current token to be #).

This might depend on whether a previous call parsed the trailing whitespace.

For what it's worth, the old parser reports a diagnostic when there's whitespace on the line preceding the #.

shyamnamboodiripad

Approving to unblock - will complete review later and we can address anything blocking in a follow up PR.

jonsequitur requested review from bleaphar and shyamnamboodiripad September 8, 2023 20:30

shyamnamboodiripad reviewed Sep 8, 2023

View reviewed changes

jonsequitur force-pushed the combinatorial-testing-for-diagnostics-4 branch from 7ed0728 to ed81852 Compare September 8, 2023 20:43

shyamnamboodiripad reviewed Sep 11, 2023

View reviewed changes

jonsequitur added 4 commits September 11, 2023 14:50

add combinatorial tests for extra trivia and progressively typed code

addfb94

improve comment parsing; more combinatorial testing

f624feb

PR feedback

9e3f60e

fix for leading request separator; CurrentToken => null instead of throw

50f1cb6

jonsequitur force-pushed the combinatorial-testing-for-diagnostics-4 branch from ba9ae08 to 50f1cb6 Compare September 11, 2023 22:54

jonsequitur added 2 commits September 12, 2023 11:19

remove redundant GetDefaultKernelName logic

c7fa384

update API baseline

13a5b86

jonsequitur requested a review from shyamnamboodiripad September 12, 2023 21:41

shyamnamboodiripad approved these changes Sep 13, 2023

View reviewed changes

jonsequitur merged commit c13d287 into dotnet:main Sep 13, 2023

colombod added the Area-API label Sep 20, 2023

Uh oh!

Conversation

jonsequitur commented Sep 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shyamnamboodiripad left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonsequitur commented Sep 8, 2023 •

edited

Loading