waitForPromises() should be removed

[virl]

waitForPromises() should be removed because it is an abstraction leak: you never know in the test what other promises tested code will use in it internal implementation.

"So waiting for a bunch of async tasks to finish in a test can be tricky." (from docs) — no, it is tricky on Android that doesn't have any SDK support for it.

On iOS, we have XCTestExpectation exactly for that, without false hope for tested code to move into predictable state by leaking its implementation details into the test.

[shoumikhin]

Hi Vladimir, if I'm not mistaken, you're concerned about a test case (presumably, integration one?) where the user may create a bunch of promises internally, which may not be related directly to the functionality being tested, and thus create a race condition when waitForPromises is being used for synchronization, which eventually would result in a flaky test?

My take on that is the user should be well aware of what waitForPromises does, and gladly use XCTest framework when the former appears insufficient or potentially error-prone for a specific test case. Luckily, waitForPromises is not the only true way to test promises, it's just the simplest one.

Thanks.

[virl]

@shoumikhin Yes, I'm concerned about that test case which will be quite common for any complex asynchronous system (and async code is complex by default). Also I'm concerned that that method will be used in production code of that system (for example, in SDK), which will create deadlock if I will use your Promises library in my app that uses SDK that uses your library too.

waitForPromises is not the only true way to test promises, it's just the simplest one

And also most incorrect one, so everyone will be accustomed to incorrect way from the start.

You cannot determine that all promises are completed anyway (so method name is a lie) due to fundamental Halting Problem: just after waitForPromises() return, some background thread/queue can just create yet another promise.

[shoumikhin]

Not sure I get why you consider it incorrect? Any examples?

Also, why do you think that method is gonna be used in production? It's located in a separate header in Objective-C, and is internal in Swift, so you can't just leverage it for anything w/o explicitly importing a test-only declaration.

I totally get that another promise can be created after waitForPromises and thus, stay unfulfilled. Then your next test will likely time out, so you're gonna investigate what went wrong and probably find a bug or fix the test case to become isolated.

[virl]

@shoumikhin

Not sure I get why you consider it incorrect? Any examples?

Yes, consider that iOS would have following method of NSThread class for async testing:

- (void) waitForAllThreadsExceptMain()

And that Apple would recommend its usage in XCTest documentation as simplest way to write asynchronous test.

You cannot wait for ALL threads and promises, even in tests, because threads and promises are ALWAYS implementation detail that may not be visible to the test and that are thus prone to race conditions from test's point of view.

I totally get that another promise can be created after waitForPromises and thus, stay unfulfilled.

Or tested code can create arbitrary number of promises while you're waiting in waitForPromises(). And it would do it as part of its CORRECT operation.

That's the core problem: you cannot distinguish freeze of the system from its correct operation. That was proved by Alan Turing in 1936: https://en.wikipedia.org/wiki/Halting_problem

Your method waitForPromises() incorrectly implies directly opposite to that.

Then your next test will likely time out, so you're gonna investigate what went wrong and probably find a bug or fix the test case to become isolated.

No, you would not find that bug, because it will be the result of asynchronous race condition by definition.

Or your waitForPromises() would just hang sometimes for arbitrary amount of time for CORRECT tests with CORRECT code.

[shoumikhin]

Sorry, if I understand you correctly, your concern around waitForPromises is that it can be potentially misused and become error-prone if blindly used everywhere?

Do you suggest to refine the docs then, and explicitly recommend using XCTest instead if unsure or facing issues with waitForPromises?

[virl]

@shoumikhin

if I understand you correctly, your concern around waitForPromises is that it can be potentially misused and become error-prone if blindly used everywhere?

No, my concern is that any use of waitForPromises() is inherently incorrect, because it implies wrong assumptions that:

1. Tested code do not use promises in its internal implementation — and thus makes async test to be dependent on implementation details and fail when tested code still works correctly but just changed implementation.
2. It can distinguish between freezed and correct states of tested code when it creates large number of promises in its internal implementation.

In overall, this method represents completely incorrect way of writing async tests: instead of waiting for the overall state of the async system (which is prone to deadlocks and race conditions) you must wait for particular events from it.

XCTestExpectation does exactly that.

Do you suggest to refine the docs then, and explicitly recommend using XCTest instead if unsure or facing issues with waitForPromises?

No, I suggest removing waitForPromises() entirely, because its testing paradigm is entirely incorrect and also useless on platform that have perfect async code testing capabilities like XCTestExpectation.

And current waitForPromises() implementation is awful anyway because it works like spinlock and eats CPU for breakfast, which will lead to excessive CI server usage. So nothing will be lost from its removal anyway.

[shoumikhin]

Sure, so you may perceive waitForPromises as a mean to wait for an event "all pending promises have been resolved up to this moment". That's the only assumption it implies. If you want to wait for some other system state, and are able to expose it in tests for proper event handling, you're welcome to use XCTest framework exclusively and never touch that utility method.

[virl]

@shoumikhin

waitForPromises as a mean to wait for an event "all pending promises have been resolved up to this moment". That's the only assumption it implies.

It is not an event, it is implementation detail of the tested code, which may change without changing code's correct external behaviour. Therefore, waitForPromise() should not be used in tests, because it makes tests dependable on hidden implementation details of tested code.

[shoumikhin]

I think it really depends on how you look at it and what exactly you test.

Sounds like you're talking about some integration tests and check for a specific set of events to happen, while ignoring the others. And you also don't expect a test to change much while working on implementation details. Surely, waitForPromises may not be an ideal candidate for that.

[virl]

@shoumikhin

Sounds like you're talking about some integration tests and check for a specific set of events to happen, while ignoring the others.

No, I'm talking about the fact that test should not depend on tested code's implementation details that it cannot see.

Your promises can (and will) be used in implementation of classes that return OTHER promises via external public API. waitForPromises() will immediately lead to incorrect behaviour of tests in that case. Because it uses fundamentally wrong testing paradigm.

[shoumikhin]

I see. Well, waitForPromises isn't considered a panacea or something every test that rely on promises must use. It's mostly used to test the lib itself. Nevertheless, we found a few other good use cases where we couldn't explicitly expose all the events to the test methods in order to observe completion, but had to wait for them somehow. Hope that helps. Feel free to send a PR to refine the docs, if needed.

[virl]

My position is that it should be removed from library, not have changed just changed docs. So no PR. :)

Exposing internal events to test can be done via Swift’s internal keyword and following import:

’’’
@testable import MyFramework
’’’

[shoumikhin]

Sounds good, happy to hear other opinions, so let this issue hang for a while.
I know it's used at Google for things where @testable doesn't just work.

[ghost]

@virl thank you for the discussion!

After discussing this further, we believe there are valid use cases where developers would want to use waitForPromises for testing purposes. However, the preferred approach is still to leverage XCTest (i.e. XCTestExpectation, XCTNSPredicateExpectation, etc.) where possible.

Additionally, to make the intention of the waitForPromises API in Swift more clear, we've limited the access level to internal (i.e. @testable is required) and defined the API in Promise+Testing.swift.

If you have any additional proposals to help make the API more clear, please feel free to submit a PR with the changes. Thank you!