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

assert API design #142

Closed
schnittstabil opened this issue Nov 6, 2015 · 20 comments
Closed

assert API design #142

schnittstabil opened this issue Nov 6, 2015 · 20 comments

Comments

@schnittstabil
Copy link

I've just found out that the error argument of .throws is not optional

.throws(function|promise, error, [message])

and that .regexTest is not consistent with the rest of the assertions:

.regexTest(regex, contents, [message])

because of its parameter order (all other assertions follow the basic scheme (actual, expected, message)).

I'm not sure if these were intended…

@vadimdemedes
Copy link
Contributor

Could you please attach some code to both of these?

I just tested the first one and it works as expected, even when I don't set error argument:

import test from '.';

test(t => {
        t.throws(function () {
            throw new Error('damn it');
        });

        t.throws(Promise.reject(new Error('damn it')));

        t.end();
});

@schnittstabil
Copy link
Author

@vdemedes That is true, it works but:

  1. it is not documented, so the behavior might change
  2. .throws(function|promise [, error][, message]) currently would not work as expected with Add string support to throws #125:
test(t => {
    t.throws(()=>{}, 'some message');
    t.end();
});

Before #125:

  ✖ [anonymous] Missing expected exception. some message

  1 test failed

After:

  ✖ [anonymous] Missing expected exception..

  1 test failed

About .regexTest:

t.is('actual', 'expected');
t.regexTest(/expected/, 'actual');

@SamVerschueren
Copy link
Contributor

Then the notation should be something like this

.throws(function|promise [, error [, message]])

@schnittstabil
Copy link
Author

@SamVerschueren That is possible and I would recommend that, even though switching from assert.throws is no longer so simple.

@SamVerschueren
Copy link
Contributor

This is mentioned by @sindresorhus a couple of times, as well in #125.

We only use the core assert so not to duplicate code. If anything can be done better, we'll do it. Also the core Node.js people have said assert is done and won't change anymore.

That assertion module is not a binding contract. That's also the reason why AVA isn't using the same names as the assert library, to abstract away the implementation.

@schnittstabil
Copy link
Author

@SamVerschueren I knew that and I like #125, the problem is, in contrast to most assertions in AVA, the name (throws) is the same and developers who switch to AVA may experience unexpected hard to grasp behavior.

Maybe we should introduce our own throws assertions with a different name, in particular, because nodes throws and doesNotThrow suffer from many problems:

try {
    assert.throws(()=>{ throw new Error('abc')}, TypeError);
} catch (err) {
    console.log(err instanceof assert.AssertionError);
    //=> false
    console.log(err.message);
    //=> 'abc'

    // I would expect something like: 
    console.log(err.message);
    //=>'Missing expected TypeError. Got unwanted exception Error('abc').'
}

@SamVerschueren
Copy link
Contributor

Not sure about a name change, throws seems the shortest and most explanatory name for this. Something like t.exception doesn't feels right.

IMO people should read the docs, and I understand that some of them might think it has the same syntax, but then again, RTFM? ;).

I guess @sindresorhus would be happy to give his thoughts on this.

@schnittstabil
Copy link
Author

I think of more powerful assertions, maybe with different parameters. As this would also support Promises, rejects seems reasonable.

EDIT: or raises
@SamVerschueren I don't suggest to change the current name, I think we should introduce a new assertion with a new name, because the behavior is not compatible with the well known assert.throws.

@SamVerschueren
Copy link
Contributor

raises seems more appropriate.

If we go for a name change, I would not keep throws. It will confuse people because it has two methods that are almost identical, just to be consistent with assert. Again, that is not the intention of AVA (but I guess you already know :)).

It's certainly not up to me to make this decisions.

@vadimdemedes
Copy link
Contributor

Oh no, let's not change the name of throws(). rejects looks like it supports only promises and raises is from Ruby.

All we have to do, is to fix & stabilize issues pointed out in the first comment.

@sindresorhus
Copy link
Member

I would prefer not to change the name of .throws. A lot of things will differ from the core assert module. Even more when we evolve further. I think we should rather fix the issues mentioned here and document clearly that the API is not core assert compatible.

As for regexTest, that unfortunate, but honestly, do we even need it now that we have power-assert? t.true(/foobar/.test(foo)); would be just as good with the excellent output of power-assert.

@sindresorhus
Copy link
Member

As for regexTest, that unfortunate, but honestly, do we even need it now that we have power-assert?

Thoughts on deprecating regexTest?

@vadimdemedes
Copy link
Contributor

@sindresorhus I'd leave it, but rename to just t.regex. To me it looks a bit more convenient (less nesting of statements):

t.regex(/ok/, 'oh ok');
t.true(/ok/.test('oh ok'));

@SamVerschueren
Copy link
Contributor

I'm with @vdemedes on this one. Would leave it as well.

@sindresorhus
Copy link
Member

@vdemedes But the current one has the wrong argument order. I suggest we deprecate regexTest and add regex with the correct argument order then. Unless anyone has a better idea.

t.regex('oh ok', /ok/);

@vadimdemedes
Copy link
Contributor

@sindresorhus oh, in that case, definitely, regexTest should be deprecated and correct regex should be implemented.

@twada
Copy link
Contributor

twada commented Nov 16, 2015

In my opinion, reducing APIs that are easy to make mistakes is the most valuable point of power-assert.

So I'm 👍 on
t.true(/ok/.test('oh ok'));

(But for this purpose I must improve output depth of SuccinctRenderer! )

sindresorhus added a commit that referenced this issue Dec 15, 2015
As we'll probably remove or replace it.

#142 (comment)
@kasperlewau
Copy link
Contributor

Is t.regex still on the table or do we resort to using t.true with an explicit regex.test? I'd love to contribute to the cause, got the necessary changes stashed and ready for tomorrows commute!

Oh and do excuse the potential necro. Just scouring through the help wanted issues.

@jamestalmage
Copy link
Contributor

It is still on the table. Go for it!

@kasperlewau
Copy link
Contributor

It is still on the table. Go for it!

Commuting tomorrow morning is gon' be fun, cheers! : )

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

7 participants