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

upgrade-snapshot-tests-dependency-fix #5

Merged

Conversation

adriansergheev
Copy link
Collaborator

@adriansergheev adriansergheev commented Jan 9, 2024

#4

  • make it possible to pass perceptualPrecision / precision as parameters. This allows for custom setup against different CI env.

@@ -149,7 +149,7 @@ extension XCTestCase {
line: UInt
) {
UIView.setAnimationsEnabled(false)
let subpixelThreshold: UInt8 = 5
let perceptualPrecision: Float = 5
Copy link
Member

Choose a reason for hiding this comment

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

This value should be between 0 and 1. What we should do here in try to find the sweet-spot where CircleCI does not fail our tests in neither FinniversKit nor FinnUI.

The comments seems to point to 0.98 being a reasonable value, but please test this before merging this PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's correct!
i looked instead at these:
image

@adriansergheev adriansergheev requested a review from bstien January 9, 2024 13:18
@adriansergheev
Copy link
Collaborator Author

adriansergheev commented Jan 10, 2024

based on research from here: pointfreeco/swift-snapshot-testing#628
i tried three different configurations:

  • 0.98 perceptualPrecision - lots of failures on (FinniversKit on CircleCI)
  • 0.95 perceptualPrecision - some failures, and only on dark mode screenshots :man-shrugging: (FinniversKit on CircleCI)
  • 0.98 perceptualPrecision + 0.9 precision - works! (FinniversKit on CircleCI)

@adriansergheev adriansergheev force-pushed the upgrade-snapshot-tests-dependency branch 3 times, most recently from f09cb59 to 4519df8 Compare January 10, 2024 22:55
@adriansergheev adriansergheev force-pushed the upgrade-snapshot-tests-dependency branch from 4519df8 to b7dd1a0 Compare January 11, 2024 08:20
@adriansergheev adriansergheev force-pushed the upgrade-snapshot-tests-dependency branch from f51f726 to 19ab879 Compare January 12, 2024 17:05
@adriansergheev adriansergheev merged commit de28e39 into finn-no:main Jan 17, 2024
@adriansergheev adriansergheev deleted the upgrade-snapshot-tests-dependency branch January 17, 2024 07:54
adriansergheev pushed a commit that referenced this pull request Jan 25, 2024
adriansergheev added a commit that referenced this pull request Jan 25, 2024
* Revert "upgrade-snapshot-tests-dependency-fix (#5)"

This reverts commit de28e39.

* Revert "Merge pull request #4 from adriansergheev/upgrade-snapshot-tests-dependency"

This reverts commit 002558c, reversing
changes made to da62ebd.

---------

Co-authored-by: Adrian Sergheev <[email protected]>
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.

2 participants