Skip to content

Conversation

@antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Jan 22, 2020

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 matches 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)

The problem

To enable experiments on memory stuff like #898, it is necessery to put the testing of memory-intensive code on right track. These tests are important but at the same time threatening the execution with sudden OOM-s, which made us to disable many of them.

Solution

It's possible to sandbox individual test code execution to separate processes with the help of RemoteExecutor which is a tool in the arcade project.

Limitation: The tool can only execute static methods, which can only consume string arguments.

Details

Bonus: simplify and repurpose Sandbox46

It's no longer needed to profile tests from Visual Studio, however it can be handy as a separate "profilable" .exe to run individual test blocks in a hard-coded way. This is useful eg. for memory profiling unit tests, since even JetBrains can't do that from IDE UI.

  • Renamed to ImageSharp.Tests.ProfilingSandbox
  • Referemce ImageSharp.Tests.csproj instead of including classes

@codecov
Copy link

codecov bot commented Jan 22, 2020

Codecov Report

Merging #1089 into master will decrease coverage by 29.32%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1089       +/-   ##
===========================================
- Coverage   83.77%   54.45%   -29.33%     
===========================================
  Files         704      704               
  Lines       29311    29311               
  Branches        0     3290     +3290     
===========================================
- Hits        24555    15961     -8594     
- Misses       4756    12823     +8067     
- Partials        0      527      +527
Flag Coverage Δ
#unittests 54.45% <ø> (-29.33%) ⬇️
Impacted Files Coverage Δ
...Implementation/Converters/CieXyzAndLmsConverter.cs 0% <0%> (-100%) ⬇️
src/ImageSharp/ColorSpaces/CieXyy.cs 0% <0%> (-100%) ⬇️
src/ImageSharp/Formats/Png/Filters/UpFilter.cs 0% <0%> (-100%) ⬇️
src/ImageSharp/ColorSpaces/Hsv.cs 0% <0%> (-100%) ⬇️
...arp/Memory/Allocators/Internals/BasicByteBuffer.cs 0% <0%> (-100%) ⬇️
src/ImageSharp/Formats/Jpeg/JpegThrowHelper.cs 0% <0%> (-100%) ⬇️
...c/ImageSharp/ColorSpaces/Companding/LCompanding.cs 0% <0%> (-100%) ⬇️
src/ImageSharp/ColorSpaces/LinearRgb.cs 0% <0%> (-100%) ⬇️
src/ImageSharp/Formats/Png/Filters/SubFilter.cs 0% <0%> (-100%) ⬇️
...tion/Converters/LinearRgbAndCieXyzConverterBase.cs 0% <0%> (-100%) ⬇️
... and 327 more

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 4973f5f...4519c55. Read the comment docs.

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

This is awesome! I didn't know any of this was possible.

Just some minor nitpicks regarding package referencing.

@JimBobSquarePants
Copy link
Member

I've no idea why the code coverage has dropped so dramatically but I'm going to push a quick change that updates the coverage setting to match Drawing and Web.

@JimBobSquarePants
Copy link
Member

@antonfirsov I'm stumped.
We've lost code coverage for all our colorspace conversion code.

https://codecov.io/gh/SixLabors/ImageSharp/tree/6f832aa77343e8d112d6ddd08765aa2e4d05519c

https://codecov.io/codecov/v4/raw/2020-01-22/67C5B7164C857642EEB62513118C846D/6f832aa77343e8d112d6ddd08765aa2e4d05519c/efa842f0-b93f-48eb-bd87-fecb40223fd2.txt

I tried looking for a spec to the lcov format to see if I could figure out what the various acronyms meant but no luck so far.

[InlineData(0, 1, .5F, 1, 0, 0)]
[InlineData(120, 1, .5F, 0, 1, 0)]
[InlineData(240, 1, .5F, 0, 0, 1)]
//[Theory]
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this WIP stuff now here?

Copy link
Member

Choose a reason for hiding this comment

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

It was just added to generate all the required info for coverlet-coverage/coverlet#700

I'll remove it

@antonfirsov
Copy link
Member Author

@JimBobSquarePants thanks for fixing the nuget stuff!

Personally I can live with those codecov issues. The tool's behavior is too random for me to trust it, I always ignore all it's reports.

@antonfirsov antonfirsov mentioned this pull request Jan 23, 2020
4 tasks
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