Add large payload validation tests to Sisk.Core#29
Add large payload validation tests to Sisk.Core#29CypherPotato wants to merge 2 commits intomainfrom
Conversation
- Added `LargePayloadTests.cs` to `tests/Sisk.Core/Tests/` to validate 10MB, 100MB, 500MB, and 1.5GB payloads. - Added `RandomStream.cs` helper for deterministic large data generation. - Updated `tests/Sisk.Core/tests.csproj` to reference `Sisk.Cadente` and target `net9.0`. - Tests use `CadenteHttpServerEngine` to verify Cadente's handling of large payloads within the Sisk framework. Co-authored-by: CypherPotato <17441745+CypherPotato@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
Adds new framework-level MSTest cases in tests/Sisk.Core intended to validate large request-body streaming (up to ~1.5GB) against a Cadente-hosted server endpoint, verifying integrity via a computed hash.
Changes:
- Add a
RandomStreamhelper to generate deterministic large payload streams without pre-buffering. - Add
LargePayloadTeststhat POST large streams to a local Sisk server route and validate the response hash. - Add an additional Cadente project reference to the test project.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| tests/Sisk.Core/tests.csproj | Adds a project reference for Cadente (Sisk.Cadente). |
| tests/Sisk.Core/Tests/RandomStream.cs | Introduces a stream that generates deterministic random bytes for large-payload tests. |
| tests/Sisk.Core/Tests/LargePayloadTests.cs | Adds multiple large-payload upload tests (10MB–1.5GB) using an in-process server and hash verification. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| private async Task<string> CalculateHashAsync(Stream stream) | ||
| { | ||
| byte[] hashBytes = await SHA1.HashDataAsync(stream); |
There was a problem hiding this comment.
This uses SHA1.HashDataAsync(...). If .NET analyzers are enabled (common in SDK-style projects), SHA1 can trigger security analyzer warnings (e.g., CA5350) which become errors due to TreatWarningsAsErrors=true. Prefer SHA256 (or another non-deprecated hash) for test integrity checks.
| byte[] hashBytes = await SHA1.HashDataAsync(stream); | |
| byte[] hashBytes = await SHA256.HashDataAsync(stream); |
| [TestClass] | ||
| public class LargePayloadTests | ||
| { | ||
| private HttpServerHostContext? _server; | ||
| private int _port; | ||
|
|
There was a problem hiding this comment.
The test project runs with [assembly: Parallelize(Scope = MethodLevel, Workers = 4)]. These payload sizes (especially 500MB and 1.5GB) can easily cause CI timeouts/OOM when multiple tests execute concurrently. Mark this class/methods as [DoNotParallelize] and/or gate the largest cases behind an env var / explicit category so they don't run in the default suite.
| using Sisk.Core.Http.Hosting; | ||
| using Sisk.Core.Routing; | ||
|
|
||
| namespace tests; |
There was a problem hiding this comment.
This file uses namespace tests; while the other test classes in this project consistently use namespace tests.Tests; (e.g., Tests/HttpRequestTests.cs, Tests/EngineRareTests.cs). Align the namespace to keep discovery/organization consistent.
| namespace tests; | |
| namespace tests.Tests; |
|
|
||
| namespace tests; | ||
|
|
||
| public class RandomStream : Stream |
There was a problem hiding this comment.
This class is in namespace tests; but appears to be a helper used by tests. If it's meant to be internal-only test infrastructure, consider making it internal sealed to reduce surface area and align with the rest of the test assembly style.
| public class RandomStream : Stream | |
| internal sealed class RandomStream : Stream |
| using System.IO; | ||
| using System.Security.Cryptography; | ||
| using System.Text; | ||
| using System.Net.Http; |
There was a problem hiding this comment.
using System.Text; is unused in this file. With TreatWarningsAsErrors=true in the test project, this will fail the build (CS8019). Remove it or use it.
| using System.Net.Http; |
| using System.Net.Http; | ||
| using Microsoft.VisualStudio.TestTools.UnitTesting; | ||
| using Sisk.Cadente; | ||
| using Sisk.Cadente.CoreEngine; |
There was a problem hiding this comment.
using Sisk.Cadente; appears unused here (types are coming from Sisk.Cadente.CoreEngine). With warnings-as-errors, this will break the build; remove the directive or reference a type from that namespace if intended.
| using Sisk.Cadente.CoreEngine; |
| [TestMethod] | ||
| public async Task TestPayload_10MB() | ||
| { | ||
| await RunPayloadTest(10 * 1024 * 1024); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public async Task TestPayload_100MB() | ||
| { | ||
| await RunPayloadTest(100 * 1024 * 1024); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public async Task TestPayload_500MB() | ||
| { | ||
| await RunPayloadTest(500 * 1024 * 1024); | ||
| } |
There was a problem hiding this comment.
The 10MB/100MB/500MB tests have no MSTest [Timeout], so a hung upload/read can stall the whole run (and parallel workers). Consider adding timeouts (or a shared timeout policy) for each method, not just the 1.5GB case.
| using var listener = new System.Net.Sockets.TcpListener(System.Net.IPAddress.Loopback, 0); | ||
| listener.Start(); | ||
| return ((System.Net.IPEndPoint)listener.LocalEndpoint).Port; |
There was a problem hiding this comment.
This duplicates ListeningPort.GetRandomPort() (and the host builder already chooses a random port when none is specified). Consider reusing the existing helper or letting HttpServer.CreateBuilder().Build() pick the port, to avoid duplicated logic.
| using var listener = new System.Net.Sockets.TcpListener(System.Net.IPAddress.Loopback, 0); | |
| listener.Start(); | |
| return ((System.Net.IPEndPoint)listener.LocalEndpoint).Port; | |
| return ListeningPort.GetRandomPort(); |
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\..\cadente\Sisk.Cadente.CoreEngine\Sisk.Cadente.CoreEngine.csproj" /> | ||
| <ProjectReference Include="..\..\cadente\Sisk.Cadente\Sisk.Cadente.csproj" /> |
There was a problem hiding this comment.
This added project reference to Sisk.Cadente.csproj looks redundant: tests already references Sisk.Cadente.CoreEngine, which itself references Sisk.Cadente transitively. If nothing in the test project uses Sisk.Cadente directly, consider removing this reference to keep dependencies minimal.
| <ProjectReference Include="..\..\cadente\Sisk.Cadente\Sisk.Cadente.csproj" /> |
|
@copilot work on your suggestions. |
|
@CypherPotato I've opened a new pull request, #30, to work on those changes. Once the pull request is ready, I'll request review from you. |
I will work on the suggestions from the code review. |
- Implemented `LargePayloadTests.cs` in `tests/Sisk.Core/Tests/` validating payloads of 10MB, 100MB, 500MB, and 1.5GB using SHA-1 hashing. - Added `RandomStream.cs` for memory-efficient deterministic large payload generation. - Updated `Sisk.Cadente` and `Sisk.Cadente.CoreEngine` to multi-target `net8.0;net9.0`. - Updated `tests/Sisk.Core/tests.csproj` to target `net8.0` and reference `Sisk.Cadente`. - Tests verified to pass on `net8.0` runtime. Co-authored-by: CypherPotato <17441745+CypherPotato@users.noreply.github.com>
Implemented framework-level tests for validating large payloads (up to 1.5GB) using the Cadente engine. This ensures the server correctly processes and validates large data streams without memory issues. Tests are integrated into the main
Sisk.Coretest suite.PR created automatically by Jules for task 10150719905609723466 started by @CypherPotato