Remove larger test length for Iri_ExpandingContents_TestData#131070
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @karelz, @dotnet/ncl |
There was a problem hiding this comment.
Pull request overview
This PR reduces the maximum input size used by the Iri_ExpandingContents_Accepted test data to avoid excessive allocations during URI property evaluation (notably ToString()), while preserving coverage for “longer than ushort” scenarios.
Changes:
- Removed the
1_000_000length case fromIri_ExpandingContents_TestData. - Retained
66_000as the “large” case to continue exercising length-expansion behavior beyondushort.MaxValue.
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "23f760f067a6ef93bfb750718aeb56496bc3d9c5",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "d349aa7988e9adf0ee0f5f0473886c0582975955",
"last_reviewed_commit": "23f760f067a6ef93bfb750718aeb56496bc3d9c5",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "d349aa7988e9adf0ee0f5f0473886c0582975955",
"last_recorded_worker_run_id": "29737569462",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "23f760f067a6ef93bfb750718aeb56496bc3d9c5",
"review_id": 4734449689
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: The Iri_ExpandingContents_TestData case with length = 1_000_000 produces a ~9M-character string for ToString plus additional properties and buffers, causing memory pressure (~100 MB) that can lead to OOM/instability (issue #130786). Removing the largest length reduces this cost while keeping meaningful coverage.
Approach: A single-line change drops 1_000_000 from the { 1, 64_000, 66_000, 1_000_000 } length array feeding the theory data. The remaining 66_000 case still exercises the important "longer than ushort" (> 65,535) code path that motivated these cases when the Uri length restriction was removed, so coverage of the boundary behavior is preserved.
Summary: This is a low-risk, well-targeted test-only change. It removes an expensive test input that risks memory exhaustion without sacrificing coverage of the > ushort length scenarios (still covered by the 64k/66k cases). No production code is affected and no correctness concerns arise. LGTM.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 27.2 AIC · ⌖ 15.5 AIC · ⊞ 10K
Closes #130786
The 1 million length test will end up allocating a ~9 million character string for
ToString.Together with all the other properties we're allocating and various buffers, this could reach some ~100 MB.
The test case was added when I removed the length restriction on Uris. The 66k case should already give us coverage for the important "longer than ushort" scenarios.