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() and t.false() should be exact and not aliases of the loose t.ok() and t.notOk() #861

Closed
WORMSS opened this issue Mar 10, 2023 · 5 comments

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..

Eventually I found why my tests were not acting like I was expecting.

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.

I don't actually see t.true() at all but I spotted it in the code as being an alias of t.ok() and I believe it being an alias is a mistake and should be stopped.

I did a hacky work around, and used t.equal(result, true) but thought best to get a solution to bring consistency to the api.

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

(and same for the t.false() assert)

If the name was t.ok() and t.truthy() I wouldn't have been so confused for so long.

@isaacs
Copy link
Member

isaacs commented Mar 11, 2023

These aliases are inherited from other assertion libraries, where assert.true() was an alias for assert.ok() and had the same semantics.

In the next major version, they'll be removed. In the current one, they're already printing deprecation warnings.

So, good news! You'll get your wish! t.true('false') will throw an error. So will t.true(true) lol

@isaacs isaacs closed this as completed Mar 11, 2023
@WORMSS
Copy link
Author

WORMSS commented Mar 11, 2023

Dear lord, how deep does these aliases go? Do you know which libraries the aliases were inherited from?

@isaacs
Copy link
Member

isaacs commented Mar 11, 2023

The main things I copied from were node's original assert module, CPAN's Test::More, and jspec. Idk what those things were modeled on. Just seemed easiest at the time to let people use whatever names for assertions that they wanted. Probably some of the patterns predate JavaScript, idk. TAP itself goes back as far as Perl v1, in 1988, though there've been quite a few changes over the years.

It's actually kind of weird that t.ok() is a truthy assertion. If it was really following the esthetic of the TAP protocol, t.pass() should be called t.ok(), and t.fail() should be called t.notOk(). But at this point, I think that'd probably be too weird.

@WORMSS
Copy link
Author

WORMSS commented Mar 11, 2023

I feel fine with t.ok() being truthy, but I think of Fetch::Response::ok() as my first thought, which basically tests of the status code is 2xx or 3xx, so it's a little loose on what it considers 'ok'. I remember the bad old days where people would test for 200 only..
But, it would have been weird if Fetch::Response::200() would test for 2xx or 3xx calls..
If that makes sense?
As I said before.. If the name was t.ok() and t.truthy() I wouldn't have been so confused for so long. But true being an actual primitive, I would expect it to match the primitive.

Oh well..

@isaacs
Copy link
Member

isaacs commented Mar 11, 2023

Right, but in the TAP protocol, "ok" and "not ok" are "primitives".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants