Fix tests on Windows

[N-Olbert]

Summary of the changes (Less than 80 chars)

• 16 tests currently fail on Windows
• 1 one non-Englisch systems (Input_Should_MatchSnapshotAndType_When_Passed, due to localized formatting)
• this is fixed with this PR

Closes #bugnumber (in this specific format)

Failing tests:

Details

HotChocolate AspNetCore test HotChocolate.AspNetCore.Tests HttpGetSchemaMiddlewareTests Download_GraphQL_Schema Download_GraphQL_Schema(path: "/graphql/schema") Download_GraphQL_Schema(path: "/graphql/schema.graphql") Download_GraphQL_Schema(path: "/graphql/schema/") Download_GraphQL_Schema(path: "/graphql?sdl") Download_GraphQL_Schema_Slicing_Args_Enabled Download_GraphQL_Schema_Slicing_Args_Enabled(path: "/graphql/schema") Download_GraphQL_Schema_Slicing_Args_Enabled(path: "/graphql/schema.graphql") Download_GraphQL_Schema_Slicing_Args_Enabled(path: "/graphql/schema/") Download_GraphQL_Schema_Slicing_Args_Enabled(path: "/graphql?sdl") HttpPostMiddlewareTests SingleRequest_Defer_Results Fusion-vnext test HotChocolate.Fusion.Execution.Tests Execution Serialization JsonOperationPlanSerializationTests Parse_Plan Parse_Plan_With_Node MongoDb test HotChocolate.Types.MongoDb.Tests BsonTypeTests Input_Should_MatchSnapshotAndType_When_Passed Input_Should_MatchSnapshotAndType_When_Passed(fieldName: "double", value: 43,229999999999997, type: typeof(MongoDB.Bson.BsonDouble)) Primitives test HotChocolate.Primitives.Tests NameUtilsTests InvalidName InvalidName(name: "$Bar") InvalidName(name: "1_Bar") InvalidName(name: "1Bar") InvalidName(name: "B+ar") InvalidName(name: "B/ar")

[N-Olbert]

ETag and ContentLength differ based on whether the response contains \n or \r\n as new line delimiter. I removed them as I think these values are out-of-scope for the actual test case.

[glen-84]

This is okay for now, though I am considering changing the approach.

Are you using LF or CRLF on Windows? I'm using LF (no autocrlf, etc.), but I'm wondering whether it might simplify things if I checked out CRLF and committed LF.

[N-Olbert]

@glen-84 I use LF with no autocrlf. Using CRLF with autocrlf opens a different can of worms (I tried it out) as that implicitly changes all multi-line raw string literals. This will change f. e. the operation hash of such operations, causing a few (around 10 or so) other tests to fail.

Anyhow, I do not see a single change here that shouldn't be required for CRLF-environments. If any, such environments will require additional adjustments.

[glen-84]

This will change f. e. the operation hash of such operations, causing a few (around 10 or so) other tests to fail.

Ya, this is a pain, but if we decided that operation hashes should vary depending on platform (not sure about this), then one option would be to scrub hashes from the snapshots.

Anyhow, I do not see a single change here that shouldn't be required for CRLF-environments. If any, such environments will require additional adjustments.

We wouldn't need the special handling in the CookieCrumble Snapshot class, or any of the ReplaceLineEndings calls.

[N-Olbert]

but if we decided that operation hashes should vary depending on platform (not sure about this)

Probably it would indeed be beneficial to have a linefeed-agnostic hash: If I recall correctly, the OperationHash is basically
the key of the OperationCacheMiddleware, so currently the same request once with \n and once with \r\n will take two cache slots. However, I think getting this would tricky and probably also too computationally intensive, since I think user data / value node literals may not be modified at all.
So (assume that all line feeds are not escaped, I just added them for better understanding):

foo(arg: "abc") { \n
    p1 { \n
        bar \n
    } \n
} \n

is indeed identical to:

foo(arg: "abc") { \r\n
    p1 { \r\n
        bar \r\n
    } \r\n
} \r\n

but

foo(arg: "a\nb\nc\n") { \n
    p1 { \n
        bar \n
    } \n
} \n

is not identical to:

foo(arg: "a\r\nb\r\nc\r\n") { \r \n
    p1 { \r\n
        bar \r\n
    } \r\n
} \r\n

Maybe its worth experimenting with it...

We wouldn't need the special handling in the CookieCrumble Snapshot class, or any of the ReplaceLineEndings calls.

A lot of snaps contain escaped new line delimiters, currently only \n (except for the http-multipart-snaps, those contain \r\n as this delimiter is defined by the spec), so I think the change in CookieCrumble would still be necessary.

[glen-84]

However, I think getting this would tricky and probably also too computationally intensive, since I think user data / value node literals may not be modified at all.

Why would they need to be modified? Wouldn't we just replace literal CRLFs with LFs in the byte array before hashing?

A lot of snaps contain escaped new line delimiters, currently only \n (except for the http-multipart-snaps, those contain \r\n as this delimiter is defined by the spec), so I think the change in CookieCrumble would still be necessary.

Some of these would be caused by NormalizeLineBreaks(), others because the resource (graphql file) in Git would currently be checked out with LF. I'm not sure how many other cases there are though.

Certainly not a trivial task. 🙂

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (8997e28) to head (c3eb1ac).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@     Coverage Diff      @@
##   main   #8747   +/-   ##
============================
============================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[glen-84]

@N-Olbert Thanks!

[N-Olbert]

Why would they need to be modified? Wouldn't we just replace literal CRLFs with LFs in the byte array before hashing?

I dont know...

If it were just a plain hash, replacing CRLFs with LFs in the byte stream would work, but since we’re using an md5/sha hash of the document / request as a unique identifier (without an explicit equality check) in a lot of cache layers, I think that approach is too simple.

F. e. we use the operation / document hash as cache key within the cache middlewares:

graphql-platform/src/HotChocolate/Core/src/Execution.Pipeline/DocumentCacheMiddleware.cs

Lines 65 to 66 in 906d9d3

	documentInfo.Hash = _hashProvider.ComputeHash(request.Document.AsSpan());
	if (_documentCache.TryGetDocument(documentInfo.Hash.Value, out document))

graphql-platform/src/HotChocolate/Fusion-vnext/src/Fusion.Execution/Execution/Pipeline/OperationPlanCacheMiddleware.cs

Lines 30 to 37 in 906d9d3

	var operationId = documentInfo.OperationCount == 1
	? documentInfo.Hash.Value
	: $"{documentInfo.Hash.Value}.{context.Request.OperationName ?? "Default"}";
	context.SetOperationId(operationId);
	var isPlanCached = false;
	if (_cache.TryGet(operationId, out var plan))

graphql-platform/src/HotChocolate/Core/src/Execution/Pipeline/OperationCacheMiddleware.cs

Lines 38 to 44 in 906d9d3

	if (!context.TryGetOperationId(out var operationId))
	{
	operationId = context.CreateCacheId();
	context.SetOperationId(operationId);
	}
	if (_operationCache.TryGetOperation(operationId, out var operation))

I think the core keypoints are:

• For structural newlines (those between tokens in the GraphQL document), normalizing \r\n → \n before hashing makes sense and won`t change anything during execution, so

foo { 
  bar 
}

could be treated as the same with either \n or \r\n or any permutation of those after line 1, 2, 3.

• But for string literals, the line endings are part of the actual request data, so they must remain unchanged, since foo(arg: "a\nb\nc\n") and foo(arg: "a\r\nb\r\nc\r\n") aren`t interchangable, as the resolver logic may depend on the exact line feed characters (e.g. "\r\n" repeated 3 times means X, while "\n" repeated 3 times means Y)

So while I see the appeal of the normalization, I don’t think it can be done reliably at the byte level without risking that distinct operations collapse into the same hash - but maybe I oversee something.

[glen-84]

There are no literal CRLFs/LFs in a regular GraphQL string, they are escaped. However, block strings would be an issue.

Maybe we need a specialized syntax printer that builds a string without whitespace between tokens, and only preserves whitespace inside strings.