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

Remove RunSerial from tests #1885

Merged
merged 3 commits into from
Dec 12, 2021
Merged

Remove RunSerial from tests #1885

merged 3 commits into from
Dec 12, 2021

Conversation

brianpopow
Copy link
Collaborator

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

This PR removes [Collection("RunSerial")] from the decoding/encoding tests, which was meant as a workaround for the sporadic OOM failures. Hopefully with #1730 this should no longer be necessary.

@brianpopow brianpopow changed the title WIP: Remove RunSerial from tests Remove RunSerial from tests Dec 9, 2021
@codecov
Copy link

codecov bot commented Dec 9, 2021

Codecov Report

Merging #1885 (edf7014) into master (3730a02) will decrease coverage by 0%.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1885   +/-   ##
======================================
- Coverage      87%     87%   -1%     
======================================
  Files         959     959           
  Lines       50392   50392           
  Branches     6258    6258           
======================================
- Hits        44113   44109    -4     
- Misses       5246    5248    +2     
- Partials     1033    1035    +2     
Flag Coverage Δ
unittests 87% <ø> (-1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...emory/DiscontiguousBuffers/MemoryGroup{T}.Owned.cs 86% <0%> (-5%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca8e984...edf7014. Read the comment docs.

@brianpopow
Copy link
Collaborator Author

No OOM so far, i will re-run the tests a few more times, just to make sure.

@brianpopow
Copy link
Collaborator Author

I have not seen any OOM so far, but similar to #1886, i have seen 3 times out of 6 runs the same failing test om macos:

AllocateMemoryGroup_Finalization_ReturnsToPool(length: 1200) [FAIL]
[xUnit.net 00:02:15.34]     AllocateMemoryGroup_Finalization_ReturnsToPool(length: 600) [FAIL]
  Failed AllocateMemoryGroup_Finalization_ReturnsToPool(length: 1200) [286 ms]
  Error Message:
   Microsoft.DotNet.RemoteExecutor.RemoteExecutionException : Remote process failed with an unhandled exception.
  Stack Trace:
  
Child exception:
  Xunit.Sdk.EqualException: Assert.Equal() Failure
Expected: 42
Actual:   0
   at SixLabors.ImageSharp.Tests.Memory.Allocators.UniformUnmanagedPoolMemoryAllocatorTests.AllocateGroupAndForget(UniformUnmanagedMemoryPoolMemoryAllocator allocator, Int32 length, Boolean check) in /Users/runner/work/ImageSharp/ImageSharp/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs:line 299
   at SixLabors.ImageSharp.Tests.Memory.Allocators.UniformUnmanagedPoolMemoryAllocatorTests.<AllocateMemoryGroup_Finalization_ReturnsToPool>g__RunTest|12_0(String lengthStr) in /Users/runner/work/ImageSharp/ImageSharp/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs:line 283

Child process:
  SixLabors.ImageSharp.Tests, Version=1.0.0.0, Culture=neutral, PublicKeyToken=d998eea7b14cab13 SixLabors.ImageSharp.Tests.Memory.Allocators.UniformUnmanagedPoolMemoryAllocatorTests Void <AllocateMemoryGroup_Finalization_ReturnsToPool>g__RunTest|12_0(System.String)

Child arguments:
  1200

  Failed AllocateMemoryGroup_Finalization_ReturnsToPool(length: 600) [538 ms]
  Error Message:
   Microsoft.DotNet.RemoteExecutor.RemoteExecutionException : Remote process failed with an unhandled exception.
  Stack Trace:
  
Child exception:
  Xunit.Sdk.EqualException: Assert.Equal() Failure
Expected: 42
Actual:   0
   at SixLabors.ImageSharp.Tests.Memory.Allocators.UniformUnmanagedPoolMemoryAllocatorTests.<AllocateMemoryGroup_Finalization_ReturnsToPool>g__RunTest|12_0(String lengthStr) in /Users/runner/work/ImageSharp/ImageSharp/tests/ImageSharp.Tests/Memory/Allocators/UniformUnmanagedPoolMemoryAllocatorTests.cs:line 290

@antonfirsov
Copy link
Member

Macos only? If yes let's just disable them with a skip condition on macos, or an [ActiveIssue(...)] aftrr opening an issue.

@JimBobSquarePants
Copy link
Member

I wonder what the specs of the macOS image are? Could it be hitting the high memory cleanup issue my machine was. I’ll try running that test locally later.

@brianpopow
Copy link
Collaborator Author

brianpopow commented Dec 11, 2021

I have added [ActiveIssue("https://github.com/SixLabors/ImageSharp/issues/1887", TestPlatforms.OSX)] to the failing test.

As far as i understand this attribute, this will skip the test only on OSX (correct me, if i am wrong)

@JimBobSquarePants
Copy link
Member

@brianpopow Yep, that's correct.
https://github.com/dotnet/runtime/blob/main/docs/workflow/testing/libraries/filtering-tests.md#activeissueattribute

@JimBobSquarePants JimBobSquarePants merged commit 93e4faa into master Dec 12, 2021
@JimBobSquarePants JimBobSquarePants deleted the bp/removeRunSerial branch December 12, 2021 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants