Skip to content
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

[release/6.0] Don't create multiple large files at the same time #63032

Merged
merged 6 commits into from
Dec 31, 2021

Conversation

jozkee
Copy link
Member

@jozkee jozkee commented Dec 21, 2021

Backport of #60606 and #62519.
Move tests that create large files (~4 gb) to OuterLoop and disables parallelization to avoid running into out of memory and time out errors.

Customer Impact

None, this is required to have a clean CI in 6.0 branch.

Testing

Changes have been tested in main for a while.

Risk

Low.

jozkee and others added 2 commits December 20, 2021 16:48
* move existing large file tests into a separate type (no code changes)

* don't run the large file tests in parallel

* use FileOptions.DeleteOnClose to ensure that each test removes it's own file

use single large file to test File.ReadAllBytes and ile.ReadAllBytesAsync for both limits
@jozkee jozkee added Servicing-consider Issue for next servicing release review test-enhancement Improvements of test source code labels Dec 21, 2021
@jozkee jozkee added this to the 6.0.x milestone Dec 21, 2021
@jozkee jozkee self-assigned this Dec 21, 2021
@ghost
Copy link

ghost commented Dec 21, 2021

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #60606 and #62519.
Move tests that create large files (~4 gb) to OuterLoop and disables parallelization to avoid running into out of memory and time out errors.

Customer Impact

None, this is required to have a clean CI in 6.0 branch.

Testing

Changes have been tested in main for a while.

Risk

Low.

Author: Jozkee
Assignees: Jozkee
Labels:

Servicing-consider, area-System.IO, test enhancement

Milestone: 6.0.x

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for backporting the fixes @jozkee !

@adamsitnik
Copy link
Member

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

}

[CollectionDefinition("NoParallelTests", DisableParallelization = true)]
public partial class NoParallelTests { }
Copy link
Member

@akoeplinger akoeplinger Dec 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't actually need this since we already have the same one here which you can include instead: https://github.com/dotnet/runtime/blob/release/6.0/src/libraries/Common/tests/System/Xml/DisableParallelization.cs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah looks like it is already that way in main, you'll need to include the file in the .csproj so it works:

<Compile Include="$(CommonTestPath)TestUtilities\System\DisableParallelization.cs" Link="Common\TestUtilities\System\DisableParallelization.cs" />

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File src\libraries\Common\tests\TestUtilities\System\DisableParallelization.cs doesn't exists in release/6.0.
I added the class here because that's how we were doing it prior to #62132.

Copy link
Member

@akoeplinger akoeplinger Dec 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it was in a different path in release/6.0 (see the linked file in my first comment), but this is just a cosmetic issue so fine if we keep what you have :)

{
using FileStream fs = new (GetTestFilePath(), FileMode.Create, FileAccess.Write, FileShare.Read, 4096, FileOptions.DeleteOnClose);

foreach (long lengthOverLimit in new long[] { Array.MaxLength + 1L, int.MaxValue + 1L })
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will need to get rid of this case because it's an edge case that isn't fixed in 6.0; see: #61519.

Suggested change
foreach (long lengthOverLimit in new long[] { Array.MaxLength + 1L, int.MaxValue + 1L })
foreach (long lengthOverLimit in new long[] { int.MaxValue + 1L })

jozkee and others added 2 commits December 21, 2021 10:43
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
@jozkee
Copy link
Member Author

jozkee commented Dec 22, 2021

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@danmoseley danmoseley merged commit 3b71fd8 into dotnet:release/6.0 Dec 31, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jan 30, 2022
@jozkee jozkee deleted the 6.0/NoInt32Overflow_OuterLoop branch April 16, 2024 15:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO Servicing-consider Issue for next servicing release review test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants