-
Notifications
You must be signed in to change notification settings - Fork 4
Add large payload validation tests to Sisk.Core #29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,120 @@ | ||||||||||
| using System; | ||||||||||
| using System.IO; | ||||||||||
| using System.Security.Cryptography; | ||||||||||
| using System.Text; | ||||||||||
| using System.Net.Http; | ||||||||||
| using Microsoft.VisualStudio.TestTools.UnitTesting; | ||||||||||
| using Sisk.Cadente; | ||||||||||
| using Sisk.Cadente.CoreEngine; | ||||||||||
|
||||||||||
| using Sisk.Cadente.CoreEngine; |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(); |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,49 @@ | ||||||
| using System; | ||||||
| using System.IO; | ||||||
|
|
||||||
| namespace tests; | ||||||
|
|
||||||
| public class RandomStream : Stream | ||||||
|
||||||
| public class RandomStream : Stream | |
| internal sealed class RandomStream : Stream |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -1,7 +1,7 @@ | ||||
| <Project Sdk="Microsoft.NET.Sdk"> | ||||
|
|
||||
| <PropertyGroup> | ||||
| <TargetFramework>net9.0</TargetFramework> | ||||
| <TargetFramework>net8.0</TargetFramework> | ||||
| <LangVersion>latest</LangVersion> | ||||
| <ImplicitUsings>enable</ImplicitUsings> | ||||
| <Nullable>enable</Nullable> | ||||
|
|
@@ -15,6 +15,7 @@ | |||
|
|
||||
| <ItemGroup> | ||||
| <ProjectReference Include="..\..\cadente\Sisk.Cadente.CoreEngine\Sisk.Cadente.CoreEngine.csproj" /> | ||||
| <ProjectReference Include="..\..\cadente\Sisk.Cadente\Sisk.Cadente.csproj" /> | ||||
|
||||
| <ProjectReference Include="..\..\cadente\Sisk.Cadente\Sisk.Cadente.csproj" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using System.Text;is unused in this file. WithTreatWarningsAsErrors=truein the test project, this will fail the build (CS8019). Remove it or use it.