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

Improve the ergonomics of power-assert #1808

Closed
chocolateboy opened this issue May 19, 2018 · 20 comments
Closed

Improve the ergonomics of power-assert #1808

chocolateboy opened this issue May 19, 2018 · 20 comments

Comments

@chocolateboy
Copy link

chocolateboy commented May 19, 2018

Major +1 on promoting power-assert. If only I could do t(result === null) 😉

-- @novemberborn #663 (comment)

The last time I used AVA, I thought power-assert had been disabled by default, as I couldn't see evidence of its helpful diagnostics in the output of various failed assertions. I've only just noticed recently that this is because it's only available via t.true (edit: and t.truthy), which I don't use that often, and which is not a particularly obvious way to unlock it.

Other test frameworks force you to use an ever-growing list of assertions and matchers, but power-assert renders almost all of these methods redundant. It would be nice to bring it center stage rather than hiding it behind the facade of its less idiomatic alternatives.

I'd like to suggest two solutions to this:

  1. make power-assertions more explicit by adding an assert function:
t.assert(foo >= bar)
  1. make power-assertions more convenient by making t itself the t.assert function:
t(foo >= bar)

The former can be achieved by adding assert as an alias for true truthy (or vice versa). The latter can be achieved by making the t object a function, which is trivial if it's a plain object and not hard if it's an instance, e.g. via:

@novemberborn
Copy link
Member

Interesting. I do agree that t.true() communicates an assertion on a property value, not so much an expression result. The power-assert output is not always expected or welcomed either. I wonder if it may be an improvement to restrict power-assert to one purposefully named assertion, e.g. t.expr(foo >= bar). That restriction may also make it easier for us to integrate the desired behavior more closely with AVA itself.

@avajs/core?

@chocolateboy
Copy link
Author

@novemberborn I don't think t.expr (assert that its argument is an expression?) is clearer or more descriptive than t.assert. What other test frameworks use that?

(P.S. I think this is a feature/enhancement request rather than (or as well as) a question :-)

@novemberborn
Copy link
Member

I don't think t.expr (assert that its argument is an expression?) is clearer or more descriptive than t.assert.

As a general rule we don't alias assertions, so since we already have t.truthy() we wouldn't introduce t.assert().

However if we were to restrict power-assert to a single assertion, then perhaps t.expr() communicates "assert that the expression in the assertion call gives truthy, or else we'll print lots of detail about what was in the expression", and I think that would be sufficiently different from t.truthy() that we could introduce it.

(P.S. I think this is a feature/enhancement request rather than (or as well as) a question :-)

Yes, will add such labels when we come to a satisfactory conclusion on this.

@chocolateboy
Copy link
Author

As a general rule we don't alias assertions, so since we already have t.truthy() we wouldn't introduce t.assert().

I think there are already aliases, e.g. t.true is effectively an alias for t.is(value, true). But if this rule is really non-negotiable, why not just rename t.truthy to the standard t.assert (and t.falsy to e.g. t.refute)?

However if we were to restrict power-assert to a single assertion, then perhaps t.expr() communicates "assert that the expression in the assertion call gives truthy, or else we'll print lots of detail about what was in the expression", and I think that would be sufficiently different from t.truthy() that we could introduce it.

will add such labels when we come to a satisfactory conclusion on this.

Just to clarify a few things:

  1. There are two parts to this proposal: a more convenient API (t(...)) as well as a more explicit and descriptive API (t.assert(...)). They are (of course) the same function, so the parts are linked, but I believe both are important as far as improving the readability, usability, and discoverability of power assertions in AVA is concerned.
  2. I don't agree that t.expr is a better name for these assertions than t.assert. In fact, I think it's even less clear and less descriptive than t.true or t.truthy. Perhaps the t.expr discussion could be moved to a separate issue (e.g. a question) so that this proposal can be considered on its own merits as an enhancement?

@novemberborn
Copy link
Member

I believe the purpose of t.true() / t.false() to be providing explicit boolean comparisons. You're right that they can be distilled to t.is(), but I think they have enough value to exist on their own.

That said, I do not think they communicate power-assert, which I believe is also your motivation for opening this issue.

I'm not wedded to t.expr(), I was just raising the possibility of restricting power-assert to a particular assertion. I think that would make power-assert easier to understand, and provides a better argument for adding an additional assertion.

Without that restriction I'm not sold on the need to make any changes (documentation improvements aside). But let's hear from @vadimdemedes or @sindresorhus first.

@chocolateboy
Copy link
Author

chocolateboy commented May 20, 2018

I was just raising the possibility of restricting power-assert to a particular assertion.

I don't think that's incompatible with this proposal.

Before

t.true(...) // power assert
t.truthy(...) // power assert
t.false(...) // power assert
t.falsy(...) // power assert

After

t.assert(...) // power assert
t(...) // power assert

The implicit power-assertion reporting can be uncoupled from t.truthy etc. if there's an explicit function.

@chocolateboy
Copy link
Author

chocolateboy commented May 20, 2018

I think that would make power-assert easier to understand, and provides a better argument for adding an additional assertion.

Without that restriction I'm not sold on the need to make any changes

Since assert does what the other functions do (and more), I think it makes sense to make it explicit and convenient first and then to (re-)consider the utility of the other functions, rather than ruling it out on the grounds that other, redundant/overlapping functions already exist.

@sindresorhus
Copy link
Member

I don't think the future direction of AVA is power-assert-first, simply because I don't think power-assert is good enough in the general case. When I first created AVA, I thought power-assert was amazing, but after having used it through the years I've found it less helpful than the explicit assertions in the majority of cases.

I would personally prefer having a separate assertion method for power-assert and remove it from the others.

@novemberborn
Copy link
Member

I would personally prefer having a separate assertion method for power-assert and remove it from the others.

Great!

To recap:

We're removing power-assert from:

  • t.truthy()
  • t.falsy()
  • t.true()
  • t.false()
  • t.regex()
  • t.notRegex()

We're adding a new assertion that is power-assert enabled. I think t.assert() is sufficient here. It should behave like t.truthy().

This will require a breaking change to https://github.com/avajs/babel-preset-transform-test-files which encodes the patterns that need replacing.


@chocolateboy I think we'll want to keep t an object. That way it's easier to compose behavior on top of it without having to forward calls to t itself.

Are you interested in working on this?

@chocolateboy
Copy link
Author

We're adding a new assertion that is power-assert enabled. I think t.assert() is sufficient here. It should behave like t.truthy().

Sounds good!

I think we'll want to keep t an object. That way it's easier to compose behavior on top of it without having to forward calls to t itself.

Not sure I follow this. Do you have a particular extension/plugin in mind this change would make things harder for?

AIUI, extensions wouldn't need to concern themselves with (or even know about) the fact that t === t.assert. They'd just reference t.assert.

A function is still an (extensible) object, and this duality appears to be standard for assert, e.g. in node, commonjs-assert and power-assert itself, which describes the simplicity and convenience of this usage as a "core value":

Though power-assert is fully compatible with standard assert interface, all you need to remember is just an assert(any_expression) function in most cases.

The core value of power-assert is absolute simplicity and stability. Especially, power-assert sticks to the simplest form of testing, assert(any_expression).

Are you interested in working on this?

I'll take a look if I have time, but I'm hoping someone will beat me to it :-)

@novemberborn
Copy link
Member

I think we'll want to keep t an object. That way it's easier to compose behavior on top of it without having to forward calls to t itself.

Not sure I follow this. Do you have a particular extension/plugin in mind this change would make things harder for?

Functional composition is a big part of how people use AVA. That may include adding additional fields to t. Syntax like {...t, additional () {}} wouldn't work if t was itself a function.

Also t() is pretty obscure, compared to assert().

@chocolateboy
Copy link
Author

Syntax like {...t, additional () {}} wouldn't work

Why wouldn't that still work?

let t = () => {}
Object.assign(t, { assign () { }, truthy () { } })
let t2 = { ...t, additional: 42 }
console.log(Object.keys(t2)) // [ 'assign', 'truthy', 'additional' ]

Also t() is pretty obscure, compared to assert().

Well, that's a consequence of AVA requiring the name to be t (currently). How hard would it be to (also) allow it to be assert?

@novemberborn
Copy link
Member

Why wouldn't that still work?

t2() would fail with a t2 is not a function.

Well, that's a consequence of AVA requiring the name to be t (currently). How hard would it be to (also) allow it to be assert?

We need something reasonably unique to hook our transforms on to. We don't have good enough AST / type analysis to see which functions are receiving our execution context, so the best we can do is transform t.something() calls.

@chocolateboy
Copy link
Author

t2() would fail with a t2 is not a function.

If extensions are using a documented API to extend the t object, I expect AVA would automatically ensure the resulting object is a function.

If they're constructing new t-like objects on the fly without AVA's help, it could still easily expose whatever mechanism it uses to make the object callable e.g.:

import { callable } from 'ava'

function extend (t) {
    let t2 = { ...t, additional () { ... } }
    return callable(t2)
}

In addition to the implementations listed above, there's also the apply trap of ES6 proxies, which are fully supported (without a flag) in AVA's oldest supported Node.js version (currently v6.14.2):

function callable (value, method = 'assert') {
    // dummy target as the `apply` trap requires the
    // target to be callable
    const fn = () => {}

    return new Proxy(fn, {
        apply (target, ctx, args) {
            return value[method].apply(value, args)
        },

        get (target, name) {
            return value[name]
        }
    })
}

const value = { assert () { return [ "assert called!" ] }, foo: 42 }
const proxy = callable(value)

console.log(proxy.foo) // 42
console.log(proxy())   // [ 'assert called!' ]

@sindresorhus
Copy link
Member

Regardless whether it's possible to use t() or not, I don't think it's a good idea. I prefer t.assert, unless we can come up with a better more explicit name.

@eemed
Copy link
Contributor

eemed commented Feb 19, 2019

Hi! I would like to take a look at this. This is my first time contributing not only to this project but to any project. I will probably need some help at some point.

@novemberborn
Copy link
Member

Go for it @eemed. Don't hesitate to ask questions 👍

@eemed
Copy link
Contributor

eemed commented Feb 28, 2019

@novemberborn So it almost works, but I have 3 tests that won't pass and I can't figure it out. Should we discuss them here or should I make a pull request?

@novemberborn
Copy link
Member

A PR would be best, thanks @eemed.

@CrispusDH
Copy link
Contributor

It could be closed?

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

5 participants