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

πŸ‘©β€πŸ”¬ [Investment Day] - Test Suite Cleanup #731

Merged
merged 10 commits into from
Jul 3, 2019

Conversation

dusi
Copy link
Contributor

@dusi dusi commented Jun 28, 2019

πŸ“² What

  • Updates comment references to VoiceOver thorough the codebase
  • Initializes template model object with explicit type for faster type inference
  • Renames environment variable to only reflect 1Password support (as oppose to a whole OS version support)

πŸ€” Why

We need to clean up once in a while..

Also the adoption of our isOSVersionAvailable API wasn't as big as we assumed mostly due to some limiting factors. The #available API from Apple is way better in this regard. We haven't been able to adopt this widely mostly due to the following limitations..

We could not use

// We could not simply use the following condition
if ApiEnvironment.current.isOSVersionAvailable(12.0) {
  // Only do iOS 12 stuff
} else {
  // Only do iOS 11 stuff
  // The build would fail because the API couldn't be used directly unless it was under `#available` check which completely beats the purpose
}

βœ… Acceptance criteria

  • 1Password button shows up and allows login using the 1Password extension on iOS 11 with 1Password.app installed
  • Tests raise exception when trying to run tests on anything but iPhone 8 / iOS 12 combo

Copy link
Contributor

@justinswart justinswart left a comment

Choose a reason for hiding this comment

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

Cool cool! Just the one suggestion.

@@ -71,11 +69,16 @@ internal class TestCase: FBSnapshotTestCase {
}

internal func preferredSimulatorCheck() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep this as:

internal func preferredSimulatorCheck() {
  guard
    let identifier = ProcessInfo().environment["SIMULATOR_MODEL_IDENTIFIER"],
    ["iPhone10,1", "iPhone10,4"].contains(identifier),
    #available(iOS 12.0, *)
  else {
    fatalError("Please only test and record screenshots on an iPhone 8 simulator running iOS 12")
  }
}

and then can we add this to TestCase:

override var recordMode: Bool {
  willSet(newValue) {
    if newValue {
      preferredSimulatorCheck()
    }
  }
}

This will only perform the preferredSimulatorCheck() when we are actually recording screenshots and not just running unit tests.
You can then remove the line calling preferredSimulatorCheck() from setUp() in TestCase.swift πŸ‘

@dusi dusi requested a review from justinswart July 3, 2019 00:17
Copy link
Contributor

@justinswart justinswart left a comment

Choose a reason for hiding this comment

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

Don't forget to remove preferredSimulatorCheck() from line 35 of TestCase :trollface:

@dusi dusi requested a review from justinswart July 3, 2019 20:34
@dusi dusi merged commit 0d86260 into master Jul 3, 2019
@dusi dusi deleted the test-suite-cleanup branch July 3, 2019 21:31
@dusi
Copy link
Contributor Author

dusi commented Jul 3, 2019

@Scollaco @ifbarrera @cdolm92 please note that you can now test on any SIM you want (iPhone SE, iPhone X, etc. without running into fatalError as oppose to before where you could only run tests against iPhone 8). Obviously this does not apply to snapshot tests which have to be run against the same device form factor that they were recorded 😎

Hope this will help make your day to day experience better. Thanks @justinswart for the idea!

@dusi dusi mentioned this pull request Jul 8, 2019
13 tasks
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.

None yet

2 participants