Testing module compatibility

[43081j]

At the minute we don't really seem to follow node's assertion interfaces or anyone else's.

Is there any reference as to what convention was followed or was it thought up just for deno?

It may be worth rewriting the testing module to adhere to node's assertion module and be entirely compatible.

Basically current implementation:

assertEqual(actual, expected, msg);
assert(equal(actual, expected), msg);

Node implementation:

assert.equal(actual, expected, msg);

So:

• assertEqual doesn't seem to be needed, only for setting a nice message (deepEqual and equal would do this themselves)
• equal does a deep equality check for us but in node does an Object.is alone (as deepEqual exists for the former)
• An AssertionError is thrown rather than an Error in node
• Negated functions exist in node such as notEqual

[43081j]

also, regarding #124:

Node deprecated all non-strict methods a while ago so, given we don't have the same constraints, I think it would make sense to have ours strict by default (i.e. we have equal perform strict equality and don't have a strictEqual at all or just have them alias the base functions).

The recent changes did move more toward what the node API is but still have completely different behaviour.

• deepEqual should be introduced to perform deep equality checks
• equal should do a basic strict equality check (Object.is)
• ok should be introduced which is what our assert currently does (a truthy check on !!value)
• throws should take an Error instance or an Error type (like in testing: introduce fail function #123, it should not compare messages but should perform a deep equality check if an instance is passed)
• doesNotReject and rejects should be introduced for async result assertions
• AssertionError should be introduced which is capable of producing a "nice" output (So we stop repeating ourselves in every function, building error messages)

Any value equality checks should then be done by ok(a == b, msg) if we follow node's decision of not having non-strict helpers.

[kitsonk]

Personally I would rather take queues from Chai assert personally.

The behaviour of assertEqual() and equal() is already established, both in all of std but also the core of Deno. There is a lot of tests that depend upon that deep comparison behaviour.

I don't think we should be pedantic about following anyone else's behaviour. We can take queues and patterns from others. The benefit of TypeScript and JSDoc is that the APIs can be self documenting if there is any confusion.

[43081j]

I don't think it is about being pedantic and following other behaviours for that reason, but rather that these implementations are well established and known. Any difference in behaviour will lead to confusion as nobody is starting 'new' with deno, they will have used one of the widely used assertion libraries beforehand.

equal really shouldn't do a deep equality check and restricting it to that just because it would mean many changes seems a little unfortunate/backwards. It would be worth the effort to have a clean API.

Even if we take chai as an example to work from, it too has different behaviour and is closer to what node implements.

The lack of an assertion exception is a huge one, too, as you can already see repetition in the recent PR (error logging is repeated). There should be one source of assertion messages which we can keep very clean.

I think all i listed still applies even if you follow chai, other than the naming convention and strictness.

[kitsonk]

I don't think all of your ideas apply personally, but even at that, this would be a meta issue for a lot of other issues that we could/should tackle. Having an AssertionError makes a lot of sense, and something I argued for before, especially having better diffing logic when outputting actual and expected, being able to re-root the stack trace, etc. All things that that assertion error does.

We can/should deprecate assertEqual but removing it would be a refactor of a lot of tests, including a lot in deno itself. So if someone is going to remove it, they need a PR that fixes it everywhere.

The behaviour though, while I wasn't a fan of it, is embedded in the conventions of Deno now. It has a wider view of what equal is than a lot of other assertion libraries, but that doesn't make it "wrong".

So again, I think each point could be tackled as a seperate issue.

[kitsonk]

For example, I would say rejects and doesNotReject should be throwsAsync or doesNotThrowAsync in my opinion... The Node.js APIs predate async/await and already feel a bit clunky, that is why individual points should be individual issues.

[43081j]

The reason for equal being simple (value equality check, usually) is that we can then have a deepEqual for when we want recursive equality checks. Leaving equal to do a deep check seems fine as long as we then have a strictEqual or similar, for when we don't want deep equality (which would be Object.is pretty much).

I agree with your rejects idea, too, but it could be argued that both should be supported in that case. If dealing with promises directly, it sounds logical to have rejects and resolves, but if dealing with async functions, it sounds logical to have throwsAsync and returnsAsync. Really depends on what context you're in.

Anyhow, i'm happy to open a few issues separately about these things but the idea of this issue in particular was to identify what those are.

Also, there's now equal and assert.equal being exported which can be a little confusing (one doesn't assert). It probably shouldn't be exposed anymore but again would need a bunch of updates to other modules.