Expand combinatorial testing of HTTP parser #3166
Conversation
| @@ -198,7 +198,8 @@ internal void NormalizeElementKernelNames(KernelInfoCollection kernelInfos) | |||
|
|
|||
| public string? GetDefaultKernelName() | |||
| { | |||
| if (TryGetKernelInfoFromMetadata(Metadata, out var kernelInfo)) | |||
| // FIX: (GetDefaultKernelName) remove from public API | |||
There was a problem hiding this comment.
Did you intend to leave this change in the current PR?
jonsequitur
force-pushed
the
combinatorial-testing-for-diagnostics-4
branch
from
7ed0728
to
ed81852
Compare
| @@ -74,17 +73,36 @@ public HttpSyntaxTree Parse() | |||
|
|
|||
| if (ParseRequestSeparator() is { } separatorNode) | |||
| { | |||
| // FIX: (Parse) uncovered | |||
There was a problem hiding this comment.
Is this something that will be fixed in a future PR?
There was a problem hiding this comment.
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.)
| } | ||
|
|
||
| return _syntaxTree; | ||
| } | ||
|
|
||
| private IEnumerable<HttpVariableDeclarationAndAssignmentNode>? ParseVariableDeclarations() | ||
| private static void AppendCommentsIfAny(List<HttpCommentNode> commentsToAppend, HttpSyntaxNode prependToNode) |
There was a problem hiding this comment.
prependToNode -> appendToNode (Or perhaps just change it to node in both functions).
| commentsToAppend.Clear(); | ||
| } | ||
|
|
||
| private static void PrependCommentsIfAny(List<HttpCommentNode> commentsToPrepend, HttpSyntaxNode prependToNode) |
There was a problem hiding this comment.
The method looks identical to the one above except for the addBefore argument. Consider creating a single AddCommentsIfAny() method with an addBefore parameter.
There was a problem hiding this comment.
The method has been removed.
|
|
||
| private HttpEmbeddedExpressionNode ParseEmbeddedExpression() | ||
| { | ||
| var node = new HttpEmbeddedExpressionNode(_sourceText, _syntaxTree); | ||
|
|
||
| node.Add(ParseExpressionStart()); | ||
| node.Add(ParseExpression()); | ||
| node.Add(ParseExpressionEnd()); | ||
| var expressionEnd = ParseExpressionEnd(); | ||
| if (expressionEnd is not null) |
There was a problem hiding this comment.
Curious why is there a null check for the end of an expression but not for the expression itself?
There was a problem hiding this comment.
I found a bug via testing that drove this specific change.
| if (IsComment()) | ||
| { | ||
| // FIX: (ParseRequest) this looks suspect... test with a comment at the start of the request node? |
There was a problem hiding this comment.
Is this for a future PR?
There was a problem hiding this comment.
Potentially, yes. There's no need to hold a PR to fix unrelated pre-existing issues.
|
|
||
| while (MoreTokens() && CurrentToken.Kind is not HttpTokenKind.NewLine) | ||
| { | ||
| ConsumeCurrentTokenInto(node); | ||
| } | ||
|
|
||
| ParseTrailingWhitespace(node); |
There was a problem hiding this comment.
consider: return PassTRailingWhitespace(node);
There was a problem hiding this comment.
I was considering removing the return values from these methods as it makes it ambiguous as to whether they return the original node.
| return ParseTrailingTrivia(node); | ||
| ParseTrailingWhitespace(node); | ||
|
|
||
| return node; |
There was a problem hiding this comment.
consider: return PassTRailingWhitespace(node);
| @@ -759,15 +772,14 @@ private bool IsComment() | |||
| return false; | |||
There was a problem hiding this comment.
minor: This line seems redundant since we already return false on line 775 below.
| { | ||
| if (CurrentToken is { Kind: HttpTokenKind.Punctuation } and { Text: "#" }) | ||
| if (CurrentToken is { Text: "#" }) |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| var commentsToPrepend = new List<HttpCommentNode>(); | ||
| if (ParseComment() is { } comment) |
There was a problem hiding this 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 #).
There was a problem hiding this comment.
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 #.
jonsequitur
force-pushed
the
combinatorial-testing-for-diagnostics-4
branch
from
ba9ae08
to
50f1cb6
Compare
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
HttpBodySeparatorNodeconcept.It also contains a few small unrelated changes from API review.