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

t.true() should be exact. #590

Open
WORMSS opened this issue Mar 10, 2023 · 8 comments
Open

t.true() should be exact. #590

WORMSS opened this issue Mar 10, 2023 · 8 comments
Labels
semver-major: breaking change Any breaking changes

Comments

@WORMSS
Copy link

WORMSS commented Mar 10, 2023

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
Copy link
Collaborator

ljharb commented Mar 10, 2023

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
Copy link
Author

WORMSS commented Mar 10, 2023

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
Copy link
Collaborator

ljharb commented Mar 10, 2023

I can certainly see your argument.

This behavior has been part of tape since its inception - added in 7948e2e, 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
Copy link
Author

WORMSS commented Mar 10, 2023

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
Copy link
Collaborator

ljharb commented Mar 10, 2023

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

@WORMSS
Copy link
Author

WORMSS commented Mar 10, 2023

I pretty much summed up this thread here.

tapjs/tapjs#861

Just for the record.

@isaacs
Copy link
Contributor

isaacs commented Mar 11, 2023

tapjs/tapjs#861 (comment)

@ljharb
Copy link
Collaborator

ljharb commented Mar 11, 2023

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

@ljharb ljharb added the semver-major: breaking change Any breaking changes label Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major: breaking change Any breaking changes
Projects
None yet
Development

No branches or pull requests

3 participants