"defined" is not a magic Chai method

[jridgewell]

Have I ever mentioned how much I hate chai's magic property-access getter method bulllshit?

.defined is not a Chai method. But because it uses stupid getter API (and not real methods), you'd never know it.

expect(null).to.be.defined; // Gee, `defined` isn't a Chai assertion but we'd never know it.
// My tests pass! What could go wrong!

Instead, any sane API would use real methods

expect(null).to.be.defined(); // Error! `defined` isn't a method. Guess we know to update it now.

$ grep "\.defined;"

extensions/amp-install-serviceworker/0.1/test/test-amp-install-serviceworker.js
89:    expect(implementation).to.be.defined;
105:    expect(implementation).to.be.defined;
127:    expect(implementation).to.be.defined;

test/functional/test-alp-handler.js
300:    expect(image).to.be.defined;

test/functional/test-impression.js
38:    expect(xhr.fetchJson).to.be.defined;

test/functional/test-xhr.js
303:                expect(error.response).to.be.defined;
315:                expect(error.responseJson).to.be.defined;
327:                expect(error.responseJson).to.be.defined;
477:          expect(e.retriable).to.be.defined;
493:          expect(e.retriable).to.be.defined;

test/integration/test-amp-ad-3p.js
66:    expect(context.clientId).to.be.defined;
69:    expect(context.container).to.be.defined;
70:    expect(context.initialIntersection).to.be.defined;
72:    expect(context.initialLayoutRect).to.be.defined;
73:    expect(context.initialLayoutRect.top).to.be.defined;
74:    expect(context.initialIntersection.rootBounds).to.be.defined;
75:    expect(context.isMaster).to.be.defined;
76:    expect(context.computeInMasterFrame).to.be.defined;
77:    expect(context.location).to.be.defined;

test/integration/test-amp-ad-doubleclick.js
84:      expect(context.initialLayoutRect).to.be.defined;
85:      expect(context.initialLayoutRect.top).to.be.defined;
86:      expect(context.initialIntersection).to.be.defined;
87:      expect(context.initialIntersection.rootBounds).to.be.defined;

[cramforce]

This is so bad.

Could be fixed with something like

const handler = {
    get: function(target, name) {
      if (Object.getOwnPropertyDescriptor(target, name)) {
          return target[name];
      }
      throw new Error('Unsupported property: ' + name)
    }
};

new Proxy(chaiPrototype, handler);

[jridgewell]

Apparently they do exactly this in Chai 4.0.0.

[rsimha]

@jridgewell This can be solved in the following ways:

1. By adding a chai property called defined that throws an error, and changing all existing invocations of to.be.defined to to.not.be.undefined.
2. By implementing a defined property that calls expect(this._obj).to.not.be.undefined;
3. By upgrading to chai 4.0.0

1 will solve the problem and prevent future misuse, but isn't elegant. 2 will add useful functionality, but won't prevent the use of the antipattern. I don't think 3 will work with the rest of our testing stack.

Thoughts?

[jridgewell]

I think 3 is the only appropriate answer. Defining #defined doesn't help when I mistype the next one, or think that #tralalalala is a Chai method.

[rsimha]

Fair point. There's a cascading chain of dependencies that will need to be upgraded to get to chai 4, but I think I have it working. Sending out an updated PR.

[rsimha]

The upgrade to chai 4 has exposed several nasty bugs in the AMP test code. Luckily, fixes for all of them are coming up in #11390. Stay tuned.

[rsimha]

This is now fixed via an upgrade to chai 4, along with many other bogus assertions that were silently passing.