Do tests really use images from 'ReferenceOutput' folder? #2158

Activity

1. br3aker

   added

   needs triage

   on Jun 19, 2022
2. antonfirsov commented on Jun 20, 2022

   

   antonfirsov

   on Jun 20, 2022

   Member

   More actions

   Nice catch! Looks like a bug sneaked into our PngEncoderTests code and we are comparing the image to itself instead of the reference file :)

   ImageSharp/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs

   Lines 624 to 628 in 75dc96a

   	using (var imageSharpImage = Image.Load<TPixel>(actualOutputFile, new PngDecoder()))
   	using (var referenceImage = Image.Load<TPixel>(actualOutputFile, referenceDecoder))
   	{
   	ImageComparer.Exact.VerifySimilarity(referenceImage, imageSharpImage);
   	}

   Other encoder tests seem to be correct.

   👍React with 👍1Reacted by James Jackson-South
3. antonfirsov commented on Jun 20, 2022

   

   antonfirsov

   on Jun 20, 2022

   Last edited by antonfirsov

   Member

   More actions

   I was writing tests for jpeg encoder PR and looked at the tests which compare saved images to reference images

   Note that the mentioned PNG tests are validating generated images. Tests which are working with file inputs are using a utility method VerifyEncoder:

   ImageSharp/tests/ImageSharp.Tests/Formats/Bmp/BmpEncoderTests.cs

   Lines 364 to 365 in 2aa150c

   	// Does DebugSave & load reference CompareToReferenceInput():
   	image.VerifyEncoder(provider, "bmp", bitsPerPixel, encoder, customComparer);

   It compares the original image to the one re-decoded from the ImageSharp encoder output by a reference decoder, therefore it doesn't need reference images. Questionable approach for Jpeg, but still used in the current tests. Maybe we should create a set of tests with smaller images and create optimized reference PNG-s instead? This would raise the question if we should use ImageSharp or some other software for generating the reference output. Scratch that doesn't make sense, I don't have a better idea for result verification than the re-decoding trick.
4. antonfirsov

   added

   test-bug

   and removed

   needs triage

   on Jun 20, 2022
5. antonfirsov

   added this to the 3.0.0 milestone on Jun 20, 2022
6. br3aker commented on Jun 21, 2022

   

   br3aker

   on Jun 21, 2022

   ContributorAuthor

   More actions

   Questionable approach for Jpeg, but still used in the current tests.

   I don't really have an idea how to compare new output color types in jpeg encoder. Funny thing, current tests found a bug in my PR :D
7. JimBobSquarePants commented on Jun 22, 2022

   

   JimBobSquarePants

   on Jun 22, 2022

   Member

   More actions

   @antonfirsov We actually do the same in the Bmp Encoder tests for quantized images.
8. brianpopow commented on Aug 30, 2022

   

   brianpopow

   on Aug 30, 2022

   Collaborator

   More actions

   What would be a solution to fix this issue?

   • Use VerifyEncoder in TestPngEncoderCore and delete the PngEncoder reference images or
   • Assume the reference images are correct and use CompareToReferenceOutput() instead?
9. JimBobSquarePants commented on Aug 30, 2022

   

   JimBobSquarePants

   on Aug 30, 2022

   Member

   More actions

   I'll have to re-read our tests. As a side I really want to put some time to focus on cleaning up our test suite. It's difficult to navigate.
10. JimBobSquarePants

    modified the milestones: 3.0.0, Future on Feb 27, 2023