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

Using ESA in application (acceptance) tests requires manual use of test waiters #2272

Open
gabrielgrant opened this issue Feb 16, 2021 · 5 comments

Comments

@gabrielgrant
Copy link

It seems that to safely use ESA in an application test requires each app to wrap any action handlers that trigger async methods on the ESA session with the waitFor decorator from @ember/test-waiters

See details in this discuss thread

While explicitly using waitFor in app code does seem to work, it also doesn’t feel like the type of thing every app should have to concern itself with. Would it make sense to have (ie would the maintainers be open to receiving PRs that implement) test waiting behavior within the internal session itself, so that tests would be paused until promises from authenticators' hooks either resolve or reject?

Or is the intention that this be something that should be handled in each authenticator? Or does it really need to be handled by each app individually? In either of these cases, it would be helpful to make note of it (either in the Implementing a Custom Authenticator](https://github.com/simplabs/ember-simple-auth#implementing-a-custom-authenticator) section or in the Testing section respectively)

@BobrImperator
Copy link
Collaborator

Sorry for a late response.

I'm trying to understand the issue, but I don't see so far how it could be happening outside of a gut feeling that it could be a problem with RSVP.Promise and Native Promise in older ember versions. All authenticators in ESA return RSVP promises so far and not native ones.

@gabrielgrant
Copy link
Author

Thanks for taking a look at this :)

My understanding of the issue is that ember's tests, by default, do not wait for all promises (even all RSVP promises) before proceeding, only those that are explicitly instrumented to be waited upon (which is the case for most of the routing-related promises in Ember core, for example). Specifically, from @rwjblue's comment on that discuss thread I linked:

nothing in the [Ember testing] system knows that it should “wait” for the async action to complete!

Lets break down what is waited for automatically by calls to await settled() (which is internally used by await click(...) as well). Settledness in this context is defined as:

Has no active runloop
Has no pending runloop timers (e.g. usages of schedule from @ember/runloop)
Has no pending test waiters (this one is important, we will come back to it)
Has no pending jQuery requests (generally not applicable in modern octane apps, since they do not use jQuery)
Has no pending router transitions (e.g. all model hooks are resolved, any redirects have been absorbed, etc)

One very important thing to call out here: we do not wait for “all promises to be resolved”! (Doing this is basically impossible anyways…)

So what I'm proposing (or at least what seems like the best course of action from my PoV) is adding an explicit test waiter into ESA (using @ember/test-waiters) so that all users' test code will wait for relevant promises in the authenticators before proceeding without the users having to do that instrumentation manually

@BobrImperator
Copy link
Collaborator

I'll need to take a look at this later with some repros, though unfortunately at the moment we're doing some major internal work, so this will need to wait.

But I'm not convinced that this is the issue, I'm running multiple login e2e test suites in my app using ESA as well as custom authenticators returning native promises and everything is correctly awaited without me doing anything in particular in my tests.

Could you please give some details on what authenticators you're using and share some of their implementation as well as ESA and ember versions?

@gabrielgrant
Copy link
Author

gabrielgrant commented Aug 30, 2021

It's been while since I was actively working on this issue (ended up taking another direction on the project in Feb after first posting this in Feb), but IIRC I was encountering the problem when trying to use ESA with Firebase.

Is it possible that your app is using ember-fetch or some other communication method that is runloop-aware? The issue in the case of Firebase is that the underlying Firebase libraries use their own communication channels that are not instrumented by Ember (not 100% sure of the details, but my understanding is that ember-fetch makes the Fetch APIs runloop-aware even when used with native promises).

@rwjblue (Ember core maintainer, and primary author of ember-test-helpers) is pretty emphatic that

nothing in the system knows that it should “wait” for the async action to complete! [because ember-test-helpers'] await settled() does not wait for “all promises to be resolved”!

and that manual instrumentation with ember/test-waiters's @waitFor is needed

There are more details, and another specific example of the original poster trying to use SailsJS, here: https://discuss.emberjs.com/t/ember-test-with-async-await-functions-not-working-as-intended/

@gabrielgrant
Copy link
Author

Actually, looking at the ember-test-helpers docs, it appears that await settled does try to wait for all JQuery-dispatched AJAX requests by default (see hasPendingRequests): https://github.com/emberjs/ember-test-helpers/blob/master/API.md#getsettledstate

Not sure if this has been updated to try to instrument non-JQuery-based AJAX requests, now that Ember has removed JQuery by default. In the case of Firebase, though, my understanding is that the auth request (and all others) is dispatched across a websocket connection, so that still wouldn't be caught without explicit instrumentation.

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

No branches or pull requests

2 participants