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

Refactor plugin declarations #585

Closed
keithamus opened this issue Jan 5, 2016 · 25 comments
Closed

Refactor plugin declarations #585

keithamus opened this issue Jan 5, 2016 · 25 comments

Comments

@keithamus
Copy link
Member

keithamus commented Jan 5, 2016

I'm going to put this document here, as the start of a design discussion around how plugins could be written for a future version of chai. Similar discussions have been had before, over in Google Groups, and issues #117 and #457; but I wanted to start a new issue which can focus on the design of a new plugin architecture, and hopefully get some feedback.

This comment gets updated regularly to reflect the discussion below, for older version of this proposal, see https://gist.github.com/keithamus/43f7d102ffbf441107a6ae19decaca23/revisions

Current Implementation

So, just to get everyone on track - right now, you can make a chai plugin by writing a little bit of code, like this:

export default (chai, utils) => {
  chai.Assertion.addMethod('equal', function (val, msg) {
    this.assert(
      val === obj,
      'expected #{this} to equal #{exp}',
      'expected #{this} to not equal #{exp}',
      val,
      this._obj,
      true
    );
  });

}

Motivation

Right now we have a nicely expressive interface for creating plugins - but it has a few problems, which can be addressed by making working on a new interface. The problems it has are:

  • It caters to expect and should interfaces as first class, but does not create methods for the assert interface.
  • Plugins define whether or not an assertion is a property or a method - there is addMethod, addProperty, and addChainableMethod. These in themselves aren't bad, but it does create a bit of an inconsistent interface, as it is an opt-in convention to use property assertions and chainable methods. Similarly, for users who only want to use method assertions, they have to use plugins like dirty-chai.
  • Also around convention, it's up to the plugin author whether or not a method supports reassignment of the message. Chai's core assertions allow you to overwrite the assertion message by passing in the last argument (e.g. expect(true).to.equal(true, 'True should be true!')). Some plugins dont do this.
  • Flags are imperative - and it's up to the methods as to whether or not they support the flag, and as such they work for some methods but not for others, and there is no way to determine which assertions support which flags.
  • Many plugins, such as chai-things or chai-as-promised effectively add one or two "interceptor" flags - for example chai-as-promised adds eventually to every method, which means programmatically rewriting every function. We should have an expressive syntax for this.
  • Some flags are "first class" flags, like not. In fact, not is so special that every assertion is required to support it. This should not be the case.
  • We have a strange and somewhat limited templating language for messages some work took place to refactor it and add lots of functionality - but this pushed us further into having specific syntaxes.
  • chai.Assertion.addMethod passes a this value, and a this.assert must be called within an assertion, to assert on anything. This could be simplified, and made less dependant on magic values like this.
  • this.assert has a list of ambiguous arguments, including boolean flags, and so it can be hard to decipher what is actually happening. If you don't pass true as the 6th argument - a diff wont show up for the assertion. This is a gotcha for even some of our best plugin authors.
  • Aliases are very common, but there is no syntax or helpers to support this - and so aliases have to be created manually.

Requirements

Given the issues, we can set out a list of requirements that a new architecture should provide:

Must haves

  • Adding methods must be made more declarative, to support pushing it to different interfaces, rather than having an imperative concatenation of methods via the existing addMethod/addProperty/addChainableMethod functions.
  • Flags should also be declarative. Flags are mainly used to alter the behaviour of one or more methods. We should provide a cleaner way to facilitate this.
  • Methods should know as little as possible about flags (maybe nothing at all?). Separation of responsibilities.
  • The methods that are passed to addMethod should be able to be implemented much simpler - most small methods probably be distilled down to returning true/false. Making them "pure functions" (arguments are the only input, we determine what to do on the output, no side effects) would be an ideal.
  • Methods should not have to determine details such as whether or not to display a diff, this should be dealt with much more on the library level.

Nice to haves

  • Drastically simplifying error messages would be nice. Perhaps we could just rely on ES6 template strings? Since almost every error message follows the same format of expected {object} to {some expectation} but got {value} - perhaps we could just have assertions declare the {some expectation} part. We can generate error messages based on the actual keyword chain used for the assertion (e.g. expect(1).to.equal(2) can be Error: expected 1 to equal 2).
  • If methods also declared their documentation, types, and examples using JavaScript, we could have some fantastic tools to generate docs and provide more helpful information with errors.

Draft concept

To start really thinking about a design - here is a draft concept I think could work when defining new assertions (this is an iteration on a previous draft in #117

Please note:

This is an opportunity to comment, pick apart, bikeshed, this API. Let's get this absolutely right to make sure we have the best plugin system for a truly extensible chai framework 😉

// All chai plugins must export an Object.
// The reason we export an object, rather than a function - is that plugins should
// not have access to chai or chai's utils. Why? Well, because we aim to modularise
// all of them into separate packages, and plugins should rely on npm modules instead
// of chai's utils to write assertions.
export default {

  // The plugin object has 6 main keys:
  //  - name: The name of the plugin. Useful for error messages
  //  - namespace: The namespace of the plugin, usually blank but could be related
  //    to the plugin.
  //  - assertions: A list of assertions that extend Assert API. These assertion
  //    objects must follow a specific format, described below.
  //  - interceptors, which are methods (which may include flags) that can alter an
  //    object before it is passed to an assertion
  //  - modifiers: Methods (which include flags) that can alter the result of an
  //    assertion before it is finally reported
  //  - globals: any property in this object will be added to the `chai` object.
  //    Useful, for example in a spy/mocking tool which might want to add a
  //    `spy()` method to this object; or an http tool might want to add
  //    `request()` method. It is also recommended that any globals get exported
  //    as named exports, to facilitate both uses below:
  //
  // ```js
  // chaiSpies = require('chai-spies');
  // chai.use(chaiSpies);
  // chai.spy(...)
  // ```
  // (or in ES6):
  // ```js
  // import chaiSpies, { spy } from 'chai-spies';
  // chai.use(chaiSpies);
  // spy(...)
  // ```

  // Assertions is a hash of assertion objects. The keys are the names of the
  // assertions, and are read at runtime and added to the fluent interface.
  assertions: {

    // Assertions are just properties on the Object literal.
    // The assertion method is given 2 arguments, the actual and the expected.
    // It can return one of two things:
    //   - A boolean, which determines the result of the assertion
    //   - An object, with the properties `result`, `actual`, and `expected`.
    //     This is more useful if the `actual` or `expected` properties have changed
    //     during the assertion, for example coercing a value to a type.
    //     If they are not given, they are assumed to be the first and
    //     second arguments, respectively.
    //
    // The add signature below will provide the following:
    //
    // - expect
    //     expect('foo').to.equal('bar');
    //   >        ^^^^^           ^^^^^ AssertionError!
    //   > expected "foo" to equal "bar".
    //
    //     expect('foo').to.equal('bar', 'mymessage');
    //   >        ^^^^^           ^^^^^ AssertionError!
    //   > expected "foo" to equal "bar": mymessage
    //
    // - should
    //     'foo'.should.equal('bar');
    //   > ^^^^^              ^^^^^ AssertionError!
    //   > expected "foo" to equal "bar": mymessage
    //
    //   'foo'.should.equal('bar', 'mymessage');
    //   > ^^^^^              ^^^^^ AssertionError!
    //   > expected "foo" to equal "bar": mymessage
    //
    // - assert
    //     assert.equal('foo', 'bar');
    //   >              ^^^^^  ^^^^^ AssertionError!
    //   > expected "foo" to equal "bar"
    //   
    //     assert.equal('foo', 'bar', 'mymessage');
    //   >              ^^^^^  ^^^^^
    //   > expected "foo" to equal "bar": mymessage
    //
    'equal': {
      // A `params` key must always be present in an assertion. It is used to
      // duck-type over the given values. In the case of this assertion, it takes
      // "any" actual value and "any" expected value. For more complex assertions
      // they can use the predicates key to pass on particular actual/expected
      // combos, as shown further down below... 
      params: [ 'any', 'any' ],
      assert: (actual, expected) => (actual == expected),
    },

    // Plugins can override other assertions by providing more specific `params`
    // methods. By duck-typing to toggle an assertion, plugins can override
    // assertions to provide a more familiar interface, meaning less for
    // developers to lean.
    // Here is an example that could be found in a theoritical "chai-react"
    // plugin, that uses a different `equal` algorythm on two React elements
    'equal': {
      params: [ React.isElement, React.isElement ],
      assert: (actual, expected) => myReactEqualityAlgo(actual, expected),
    },

    // Properties like `.ok` are methods like any other. Interfaces (like expect)
    // can introspect the methods arity (`function.length`) to determine whether
    // or not they are property vs method assertions (`.ok` vs `.ok()`). This
    // decision will no longer be made by the plugin - so no more `addProperty`
    // vs `addMethod`
    //
    // The `ok` signature below will provide the following (note the `be` keyword
    // is automatically part of the expect/should interfaces):
    // 
    // - expect
    //     expect(false).to.be.ok();
    //   >        ^^^^^ AssertionError!
    //   > expected false to be ok
    //
    //     expect('foo').to.be.ok('mymessage');
    //   >        ^^^^^ AssertionError!
    //   > expected 'foo' to be ok. mymessage
    //
    // - expect-zen (Unary assertions become properties)
    //     expect(false).to.be.ok
    //   >        ^^^^^ AssertionError!
    //   > expected false to be ok
    //
    // - should
    //     (false).should.be.ok();
    //   >  ^^^^^
    //   > expected false to be ok
    //
    // - should-zen (Unary assertions become properties)
    //     (false).should.be.ok
    //   >  ^^^^^ AssertionError!
    //   > expected false to be ok
    //
    // - assert
    //     assert.ok(false);
    //   >           ^^^^^ AssertionError!
    //   > expected false to be ok
    //
    //   assert.ok(false, 'mymessage');
    //             ^^^^^ AssertionError!
    //   > expected false to be ok: mymessage
    //
    'ok': {
      params: [ 'any', 'any' ],
      assert: (actual, expected) => Boolean(actual),
    },

    // An assertion can also define aliases by passing an `aliases` array.
    //
    // Normally, the method name is written in a declarative style (`a`),
    // because most of the interfaces are declarative ("expect().to.be.a")
    // however, an `imperative` String property can provide a naming hint for
    // imperative interfaces (such as `assert`) to use instead. This is
    // different from `aliases` because an interface can choose to use only the
    // imperative name, or the canonical one and its aliases.
    //
    // In addition, if your assertion modifies the expected/actual values,
    // rather than just returning a boolean which could lead to an unhelpful
    // error message, you can instead pass an object with `result`, `actual`,
    // and `expected` properties - which can be used to provide more information
    // to the error messages.
    //
    // The below will provide the following:
    //
    // - expect
    //     expect('foo').to.be.a('number');
    //   >        ^^^^^          ^^^^^^^^ AssertionError!
    //   > expected "foo" to be a "number" but got "string"
    //
    //     expect('foo').to.equal('number', 'mymessage');
    //   >        ^^^^^           ^^^^^^^^ AssertionError!
    //   > expected "foo" to be a "number" but got "string": mymessage
    //
    // - should
    //     'foo'.should.be.a('number');
    //   > ^^^^^             ^^^^^^^^ AssertionError!
    //   > expected "foo" to be a "number" but got "string"
    //
    //     'foo'.should.equal('number', 'mymessage');
    //   > ^^^^^              ^^^^^^^^ AssertionError!
    //   > expected "foo" to be a "number" but got "string": mymessage
    //
    // - assert
    //     assert.type('foo', 'number');
    //   >             ^^^^^  ^^^^^^^^ AssertionError!
    //   > expected "foo" to be a "number" but got "string"
    //
    //     assert.type('foo', 'number', 'mymessage');
    //   >             ^^^^^  ^^^^^^^^ AssertionError!
    //   > expected "foo" to be a "number" but got "string"
    //
    'an': {
      aliases: [ 'a' ],
      imperative: 'type',
      params: [ 'any', 'any' ],
      assert: (actual, expected) => {
        const actualType = typeof actual;
        const expectedType = String(expected).toLowerCase();
        return {
          result: actualType != expectedType,
          actual: actualType,
          expected: expectedType,
        }
      }
    }

    // A method can also add docs and examples for even more information about
    // an assertion. Here are the mechanisms for documentation and examples:
    //  - description: a small sentence about what the assertion does
    //  - examples: an array of arguments that, when applied to the assertion
    //    method, will cause the assertion to pass
    //  - badExamples: an array of arguments that, when applied to the assertion
    //    method, will cause the assertion to fail
    //
    // The `.example` and `.badExample` calls can also be used as tests themselves,
    // meaning an assertion's code serves to be the complete reference for itself.
    // We could introduce new tooling such as theoritical "chai-plugin-test" and
    // "chai-doc-gen" libraries which could provide these functions and make
    // plugin development almost trivial.
    //
    // With these extra features, we can generate helpful error messages about
    // how to use the assertions if they are used improperly. For example if
    // a `greaterThan` assertion only takes a number, then we could have the
    // following error message, when the user passes it a string:
    //
    //   expect('foo').to.have.length.greaterThan('a string');
    // >                                          ^^^^^^^^^^ TypeError!
    // > You wrote `expect('foo').to.have.length.greaterThan('a string')` but no
    // > assertions can satisfy this. The plugin chai-operators didn't
    // > understand expected value `'a string'`. It wants the expected argument
    // > to be a Number.
    // > Here are some examples of how chai-operators `greaterThan` can be used:
    // >   ## greaterThan
    // >
    // >   Assert on the type of a given value
    // >
    // >   ### Examples
    // >     expect(10).to.be.above(5)
    // >     expect([1, 2, 3]).to.be.above(2)
    // >     expect(5).to.be.above(10)
    // >     expect('test').to.be.above(3)
    // >
    // >   ...
    // >   ### Example failures:
    // >     expect(5).to.be.above(10);
    // >   >        ^              ^^ AssertionError!
    // >   > expected 5 to be above 10
    // >
    // >     expect([1, 2, 3]).to.be.above(5);
    // >   >           (3)                 ^ AssertionError!
    // >   > expected [1, 2, 3] to have a length above 5 but got 3
    // >
    // >     expect('test').to.be.above(5);
    // >   >         (4)                ^ AssertionError!
    // >   > expected 'test' to have a length above 5 but got 3
    //
    'above': {
      aliases: ['gt', 'greaterThan'],
      description: `Asserts that the target is greater than value.`,
      params: [
        'oneOfType': [
          'number',
          'shape': {
            length: number
          }
        ],
        'number',
      }
      examples: [
        [ 10,      5  ],
        [ [1,2,3], 2  ],
        [ 'test',  3  ],
      ],
      badExamples: [
        [ 5,       10 ],
        [ [1,2,3], 5  ],
        [ 'test',  5  ],
      ],
      assert: (actual, expected) => {
        const useLength = typeof actual === 'number';
        actual =  useLength ? actual : actual.length;
        return {
          result: actual > expected,
          actual,
          expected,
        }
      },
    },
  },


  // modifiers
  // modifiers are flags that can alter the result of an assertion before
  // it is finally reported. For example `.not` is a modifier - it toggles the final
  // result of an assertion.
  // Modifiers are given the full object result of an assertion call, and get an
  // opportunity to modify the output before it is sent to chai to report on.
  // If the underlying assertion returns a boolean, then the modifier will still
  // be given the full result object (`{ result, actual, expected }`).
  // The composition of a modifier looks like:
  //    modifier(assertion(actual, expected)).
  modifiers: {

    // .not is the classic example of a modifier flag: it flips the result boolean
    // so `false` becomes `true` and `true` becomes `false`, changing the behaviour
    // of a particular assertion.
    // Because the error messages are composed of the actual code developers
    // write, no sepcial error messaging needs to be created:
    // 
    //     expect('foo').to.not.equal('foo');
    //   >        ^^^^^               ^^^^^ AssertionError!
    //   > expected "foo" to not equal "foo". They are the same value and share
    //   > the same reference.
    
    'not': (output) => {
      output.result = !output.result;
      return output;
    },

  },

  // Here is an example of an interceptor flag - something akin to Chai-Things
  // `all` assertion, or Chai-As-Promised `eventually` assertion. An interceptor
  // is passed the method that would have been called if the flag wasn't present,
  // and the arguments that were given to that. interceptors act as a kind of
  // "proxy assert" - they can change the value of any given arguments, or run
  // the assertion multiple times, or perhaps conditionally run the assertion.
  // The composition of the interceptor looks like:
  //    interceptor(assertion, actual, expected).
  interceptors: {

    // all from chai-things is an example of an interceptor that calls the desired
    // method on sub-properties of the given object (`actual`). In the case of .all,
    // it calls the assertion for every property of the array. As soon as the first
    // failing call occurs (result === false) it returns that failure.
    'all': {
      params: [ 'any', (actual) => actual instanceof Array, 'any' ],
      assert: (assertion, actual, expected) => {
        for (const element in actual) {
          let output = assertion(element, expected);
          if (output.result === false) {
            return output;
          }
        }
        return true;
      },
    },

    // deep from chai-core is an example of an interceptor that does different
    // things for different methods, as such it has many different aliases,
    // for example:
    'deep': {
      params: [ 'any', 'any', 'any' ],
      assert: (assertion, actual, expected) => {
        switch(assertion.name) {
          // deep.equal calls deep equality on a property, overriding the original
          // assertion
          case 'equal':
            return deepEqual(actual, expected);
          // deep.property traverses the object using the given string,
          // overriding the original property assertion
          case 'property'
            return getDeepProperty(actual, expected),
        }
      },
    },

  },

};
This was referenced Jan 5, 2016
@electricmonk
Copy link

As I wrote in #528 and #467, one major problem in my opinion is sharing namespaces, for instance two plugins that define a depth assertion - for instance, both a SwimmingPool and a BTree can have a depth. Currently plugins can use overwriteMethod to imperatively decide whether they support the asserted object. As soon as #467 is solved, I see no reason for any plugin to use addMethod, and as agreed with @keithamus in #117 I intend to amend the documentation to reflect that the best practice is to user overwriteMethod.

Having said that, I think that the new API should allow declaring a predicate for whether this plugin is applicable for the object under assertion:

export default {
   assertions: {
      'depth': {
         'predicate': obj => obj instanceof SwimingPool
         'assert': {...}
      }
   }
}

@keithamus
Copy link
Member Author

The intent with the above examples was that interceptors would handle "overriding" of methods, as demonstrated with the equal interceptor:

    // Interceptors can also override methods without using any flags, instead it can
    // use ducktyping to toggle an assertion (kind of like one of those Railroad
    // switches, redirecitng the assertion given a set of requirements).
    // Here is an example that could be found in a chai-react plugin, that uses a
    // different deep equality on two React elements
    'equal': (assertion, actual, expected) => {
      if (React.isElement(acutal) && React.isElement(expected)) {
        return {
          result: myReactDeepEqualityAlgo(actual, expected),
        }
      } else {
        return assertion(actual, expected);
      }
    }

Having said that, I like the idea of having a predicate based assertions - but I wonder if that is perhaps something that becomes difficult to debug:

  1. What happens if you call an assertion for something which has no matching predicates? If I call expect(thingThatMatchesNoPredicates).to.equal(foo), does it error saying that the assertion can't be executed? How do we do that in a user friendly way?
  2. Presumably if a predicate function is omitted, then that particular assertion can "handle any type" at goes to the bottom of the stack, and asserts with predicates provided go to the top? In that case, what happens when we have two methods which both omit predicates? Is one an invalid plugin?
  3. Presumably the predicate determines viability of actual? or expected? or `both?

@electricmonk
Copy link

I'm afraid that the way you described the API might be a bit too loose, so that people (like I just did!) could miss it altogether!

What happens if you call an assertion for something which has no matching predicates? If I call expect(thingThatMatchesNoPredicates).to.equal(foo), does it error saying that the assertion can't be executed? How do we do that in a user friendly way?

Well, I expect that an exception would be thrown. What happens in the solution for #467?

Presumably if a predicate function is omitted, then that particular assertion can "handle any type" at goes to the bottom of the stack, and asserts with predicates provided go to the top? In that case, what happens when we have two methods which both omit predicates? Is one an invalid plugin?

Good question. What did you imagine in your design? what you're doing, as I understand, is kind of a chain of responsibility pattern. How would it deal with a plugin with no predicate? with a conflict? seems like it's a matter of sorting the plugins, so that the first plugin registered wins.

Presumably the predicate determines viability of actual? or expected? or both? actual.expectedis created in the call site, where the caller is aware of the type ofactualso they shapeexpected` accordingly. It is, after all, their expectation :)

@keithamus
Copy link
Member Author

Well, I expect that an exception would be thrown. What happens in the solution for #467?

An exception should be thrown - but what kind of exception? How can we surface this information to the user in a way that they would understand without being a plugin author.

Good question. What did you imagine in your design?

The design I have above skirts over this issue by making the author manually override methods. Not saying this is better or worse.

what you're doing, as I understand, is kind of a chain of responsibility pattern. How would it deal with a plugin with no predicate? with a conflict? seems like it's a matter of sorting the plugins, so that the first plugin registered wins.

The problem here is that we're pushing the problem onto the user, meaning more support requests/bug reports, a requirement for better docs for every plugin. It could potentially be a big problem. Ideally we should have a solution which does not depend on fragile things like plugin order.

@electricmonk
Copy link

Eventually this is probably your decision, right? :)

I think that one of the things that really hinders progress here is the fact that expect and should are language chaining-oriented. Compare, for instance, to the API and usage of matching frameworks such as hamcrest or Specs2.

In both, additional matchers (plugins) are just functions that are passed to an assertion method, so that the only chance for a conflict would be the user actively importing two matchers with the same name - in which case the file will not compile / run.

@keithamus
Copy link
Member Author

Eventually this is probably your decision, right? :)

Hopefully not. I'm not a plugin author, nor a BDFL. I simply have the benefit of being the consumer of all issues meaning I can specify requirements for improving our plugin system. The reason this issue exists is for active bikeshedding by plugin authors so that they're getting the simplest system for writing plugins (that falls inline with some of the major issues chai faces).

In both, additional matchers (plugins) are just functions that are passed to an assertion method, so that the only chance for a conflict would be the user actively importing two matchers with the same name - in which case the file will not compile / run.

This is definitely an option. I have considered looking at enforcing name-spacing of plugins, so that conflicts can't occur, but I've steered away from that for a few reasons:

  • Root namespace becomes "first class". Chai's builtin matchers should not be more important than plugins, especially given the way some plugins are used (for example testing react components) - where users barely touch the builtins and instead 90% of their assertions are plugin methods.
  • It adds extra overhead; having to write expect(mySpyThing).to.be.a.spy.and.be.called(...) or assert.spyCalled(mySpyThing) is not very pretty, and is extra typing for sometimes no reason.

I suppose we could try to implement some kind of trait based system similar to hamcrest, it doesn't differ that much from the predicate system you proposed above, with similar pitfals.

Ultimately everything will have upsides and downsides, our rubric is:

  • Is it at least as good or better than what we have right now (in every way)
  • Have we considered enough options to reasonably assert than this one has the least downsides and/or most upsides
  • Does this option have the least number of donwsides that turn into users filing issues.

@electricmonk
Copy link

Ok. How do we make sure that all plugin authors know that this issue exists and can put in their 2 cents?

@keithamus
Copy link
Member Author

I have, in the past, @mentioned all plugin authors for sweeping chai changes (e.g. for migrating the chai plugins page), but thats kind of the nuclear option.

@keithamus
Copy link
Member Author

@vesln and @marcodejongh as you've experienced some problems with the existing plugin architecture (producthunt/chai-enzyme#11) I was hoping I could get your thoughts and opinions on the ideas expressed here.

@vesln
Copy link
Member

vesln commented Jan 22, 2016

@keithamus yep! i'm going to go through the proposal this weekend and share feedback

@keithamus
Copy link
Member Author

Awesome thanks!

@keithamus
Copy link
Member Author

Hey @vesln did you manage to get any time to look at this? I'd be really interested in hearing your feedback.

@keithamus
Copy link
Member Author

@electricmonk I think having a predicate kind of syntax is the right way to go - and pushing everything into assertions may just be the right way to go. I think if a predicate was required for every assertion, it would solve a lot of the issues. Thoughts?

@electricmonk
Copy link

@keithamus can you please elaborate with a usage example?

@keithamus
Copy link
Member Author

@electricmonk it'd be the same as you suggested here but the predicate key is a required key - and if it isn't present then Chai would throw upon loading the plugin.

I think the way we express predicates could be slightly different though - I think predicate could be an Array of either constructors or functions which match up to arguments, for example:

export default {
   assertions: {
      // This would throw an error, as it has no `predicate` key
      'equal': {
         'assert': (actual, expected) => actual === expected
      },

      // This would throw an error, as `predicate.length` does not match `assert.length`
      'equal': {
         'predicate': [Object],
         'assert': (actual, expected) => actual === expected
      },

      'equal': {
         // This predicate will match `actual instanceof Object` and `expected instanceof Object`
         'predicate': [ Object, Object ]
         'assert': (actual, expected) => actual === expected
      },

      'equal': {
          // This predicate will match `React.isElement(actual)`  and `React.isElement(expected)`
          'predicate': [ React.isElement, React.isElement ]
          'assert': (actual, expected) => myReactEqlAlgo(actual, expected)
      }
   }
}

(Having said all of that, I'd like to simplify the name predicate to something like params)

@electricmonk
Copy link

why do we need a predicate against the expected param? I'm assuming that the developer who uses the plugin, at the call site, knows which of the overloaded namespace plugins she intends to use. So the only problem we need to solve here would be which plugin, out of many plugins with the same namespace, to choose for a given actual value.

Am I making sense?

@keithamus
Copy link
Member Author

You're making sense, but I think there's maybe more utility in checking all params including expected, as they could end up calling different code paths, for example:

export default {
   assertions: {
      'an': {
         'predicate': [Object, String]
         'assert': (actual, expected) => typeof actual === expected
      },

      'an': {
         'predicate': [Object, Function]
         'assert': (actual, expected) => actual instanceof expected
      },
   }
}

This could be an if inside one function - but then we run into the same kind of problems we have right now.

@electricmonk
Copy link

fair enough.

@vesln
Copy link
Member

vesln commented Feb 6, 2016

@keithamus sorry for getting back to you that late, it's been a bit intense over here.

the proposal looks great! awesome work!

one thing I want to also focus on is the ability of modules like hippie to utilize chai.js's assertions... but only what they need - eg. hippie might implement its own error class, have different error messages etc.

i have not put too much thought into this, so i know i'm describing it super vaguely. i hope things normalize soon so i can jump in and we start the road to the next major version.

ps. i'm super impressed with your contributions so far, hats down!

@Alhadis
Copy link
Contributor

Alhadis commented Aug 23, 2016

It caters to expect and should interfaces as first class, but does not create methods for the assert interface.

Is this the reason why calling Chai.Assertion.overwriteMethod doesn't work for assert(expected, actual, "Message")?

@lucasfcosta
Copy link
Member

lucasfcosta commented Aug 23, 2016

Hi @Alhadis, thanks for your question.
I'm not sure I fully understand your doubt, but what @keithamus meant is that when using plugins to create assertions, those assertions created can only be applied when using the expect and should interfaces.

When using overwriteMethod it does not work directly for any method on the assert interface because the property you are overwriting is overwritten on the Assertion prototype, which is then used under the hood by other assert methods, as you can see here, for example. So, if you want to use overwriteMethod and the assert interface together you should see which methods from the Assertion object your assert method uses under the hood and then overwrite those methods.

Please let me know if I misunderstood your doubt or if you want to know anything else 😄

@Alhadis
Copy link
Contributor

Alhadis commented Aug 24, 2016

Ah right. No no, you understood my doubt correctly. :) Thanks for the speedy response! You guys rock.

@nathanboktae
Copy link

I like this proposal too :) Making the assertion chain context aware will be nice.

@keithamus
Copy link
Member Author

@TiraO mentioned over in #1097

It's difficult to add chainable methods that work nicely with other chainable method plugins. Using overwriteChainableMethod to overwrite 'friendly' plugins feels like a workaround. It means that your plugin has to know about and depend on any plugins that it should work with.

Right now the chaining framework can make behavior hard to understand or control as a plugin developer, which can result in unexpected behavior:

describe("when the end of the chain does not include an assertion", function () {
  it('can appear to be both true and false for the same chain', function () {
    expect({ x: 700, y: 100, z: 150 }).to.jsonEqual.roughly({ x: 700, y: 100, z: 150 });
    expect({ x: 700, y: 100, z: 150 }).to.not.jsonEqual.roughly({ x: 700, y: 100, z: 150 });
  });
});

describe("when the first method in the chain includes assertions", function () {
  it('can be accidentally and silently superceded by future assertions', function () {
    expect({ x: 700 }).to.not.jsonEqual.eql({
      toJson: function () {
        return JSON.stringify({ x: 700 })
      }
    });
  });
});

I would like to see a clearer contract for chainable methods to communicate with each other. However, it would probably require a large overhaul of the api. Here are some features I might expect:

  • allow chainable methods to wrap each other's comparators (instead of overwriting the entire assertion)
  • combine assertion messages in a human-readable way
  • raise an error when no assertions complete a chain
  • chained methods do not have their own assertions

Obviously this is a complex problem and deserves more thought than I've put into it so far.

@lucasfcosta
Copy link
Member

As we've been discussing a lot recently, we plan to make plugins a first class citizen for Chai v5 and make it easy for everyone to add plugins with assertions, modifiers and all that as easy as it is to add those things to the main library.

Due to house-cleaning purposes, I'll be closing this issue for now, but we'll definitely use this issue as reference and take into account what's been discussed here when implementing it.

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

6 participants