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

Test helpers wait and andThen not waiting after jQuery 3 upgrade #14566

Closed
dwickern opened this issue Oct 31, 2016 · 21 comments
Closed

Test helpers wait and andThen not waiting after jQuery 3 upgrade #14566

dwickern opened this issue Oct 31, 2016 · 21 comments

Comments

@dwickern
Copy link

dwickern commented Oct 31, 2016

Upgrading to jQuery 3 can cause mysterious, intermittent test failures. andThen blocks will sometimes fire before the logic is done executing. This happens when using jQuery promises (either directly or through third-party jQuery UI components).

Specifically, this change in jQuery 3:
http://jquery.com/upgrade-guide/3.0/#callback-invocation

Another behavior change required for Promises/A+ compliance is that Deferred .then() callbacks are always called asynchronously. Previously, if a .then() callback was added to a Deferred that was already resolved or rejected, the callback would run immediately and synchronously.

This means that, even if an app uses jQuery promises in a synchronous manner (ie. $.Deferred.resolve(value)), the code runs asynchronously and must be tested asynchronously.

Testing them asynchronously is not easy because Ember's wait helpers do not wait for jQuery promises (only route transitions and Ajax requests).

@dwickern
Copy link
Author

dwickern commented Oct 31, 2016

One solution: the app developer can wrap any jQuery promise interactions with Ember.Test.registerWaiter as described here. Then Ember will wait for those promises to complete.

This puts a heavy burden on the app developer, and doesn't help at all if jQuery promises are used internally (and not exposed) by third-party code.


Another solution: make Ember aware of all jQuery promises. This can be done using some hacks. Add to /tests/test-helper.js

/** the number of unresolved promises */
let pending = 0;

Ember.Test.registerWaiter(() => pending === 0);

function shim(obj, name, fn) {
  let original = obj[name];
  obj[name] = function(...args) {
    return fn.apply(this, [ original, ...args ]);
  };
}

function trackedThen(oldThen, onFulfilled, onRejected, onProgress) {
  ++pending;
  return oldThen.call(this, function success(...args) {
    --pending;
    if (onFulfilled) onFulfilled(...args);
  }, function failure(...args) {
    --pending;
    if (onRejected) onRejected(...args);
  }, onProgress);
}

shim(window.jQuery, 'Deferred', function(oldDeferred, ...args) {
  let deferred = oldDeferred.call(this, ...args);
  shim(deferred, 'promise', function(oldPromise, ...args) {
    let promise = oldPromise.call(this, ...args);
    shim(promise, 'then', trackedThen);
    return promise;
  });
  return deferred;
});

@pixelhandler
Copy link
Contributor

pixelhandler commented Nov 4, 2016

@dwickern can you create a working code example to demonstrate the issue? Perhaps an ember-twiddle ?

@dwickern
Copy link
Author

dwickern commented Nov 4, 2016

@jordpo posted this repro in ember slack: https://github.com/jordpo/ember-test-issue

With jQuery 3:

$ ember test --filter Acceptance
not ok 1 PhantomJS 2.1 - Acceptance | auth/login: visiting /auth/login
    ---
        actual: >
            auth.login
        expected: >
            dashboard
        stack: >
            http://localhost:7357/assets/tests.js:21:19
            andThen@http://localhost:7357/assets/vendor.js:51156:41
            http://localhost:7357/assets/vendor.js:50932:24
            isolate@http://localhost:7357/assets/vendor.js:51998:30
            http://localhost:7357/assets/vendor.js:51952:23
            tryCatch@http://localhost:7357/assets/vendor.js:61487:22
            invokeCallback@http://localhost:7357/assets/vendor.js:61502:23
            publish@http://localhost:7357/assets/vendor.js:61470:23
            http://localhost:7357/assets/vendor.js:50950:24
            invoke@http://localhost:7357/assets/vendor.js:11523:20
            flush@http://localhost:7357/assets/vendor.js:11587:17
            flush@http://localhost:7357/assets/vendor.js:11395:22
            end@http://localhost:7357/assets/vendor.js:10709:30
            run@http://localhost:7357/assets/vendor.js:10831:21
            run@http://localhost:7357/assets/vendor.js:34261:32
            http://localhost:7357/assets/vendor.js:51460:36
        Log: |
    ...
ok 2 PhantomJS 2.1 - JSHint | acceptance/auth/login-test.js: should pass jshint
ok 3 PhantomJS 2.1 - JSHint | helpers/module-for-acceptance.js: should pass jshint

1..3
# tests 3
# pass  2
# skip  0
# fail  1

With jQuery 2:

$ bower install jquery#^2 && ember test --filter Acceptance

ok 1 PhantomJS 2.1 - Acceptance | auth/login: visiting /auth/login
ok 2 PhantomJS 2.1 - JSHint | acceptance/auth/login-test.js: should pass jshint
ok 3 PhantomJS 2.1 - JSHint | helpers/module-for-acceptance.js: should pass jshint

1..3
# tests 3
# pass  3
# skip  0
# fail  0

# ok

@validkeys
Copy link

I'm actually also having this issue when using regular Ember.$.ajax methods. Since upgrading to 2.9, all of my tests that jquery ajax requests cause tests to fail either with:

  1. the tests failing even though if I step through each line in the debugger, all of the conditions are visually met
  2. Getting the error onFulfillment is not a function

The weird this is that I'm wrapping the ajax request in an RSVP promise, so there shouldn't be any incompatibilities that i know of.

Below is my code:

    let self = this;
    return new Ember.RSVP.Promise((resolve, reject) => {
      Ember.$.ajax({
        method:     "POST",
        url:        "/api/v1/sessions",
        data:        JSON.stringify(options),
        contentType: "application/json; charset=utf-8",
        dataType:    "json",
        cache: false
      }).then((res) => {
        let { user } = res;
        Ember.$.ajaxSetup({
          headers: {
            "Authorization": `Bearer ${res.jwt}`
          }
        });
        if (user) {
          Ember.run(self, function() {
            self.get('segment').identifyUser(user.id, { name: `${user.firstName} ${user.lastName}` });
            self.get('store').pushPayload('user', { user });
          });
        }
        resolve(res);
      }, (xhr/*, status, err*/) => {
        console.log("authenticate error", arguments);
        return reject(xhr.responseJSON);
      });

@dwickern
Copy link
Author

dwickern commented Nov 6, 2016

@validkeys I was confused about this as well. I thought wrapping the jQuery promises with RSVP promises would solve the problem, but it doesn't help.

wait / andThen don't wait for jQuery promises or RSVP promises to resolve. Your andThen will fire before your promise.then().

If you changed your code like this, it would work:

    let isRunning = false;
    Ember.Test.registerWaiter(() => isRunning);

    isRunning = true;
    return new Ember.RSVP.Promise((resolve, reject) => {
      Ember.$.ajax({ ... }).then((res) => {
        isRunning = false;
        resolve(res);
      }, (xhr) => {
        isRunning = false;
        return reject(xhr.responseJSON);
      });

@pixelhandler
Copy link
Contributor

pixelhandler commented Nov 11, 2016

@dwickern the current release of Ember (2.9.1) seems to indicate jQuery v3 should be supported, see https://github.com/emberjs/ember.js/blob/v2.9.1/config/package_manager_files/package.json#L11-L13

However, it appears that jQuery.ajax in v3 may not be compatible with the wait test helpers.

@pixelhandler
Copy link
Contributor

@dwickern I tried to make an ember-twiddle example see: https://ember-twiddle.com/67dc4c9ffcc743610b9cff5bea7b80e0?openFiles=tests.acceptance.index.js%2C but can't get the test feature to just work for an acceptance test.

@dwickern
Copy link
Author

@pixelhandler looks like it might be related to ember-cli/ember-cli#6247

Twiddle hasn't updated to [email protected]

@pixelhandler
Copy link
Contributor

@dwickern I have a repo that I'm working on for ember-fetchjax which uses fetch or ajax. The repo is using jQuery 3.1.1. The ajax use does wrap jQuery.ajax with a Promise and the acceptance tests seem to work fine with andThen behavior.

I think there are various options you can use with jQuery 3.1.x

  • Wrap $.ajax with Promise
  • Use ember-ajax addon
  • Perhaps migrate to Fetch with a polyfill file for XHR (as long as you don't need to cancel a request)
    • The addon above is an example that you could use to progressively migrate, use Fetch where it makes sense and use AJAX where you still need it.

@pfmmfp
Copy link

pfmmfp commented Jan 23, 2017

I'm running into this same problem on an acceptance test against the login page using ember-simple-auth-token addon. The addon is actually wrapping the $.ajax in a RSVP.Promise (ref here). So it seems that it is not an option? Will keep looking into this.

@dwickern
Copy link
Author

@pfmmfp You could try the workaround in #14566 (comment)

morhook added a commit to morhook/ember-simple-auth-token that referenced this issue Jan 24, 2017
morhook added a commit to morhook/ember-simple-auth-token that referenced this issue Jan 24, 2017
@morhook
Copy link
Contributor

morhook commented Jan 24, 2017

As my PR shows (inspired on the jQuery migration guide), can't we migrate the then usage to done and fail ? This looks to not have random failures in my testing.

In other words, the andThen seems to wait correctly when wrapping done and fail inside an RSVP.

The ember-simple-auth library is also calling .then on the jquery.ajax call, so I expect to have random failures on acceptance testing (with jQuery 3.x.x of course), regarding test-case of @dwickern ( https://github.com/jordpo/ember-test-issue )

p.s.: I'm working on this issue with @pfmmfp if it helps

@dwickern
Copy link
Author

@morhook Apparently jQuery still calls done synchronously:

p=$.Deferred().resolve(42)
p.done(r => console.log(r)); console.log('done')
// prints: 42 done

p.then(r => console.log(r)); console.log('then')
// prints: then 42

done and fail are not standard Promise APIs though so you may not want to rely on them.

@ashrafhasson
Copy link

I'm facing this issue in acceptance test too. I use ESA 1.4.0 and leveraging ember-data 2.13.1 to fetch the user and load it into a current-user service. Attempting to work around it with registerWaiter() isn't helping.

import Ember from 'ember';
import DS from 'ember-data';

const { set, get } = Ember;

export default Ember.Service.extend({
  init() {
    this._super(...arguments);

    if (Ember.testing) {
      set(this, '_loading', false);
      Ember.Test.registerWaiter(() => get(this, '_loading') === false);
    }
  },

  session: Ember.inject.service('session'),
  store: Ember.inject.service('store'),

  userId: Ember.computed.oneWay('session.data.authenticated.user_id'),

  permissions: Ember.computed('user.securityGroups.[]', {
    get() {
      let promise = new Ember.RSVP.Promise((resolve) => {
        const permissions = get(this, 'user.securityGroups')
              .mapBy('permissions')
              .reduce((a, b) => a.concat(b))
              .toArray();

        Ember.run(() => resolve(permissions));
      }, 'Current User Service: Permissions Promise');

      if (Ember.testing) {
        set(this, '_loading', true);
        return DS.PromiseArray.create({
          promise: promise.finally(() => set(this, '_loading', false))
        });
      }

      return DS.PromiseArray.create({
        promise: promise
      });
    }
  }),

  load() {

    let promise = new Ember.RSVP.Promise((resolve, reject) => {
      let store = get(this, 'store');

      if (get(this, 'session.isAuthenticated')) {
        let userId = get(this, 'userId');
        const user = store.findRecord('user', userId);
        Ember.run(() => resolve(user));
      } else {
        resolve();
      }
    }, 'Current User Service: loading user');

    if (Ember.testing) {
      set(this, '_loading', true);
      let userPromiseObject = DS.PromiseObject.create({
        promise: promise.finally(() => set(this, '_loading', false))
      });

      set(this, 'user', userPromiseObject);
    }

    let userPromiseObject = DS.PromiseObject.create({
      promise: promise
    });

    set(this, 'user', userPromiseObject);
  }
});

@ashrafhasson
Copy link

downgrading to jquery 2.2.4 doesn't seem to resolve the test failure. I'm using ember/ember-cli 2.14.1, ember-data 2.13.1.

@lolmaus
Copy link
Contributor

lolmaus commented Mar 9, 2018

I've upgraded to Ember 3.1.0-beta.1. I've renamed my tests/ folder, created one with the new test infrastructure and now I'm porting my tests to it.

I'm seeing various tests finishing prematurely. Lots of calling set on destroyed object errors.

One of the tests is particularly interesting in regard of this issue. It does this: await loginPage.loginButton.click() which is calling ESA's session.authenticate() which in turn is using ember-ajax.

Previously this was working fine: the test waited for logging in to complete and would resume after transitioning to another route. Now the test resumes while the app is still in the loading route and the login form says Logging in.... Adding await wait() does not help.

The interesting part is that ember-ajax does its own registerWaiter: https://github.com/ember-cli/ember-ajax/blob/v3.1.0/addon/mixins/ajax-request.js#L90-L95

I've edited that waiter to console.log the number, and I've updated my authenticator to do this:

    console.log('starting')
    return this
      .get('ajax')
      .login(password)
      .then(result => {
        console.log('finishing')
        return result
      })

The resulting output is:

image

Note how the number resets back to zero, and the waiter triggers several times before .then resolves. I believe this is the root of the issue: the waite r should respect .thens attached to the promise. In other words, the waiter should not resume the test immediately after the promise resolves, it should also wait for .thens attached to the promise to resolve as well.

PS ember-ajax is using jQuery, but it wraps its promise into RSVP.

@lolmaus
Copy link
Contributor

lolmaus commented Mar 9, 2018

I've tried editing ember-ajax to move the waiter counter decrement from jQuery.always() into RSVP.Promise.finally() and it made no difference.

So maybe this issue isn't related to jQuery at all. My current impression is that we now have to apply a waiter to every promise manually, which is a heavy burden for developers, like @dwickern pointed out.

@lolmaus
Copy link
Contributor

lolmaus commented Mar 10, 2018

I've tried using registerWaiter for my promises, but it does not help: the test still won't wait for transitions initiated from inside those promises.

@lolmaus
Copy link
Contributor

lolmaus commented Mar 10, 2018

I've published a reproduction.

Project: https://github.com/bread-maker/bread-maker-ember-frontend
Branch: ember-3-upgrade
Test command:

ember t -s --no-launch -f "should redirect to login from settings when not authenticated"

Things to try:

  1. Run the command, open the link and see two assertions fail because the test finishes prematurely:
    • current URL
    • Current token in session
  2. Uncommenting app/pods/login/controller.js#L23 will apply registerWaiter to the promise returned by ESA's session.authenticate().
    Now the Current token in session assertion passes, but the current URL assertion still fails. That's because the test still finishes prematurely, before the transition initiated by session.authenticate() completes.
  3. Uncomment the debugger statement on tests/acceptance/login-test.js#L38 to freeze the app at the exact moment the test is about to finish: you'll see that the transition hasn't happened yet.
  4. Now comment the debugger back and uncomment await pauseTest() on tests/acceptance/login-test.js#L39. The pause will prevent the test from completing without freezeing the app. You'll see that the transition does happen, and the test would succeed if it waited for the transition.

Here's the same test before the Ember 3 upgrade. It does work successfully.

@pixelhandler
Copy link
Contributor

@dwickern what do you think about closing out this issue? Now that we have the new ember test helpers we have an alternative to using andThen(), https://github.com/emberjs/ember-test-helpers/blob/master/API.md

@dwickern
Copy link
Author

I don't have this issue any more after switching from andThen() to async/await

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants