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

Rewrite images comparison tests using images input #10

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

IvanNextToJunior
Copy link

This time from another branch

@dxCLA
Copy link

dxCLA commented Oct 19, 2022

CLA assistant check
All committers have signed the CLA.

@schmidt9
Copy link
Contributor

@rzakhar please check first test that uses images input (cd7d960), it works, but fails on pixels comparison inside assertComparison method (actualDifference.image == expectedDifference.image).

Meanwhile Difference values pass test with given accuracy 1e-6
expected 0.19183700000000001 vs actual 0.19183762087210363

Should it mean that we also should apply similar accuracy while comparing pixels arrays too?

@rzakhar
Copy link
Contributor

rzakhar commented Oct 24, 2022

1e-6 accuracy is equivalent to 3 bad pixels on the 1179 x 2556 screenshot. It looks like quite a reasonable threshold. By ISO 13406-2 standard, it is close to Pixel Fault Class II.

I would keep the accuracy as is

@schmidt9
Copy link
Contributor

@rzakhar I mean pixels arrays are being compared always strict so test will always fail with any non-zero difference

@schmidt9
Copy link
Contributor

XCTAssertTrue(actualDifference.image == expectedDifference.image, file: file, line: line) does not takes into account any accuracy

@rzakhar
Copy link
Contributor

rzakhar commented Oct 24, 2022

You can use method.compareImages twice

The first time to compare reference images and assert the difference value (delta +- 1e-6). Use the passed method for it.
The second time to compare different images and assert the difference value (0 +- 1e-6). Use the strict method for it.

@schmidt9
Copy link
Contributor

Sorry I don't get you idea. In my understanding if I reuse images produced by SUITCase itself the strict test should pass out of the box.

I don't get why SUITCase produces inside compareImages method from the same Reference and Unexpected images a Difference image which is not equal to the same image loaded from assets. The only point I could imagine I save Difference image from test results log using Preview app, so this app could export Difference image with some specific png settings.

@rzakhar
Copy link
Contributor

rzakhar commented Oct 24, 2022

What is the difference between difference images? Would you please provide examples?

@schmidt9
Copy link
Contributor

While testing difference images I noticed a strange thing - Difference image from assets turns green when converted to RGBAImage

let differenceImage = UIImage(named: "en_iPhone_X_difference_strict", in: .module, compatibleWith: nil)!
        
try! differenceImage.writePNG(filePath: "/Users/user/Desktop/assets_diff.png")
try! RGBAImage(uiImage: differenceImage).uiImage.writePNG(filePath: "/Users/user/Desktop/rgba_diff.png")

This gives me

Снимок экрана 2022-10-24 в 16 32 29

@schmidt9
Copy link
Contributor

What is the difference between difference images? Would you please provide examples?

Running in assertComparison with strict method:

            let actualDifference = try method.compareImages(actual: image1.uiImage,
                                                            reference: image2.uiImage)
            
            let differenceDifference = try method.compareImages(actual: actualDifference.image.uiImage,
                                                                reference: expectedDifference.image.uiImage)

differenceDifference.value is ≈ 0.129131, so difference images seem to be quite different, is it related to turning to green?

Copy link
Contributor

@rzakhar rzakhar left a comment

Choose a reason for hiding this comment

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

Hey! Thanks for the PR. Remember to include a proper explanation to the PR. Things that can help:

  1. Clear title that shortly explains the changes
  2. Proper PR description with a detailed explanation
  3. Test data or setup required for a reviewer to verify changes.
  4. An .xcresult file of the successfully passed tests

.swiftpm/xcode/xcshareddata/xcschemes/SUITCase.xcscheme Outdated Show resolved Hide resolved
Tests/SUITCaseTests/CompareImagesTests.swift Show resolved Hide resolved
@@ -76,37 +76,29 @@ class CompareImagesTests: XCTestCase {
}

func testStrictComparison() {
let referenceImage = UIImage(named: "en_iPhone_X_reference_strict", in: .module, compatibleWith: nil)!
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we reuse these images in all tests. Can you please

  1. Reduce code repetition by moving these variables from the test cases to the test classes?
  2. Double-check that assets only include the images we need.

@schmidt9
Copy link
Contributor

schmidt9 commented Oct 26, 2022

Hey! Thanks for the PR. Remember to include a proper explanation to the PR. Things that can help:

  1. Clear title that shortly explains the changes
  2. Proper PR description with a detailed explanation
  3. Test data or setup required for a reviewer to verify changes.
  4. An .xcresult file of the successfully passed tests

@rzakhar
1 and 2 will be fixed
3 see please above my comments regarding difference images
4 Tests do not pass, that's the problem I do not know why, that's not clear about pixel arrays comparison and I still don't know why difference images are different, maybe you have a clue?

@IvanNextToJunior IvanNextToJunior changed the title add assets Rewrite images comparison tests using images input Oct 26, 2022
@IvanNextToJunior
Copy link
Author

Rewrite images comparison tests using real images input from assets

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants