Plugin support

[paulmelnikow]

As I mentioned in #26 I'm working on a POC of some tests for https://shields.io/ (see badges/shields#927).

I'd like to add plugins for:

• A custom assertion
• Setting up and cleaning up mock requests using nock
  • Probably that will look like this: .intercept(nock => nock(...).get(...))

I noticed custom plugins were mentioned in the readme. Any thoughts on how the inheritance would be implemented, or libraries with good patterns to follow? Mixins could be a way to do it.

The nock plugin would also need a before() function to register callbacks, as a counterpart to after(), which seems straightforward enough.

[paulmelnikow]

Opened #28 for before callbacks.

[paulmelnikow]

Currently after callbacks are only invoked when the request succeeds. Presumably that's to avoid spitting out more failed tests after one has already failed.

We could make after callbacks run after every test.

Alternatively, we could make a separate function to register cleanup functions like .finally.

As a developer using the library, is confusing to register after callbacks and have them not be invoked, especially given that in Mocha, after always runs, and is intended for cleanup. On that principle, changing the behavior of after may be the way to go.

[paulmelnikow]

The nicest pattern I found is this mixin pattern. It's designed for ES6 classes, and it's decorator-friendly. There's even a library that makes it really easy to follow.

If IcedFrisby is going to be maintained going forward, migrating to ES6 classes is a good idea. They're supported natively in Node 6. I doubt we really need to worry about targeting browsers. If users are on an old version of Node, they can use an older version of the library.

In the short term, I'd suggest sticking with ES5-style prototypal inheritance, while following this pattern as closely as possible. That way, it'll be an easy transition to classes.

I made a proof of concept of an "icedfrisby-nock" plugin. It relies on a hack to get access to the Frisby constructor, and would be much cleaner if the module exported that constructor instead. I'll open a PR for that. Cleanup is unreliable, as I mentioned in my comment above. For setup, it depends on #28.

[paulmelnikow]

Opened #29 to export the constructor.

[paulmelnikow]

@MarkHerhold Are you available to weigh in on significant design decisions? In particular I'd love your thoughts on when after() should be called, see my comment above #27 (comment).

[MarkHerhold]

I would tend to disagree with the notion of making after() run all the time, although I do see your point.

Here's my reasoning. Most of it boils down to user experience.

• Normally people test things that work, first and foremost. If devs have more time, they may even add tests for things that are supposed to fail. By changing the behavior of after() you are tailoring to an even more limited population of users that are expecting their first condition to fail. The rest of the users will be left wondering why the next callback has been called.
• Normally state-dependent tests rely on order. For example, a common pattern is to create some entity (POST), check that it exists (GET), then delete it (DELETE). If the create fails, then the next two are going to fail causing users to think they wrote the second two tests incorrectly before figuring out it was just the create that failed.
• This is a serious breaking change requiring a major version bump

Given this, I think .finally() is both more descriptive and doesn't cause breaking changes. 👍

---

As for the Node.js support and new features, I usually deprecate and remove support for older versions of Node.js as soon as possible because supporting older versions of everything slows the overall community down. Also, no one likes writing code for old platforms.

[paulmelnikow]

Thanks, that's super helpful. I hadn't considered chaining multiple requests together. I agree with your conclusion. It makes sense for after() to keep the same semantics as the inspections, which stop on the first failure, and to use finally() for code which always runs.

I agree with your points about backward compatibility as well. In the long run, I'd like to make the API as clear as possible and avoid the semantic clash with Mocha. Perhaps at some point we could identify a synonym, make that an alias, and eventually deprecate the original. Maybe next() would capture the idea of a sequence.

Re Node, makes sense about pushing things forward!

[paulmelnikow]

Added tests of the current after() behavior in #32.

[paulmelnikow]

Added finally() in #45.

[paulmelnikow]

Opened #46 to migrate to an ES6 class.

At that point all that's left to do here is write a bit of documentation.

My plugin is working well: https://github.com/paulmelnikow/icedfrisby-nock