search

tape-testing / tape

tap-producing test harness for node and browsers
MIT License
5.77k stars 307 forks source link

t.true() should be exact. #590

Open WORMSS opened 1 year ago

WORMSS commented 1 year ago

I know you have t.ok(true); t.ok("moose"); and that is absolutely fine to be loose. but I was wracking my brain for ages trying to work out why tests were passing, but in real environment, things were breaking..

t.true("false") should NOT pass.. That is just crazy.

Especially when you have t.equals() and t.looseEquals() So t.equals() is strict by default.. but t.true() is loose by default..

There just doesn't seem to be consistency in the api.

ljharb commented 1 year ago

true is an alias for ok, which has no strict/loose variants - it's just checking truthiness/falsiness. If you want exact, use t.equal(x, true).

WORMSS commented 1 year ago

Yes, I have since read the documentation, I believe it being an alias is a mistake and should be stopped. Yes, I did use equals, as a hacky work around, rather than a solution to the inconsistency in the api.

ok() I believe is acceptable to be loose.. true() I believe is NOT acceptable to be loose, and should no longer be an alias of ok() but it's own method to test for true

ljharb commented 1 year ago

I can certainly see your argument.

This behavior has been part of tape since its inception - added in https://github.com/ljharb/tape/commit/7948e2ee439135efee86ea0b7ce24c9deea4b018, modeled after https://www.npmjs.com/package/tap, and is how tap continues to behave.

It's not worth a breaking change just for something like this, and i think mimicking tap is a pretty important priority. Can you try making your argument there as well?

WORMSS commented 1 year ago

Funny thing, I'm looking through their docs.. and .true() and .false() don't actually appear within their asserts list.

I literally had to go into their code to even find that these aliases exist at all.

ljharb commented 1 year ago

Indeed, aliases don't seem documented at all, but they've survived many major versions none the less.

WORMSS commented 1 year ago

I pretty much summed up this thread here.

https://github.com/tapjs/node-tap/issues/861

Just for the record.

isaacs commented 1 year ago

https://github.com/tapjs/node-tap/issues/861#issuecomment-1464856705

ljharb commented 1 year ago

Seems reasonable to remove those aliases in the next semver-major, but I'm not sure when that will ever occur :-)