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

Feature/alert controller assertion #20

Merged
merged 8 commits into from Jul 19, 2017

Conversation

lgauthier
Copy link
Collaborator

@nhgrif This adds an assertion for testing that a view controller presented a UIAlertController with a particular style, title, message, and actions. Just so you're aware, in order to test this assertion, I had to be able to successfully present an alert which isn't possible without a window. So, I added a new test target that actually runs an empty UIKit app so I have a window to add my presenter view controller to for the tests.

@lgauthier lgauthier requested a review from nhgrif July 13, 2017 00:16
Copy link
Contributor

@nhgrif nhgrif left a comment

Choose a reason for hiding this comment

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

Going to wait on reviewing this further until you've investigated the host application / UIWindow stuff we just discussed.

@lgauthier
Copy link
Collaborator Author

lgauthier commented Jul 13, 2017

@nhgrif I've done some testing with attempting to successfully present a view controller without using a host application target. I've tried a random assortment of things, but I haven't had any luck so far.

If we simply create a UIViewController instance and call present(_:animated:completion:) on it, the presentation fails and the following warning is printed to the console:

Warning: Attempt to present <SomeViewController: 0x7fb49ef25d20> on <SomeOtherViewController: 0x7fb49ee369d0> whose view is not in the window hierarchy!

From this warning, it's clear that we at least need to have a UIWindow instance to add our view controller to. If we instantiate our own and make our view controller the root view controller of that window, the same warning from above is printed to the console and the presentation still fails. If we attempt to make this window the key window by calling makeKey() or makeKeyAndVisible(), an exception is thrown:

caught "NSInternalInconsistencyException", "props must have a valid clientID"

This is a pretty vague error message, but my guess is that the window cannot become a key window without an application. However, without a host application target, UIApplication.shared has no value:

(lldb) po UIApplication.shared
<uninitialized>

Any attempt to initialize a UIApplication instance manually results in an exception being thrown:

caught "NSInternalInconsistencyException", "There can only be one UIApplication instance."

Adding the code below to the test target in an attempt to designate a UIApplicationDelegate for the main function the way you might normally do it in an iOS application doesn't work either. In this case, application(_:didFinishLaunchingWithOptions:) never even gets called. It seems that the @UIApplicationMain attribute is ignored. I imagine it's only recognized in an application target.

import UIKit

@UIApplicationMain
class AppDelegate: UIResponder, UIApplicationDelegate {
    
    var window: UIWindow?
    
    func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplicationLaunchOptionsKey: Any]?) -> Bool {
        
        window = UIWindow(frame: UIScreen.main.bounds)
        window?.rootViewController = UIViewController()
        window?.makeKeyAndVisible()
        
        return true
    }
}

Got any other ideas for me to test out?

@lgauthier lgauthier force-pushed the feature/AlertControllerAssertion branch from 4c3721f to 89bbc12 Compare July 13, 2017 19:57
@lgauthier lgauthier force-pushed the feature/AlertControllerAssertion branch from c118948 to 1f7a438 Compare July 13, 2017 20:05
@lgauthier
Copy link
Collaborator Author

@nhgrif I've removed the hosted tests target. Setting the view controller as the root view controller of the window doesn't seem to add the view controller's view to the window unless the window is the key window (and it can't be the key window without a host application). However, you can simply add the view controller's view as a subview to the window directly. As long as the view controller's view is a subview of the window, it can successfully present another view controller. So, that's what I did for the alert controller assertion tests.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 98.985% when pulling 1f7a438 on feature/AlertControllerAssertion into fcd3026 on develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 98.985% when pulling 1f7a438 on feature/AlertControllerAssertion into fcd3026 on develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 98.985% when pulling 1f7a438 on feature/AlertControllerAssertion into fcd3026 on develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 98.985% when pulling 1f7a438 on feature/AlertControllerAssertion into fcd3026 on develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 98.985% when pulling 1f7a438 on feature/AlertControllerAssertion into fcd3026 on develop.


// MARK: Action Comparison

fileprivate func matchesActualAction(_ action: UIAlertAction) -> Bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't we just implement a == function for UIAlertAction (and make it Equatable)? If we did that, we could just use XCTAssertEqual on them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, Equatable defines an interface for equating two instances of the same type. In this case, the two things we are comparing would be ExpectedAlertAction and UIAlertAction. I suppose I could implement == (lhs: ExpectedAlertAction, rhs: UIAlertAction) -> Bool for them, but since it's not actually conforming to Equatable, I think I'd also have to implement != (lhs: ExpectedAlertAction, rhs: UIAlertAction) -> Bool, and I think you'd have to implement the operators with the arguments in the reverse order as well. I think Equatable just gives you all that via protocol extensions.

What are your thoughts? In the context where I'm currently using this method, it wouldn't benefit me to be able to use XCTAssertEqual on them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. This makes sense.


class QuotedStringTests: MTKBaseTestCase {

func testQuotedString() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worthwhile to throw in a test for something that looks like this: quotedString("\"I've got quotes in me!\"")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, I think so. Here's a question for you though. What would you expect the output of that to be? Should it simply add quotes around it again even if it already has quotes, or should it detect that it has quotes and not modify the original string? I'm leaning towards adding the extra quotes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it should add the extra quotes indiscriminately. It should only not add quotes in the case of nil (which I believe is what I saw as the current behavior).

if descriptionsForUnexpectedFailures.isEmpty {
expectingFailure = nil
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change primarily to allow multiple failures to be tested at once? Original implementation would only allow for a single failure at a time, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's been a while since I made this change, so I might need to look over it again to verify, but if I recall correctly, this change makes it more obvious what was different about the actual failures in comparison with what we were expecting. I think I was having a test fail and I was having trouble figuring out what wasn't correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, that makes sense.

/// - line: The line number to report the failure for. The default value will report the line number of the calling site.
public func MTKAssertAlertIsPresented(by presenter: UIViewController, style: UIAlertControllerStyle, title: String?, message: String?, actions: [ExpectedAlertAction], _ failureMessage: String? = nil, file: StaticString = #file, line: UInt = #line) {

guard let alert = presenter.presentedViewController as? UIAlertController else {
Copy link
Contributor

Choose a reason for hiding this comment

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

So, I can't remember how this presentedViewController property works in the case of trying to call present multiple times on a view control. Is that even possible or will it cause other problems?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A view controller can only present one view controller at a time. If you try to present another view controller while already presenting, it will fail and a warning will be logged to the console.

Copy link
Contributor

@nhgrif nhgrif left a comment

Choose a reason for hiding this comment

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

Approved as soon as all the automation completes.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 98.985% when pulling d596f9b on feature/AlertControllerAssertion into fcd3026 on develop.

@lgauthier lgauthier merged commit c48e787 into develop Jul 19, 2017
@lgauthier lgauthier deleted the feature/AlertControllerAssertion branch July 19, 2017 21:37
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

3 participants