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

Migrate snapshot tests away from unmaintained testing frameworks #2791

Closed
1ec5 opened this issue Jan 29, 2021 · 9 comments
Closed

Migrate snapshot tests away from unmaintained testing frameworks #2791

1ec5 opened this issue Jan 29, 2021 · 9 comments
Assignees
Labels
build Issues related to builds and dependency management. op-ex Refactoring, Tech Debt or any other operational excellence work. UI Work related to visual components, Android Auto, Camera, 3D, voice, etc.
Milestone

Comments

@1ec5
Copy link
Contributor

1ec5 commented Jan 29, 2021

MapboxNavigationTests currently relies on a varied assortment of testing frameworks; most of them are redundantly used for snapshot tests:

Some of these testing frameworks are unmaintained and probably won’t survive the transition to SPM in #2629. We should migrate any tests using Cedar, FBSnapshotTestCase, or SnappyShrimp to an actively maintained fork or an alternative library. Ideally, we should end up with fewer testing dependencies, because each dependency adds a learning curve and an unnecessary increase in the time it takes to set up a development environment using Carthage or CocoaPods.

/cc @mapbox/navigation-ios

@1ec5 1ec5 added build Issues related to builds and dependency management. op-ex Refactoring, Tech Debt or any other operational excellence work. - tests labels Jan 29, 2021
@1ec5 1ec5 added this to the v2.0.0 milestone Jan 29, 2021
@zugaldia
Copy link
Member

zugaldia commented Feb 4, 2021

Are any of these libraries shared across Mapbox SDKs? Wondering if there's an opportunity here to consolidate testing strategies across products.

/cc: @knov

@1ec5
Copy link
Contributor Author

1ec5 commented Feb 5, 2021

Are any of these libraries shared across Mapbox SDKs?

  • All our Swift service libraries use OHHTTPStubs for mocking network-related tests.
  • One of our internal testing applications uses SnappyShrimp.
  • One of our internal testing applications uses Quick and Nimble.
  • MapboxMobileEvents uses Cedar.

@1ec5
Copy link
Contributor Author

1ec5 commented Feb 6, 2021

The map SDK uses XCUITest directly for UI testing, but there’s a limit to how much we can share approaches between the two SDKs, because testing the internals of a view is different than testing a whole UI.

@1ec5
Copy link
Contributor Author

1ec5 commented Feb 16, 2021

iOSSnapshotTestCase is a popular successor to FBSnapshotTestCase, but it isn’t maintained either and doesn’t support SPM: uber/ios-snapshot-test-case#91.

@1ec5
Copy link
Contributor Author

1ec5 commented Feb 16, 2021

SnapshotTesting looks like a promising replacement for all three unsupported frameworks.

@truburt truburt modified the milestones: v2.0.0, v2.0.0 (GA) Feb 26, 2021
@truburt truburt added the UI Work related to visual components, Android Auto, Camera, 3D, voice, etc. label Mar 2, 2021
@truburt truburt modified the milestones: v2.0.0 (GA) (iOS), v2.0.0 (GA) Mar 5, 2021
@1ec5 1ec5 modified the milestones: v2.0.0 (GA), v2.0.0 (RC) Mar 5, 2021
@alanzeino
Copy link

I'm actively working on reviving the iOSSnapshotTestCase repo, in the meantime I'll be working on SPM support on my branch until I get ownership of the original repo again, feel free to use that:

https://github.com/alanzeino/ios-snapshot-test-case

I've merged in SPM support today. Since SPM uses git URIs you can use that for now and change the URI back to the original repo once it's back under development again (note, the URI for the original repo will likely change again to a separate organization at this point).

@S2Ler S2Ler self-assigned this May 27, 2021
@S2Ler
Copy link
Contributor

S2Ler commented May 27, 2021

I'm struggling with configuring alanzeino/ios-snapshot-test-case with SPM build system. The problem is that I don't see a way to specify reference images path. According to docs, there are currently three ways:

 1. Set the preprocessor macro FB_REFERENCE_IMAGE_DIR to a double-quoted c-string with the path. This only works for Objective-C tests.
 2. Set an environment variable named FB_REFERENCE_IMAGE_DIR with the path. It takes precedence over the preprocessor macro to allow for a run-time override.
 3. Keep everything unset, which will cause the reference images to be looked up
    inside the bundle holding the current test; in the
    Resources/ReferenceImages_* directories.

We immediately dismiss the first option because we use Swift.
The third option doesn't work by default with the SPM resource system.
It leaves us with the second option, which doesn't seem to work either. Xcode doesn't expand environment variables for some reason.
Another option could be forking this library and landing the fixes we need, but it is a worse long-term solution due to the usage of Objective C.

A fork with SPM support isn't active as well and will need more improvements to make it suitable for us.

  • I suggest migrating to SnapshotTesting library for the following reasons:
  1. Previous FBSnapshotTestCase fork is dead.
  2. A fork with SPM support isn't active as well and will need more improvements to make it suitable for us.
  3. It is written in Swift and will be easier to fork if needed.
  4. Although it has one limitation that it doesn't support snapshotting from many simulators, it works out of the box. There is an issue to add support for this, but it is stale: [Feature] Store snapshots from multiple simulators side-by-side pointfreeco/swift-snapshot-testing#182.
  5. It is a pretty robust library with support for different snapshot/diff strategies.

I successfully migrated ManeuverViewTests.swift with minimal code changes.

Also, I suggest using git lfs to store snapshot images for the following reason: Committing images to the git repository dramatically increases the repository size. As time goes, more and more snapshot images will be added to the repository, increasing our repository size and consuming unnecessary bandwidth during repository checkout and disk space on user/developer machines.

  • I have to investigate if it will break our users or not before committing to this approach. So this approach is in investigation stage. If you have any thoughts about that, let's discuss.
    An alternative approach would be storing these snapshot images somewhere else, like in the S3 bucket, but it is a more complicated approach.

cc @mapbox/navigation-ios @1ec5

S2Ler added a commit that referenced this issue May 28, 2021
The motivation for migrating to new snapshot testing library is described here: #2791 (comment).

Main consequence is that we test only on one device.

The env for current snapshots are: iPhone 12 Pro Max, iOS 14.5.

Xcode based project isn't updated yet because the work depends on #3031.
@S2Ler
Copy link
Contributor

S2Ler commented May 31, 2021

Follow up on:

I have to investigate if it will break our users or not before committing to this approach. So this approach is in investigation stage. If you have any thoughts about that, let's discuss.

Unfortunately without a workaround, using git-lfs breaks SPM support for the clients with the following error:

git-lfs filter-process: git-lfs: command not found

This is significant usability downgrade to my opinion.

S2Ler added a commit that referenced this issue Jun 4, 2021
The motivation for migrating to new snapshot testing library is described here: #2791 (comment).

Main consequence is that we test only on one device.

The env for current snapshots are: iPhone 12 Pro Max, iOS 14.5.

Xcode based project isn't updated yet because the work depends on #3031.
S2Ler added a commit that referenced this issue Jun 11, 2021
The motivation for migrating to new snapshot testing library is described here: #2791 (comment).

The env for current snapshots are:
- iPhone 12 Pro Max, iOS 14.4
- iPhone 11 Pro Max, iOS 13.7

Xcode based project isn't updated yet because the work depends on #3031.

Fix regression in test due to global dependency on applied Theme in tests.

Some tests change the global theme. To return it to the expected state,
we apply the needed theme in our snapshot tests.

Temporary fix to InstructionLabel clear cache functionality

Check #3043 (comment)
for more info.
@1ec5
Copy link
Contributor Author

1ec5 commented Jun 15, 2021

Implemented in #3043 in the release-v2.0 branch.

@1ec5 1ec5 closed this as completed Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues related to builds and dependency management. op-ex Refactoring, Tech Debt or any other operational excellence work. UI Work related to visual components, Android Auto, Camera, 3D, voice, etc.
Projects
None yet
Development

No branches or pull requests

6 participants