Callback context is inconsistent

[trentmwillis]

Currently when callbacks for a test module are contextualized, they get bound to the test module rather than the test context. This causes inconsistent behavior, since setup/teardown get invoked with the test context.

Practically, this means that if you do something such as:

moduleFor('component:foo-bar', {
  integration: true,

  setup() {
    this.set('title', 'first title'); // works as expected
  },

  changeTitle() {
    this.set('title', 'second title'); // doesn't work
  }
});

test('it renders', function(assert) {
  this.changeTitle();
});

changeTitle won't work properly, instead you have to do:

changeTitle() {
  this.context.set('title', 'second title');
}

This is inconsistent and not intuitive. Can we change it such that all callbacks are invoked with the test context? This might limit access to some test module properties, but I feel most of those shouldn't be getting accessed anyway.

[rwjblue]

Generally, I agree with you that the context should be the same for all of the callbacks. Unfortunately, this is a breaking change and I am not sure how exactly we would go about deprecating the current behavior.

@dgeb - Thoughts?

[mmun]

I also agree. Having consistent context on all the methods is important for preserving the QUnit mental model.

[trentmwillis]

@rwjblue true, definitely breaking. Could we do something like proxy attributes from the test module onto the context? Would probably get ugly though.

[rwjblue]

Yes, @mmun and I were chatting this through in slack and I came up with the following snippet, that should work in a backwards compatible way:

buildDeprecatedContext(module, context) {
  let keysForDeprecation = Object.keys(module);

  if (keysForDeprecation.length === 0) {
    return; // do not call Object.create if we don't need to
  }

  let wrappedContext = Object.create(context);

  for (let index = 0, length = keysForDeprecation.length; index++; index < length) {
    let key = keys[index];

    Object.defineProperty(wrappedContext, key, {
      get: function() {
        Ember.deprecate('ffffuuuuuu, fix it!');
        return module[key];
      }
    });
  });
},

And call that from contextualizeCallbacks (or something like that).

We would need tests for:

• accessing the TestModule properties on this with a deprecation
• accessing the test context as this without deprecation

@trentmwillis - Think you would like to tackle this?

[trentmwillis]

I'd definitely be willing to work on this. One remaining question, how should we handle the situation where a user has defined something on the context that conflicts with the module? I'm thinking let the module "win" (as happens in your snippet), but wasn't sure if there is a better approach for that edge case.

[rwjblue]

Yes, module should win with a more specific deprecation

[Turbo87]

@rwjblue seems this has been resolved by #136