Unexpected behavior in `throws`

mindplay-dk commented 3 years ago

The method declaration of throws does not match it's behavior.

I had written quite a lot of tests like these, which were passing:

  throws(
    () => getConfig(fullyLoadedDocumentInBlockingMode),
    "must load the script from `<head>`",
    'it throws when `blockingmode="auto"` is requested, but the script was not loaded synchronously in head'
  );

Here, "must load the script from <head>" is part of the exception message I was expecting.

To my bewilderment, I discovered the tests were passing even though my expected string did not match the exception.

I went to the implementation and found that it was swapping the arguments:

https://github.com/lorenzofox3/zora/blob/073691c40f8c1378215139e58e346a2e4ef6b687/src/assertion.ts#L104-L106

I'm guessing maybe this is "leftovers" from an earlier plain JS version of Zora?

While it is possible to overload function signatures, by type-checking argument types at run-time, with TypeScript this needs to be declared using multiple function signatures. (Otherwise, you're seeing extremely misleading information in an IDE - a function signature with wrong parameter positions/names.)

But I was actually expecting an expected string value would just work - that it would check either for the contents of a thrown string, or for the message property of the Error class.

I guess this would be a breaking change though - and there are probably folks using this from plain JS, so I understand if you might not want to change this. It's a bit of a shame though. This was already my preferred pattern for testing expected exceptions in another test-framework I've been using - and the risky thing with the Typescript interfaces, is that this would appear to be correct and would appear to work, until you get suspicious enough to dig into the source.

Also, I don't understand the utility. Testing for an exception, without specifying anything about the exception you're expecting, seems like really bad practice - that exception could be anything; it could come from anywhere. The expected argument really shouldn't even be optional, in my opinion, as it's very likely to yield false positives - so long as something throws, you'll have passing tests, even if this is from internal errors your code and not from the throw you're trying to test.

Would you consider a PR to change this in the next major release? 🤔

mindplay-dk commented 3 years ago

The documentation is actually wrong on this point, too:

throws(fn: Function, expected?: string | RegExp | Function, description ?: string) expect an error to be thrown, you check the expected error by Regexp, Constructor or name

It says expected can be a string - which it can, but then it doesn't mean expected anymore. I'm also not sure what's meant by "name" here - you can pass a name? the name of what?

Since it does accept a Function, my next mistake was to assume I could pass a predicate function and check the exception myself - but the expected function is a constructor, so I should guess I should have read the manual. Who the hell reads manuals? 😅

Instead of all these confusing overloads, what do you think about a simple expected signature like Error => boolean? 🤔

If the library included some built-in predicates, this could make these assertions more flexible and readable.

For example:

t.expect(() => myFunction(), exception.thrown);

t.expect(() => myFunction(), exception.notThrown);

t.expect(() => myFunction(), exception.is(MyException));

t.expect(() => myFunction(), exception.is(Error));

t.expect(() => myFunction(), exception.isNot(SomeOtherException));

t.expect(() => myFunction(), exception.matches(/^my_pattern$/));

t.expect(() => myFunction(), exception.contains("my string"));

The exception functions would all be factory functions returning an exception? => boolean predicate.

With a new name like expect, this change would be backwards compatible.

lorenzofox3 commented 3 years ago

You are right: the doc and types seem incorrect.

The implementation is based either on expectation being constructor or regexp (the latter would match your use case I believe).

Also, I don't understand the utility. Testing for an exception, without specifying anything about the exception you're expecting, seems like really bad practice - that exception could be anything; it could come from anywhere. The expected argument really shouldn't even be optional, in my opinion, as it's very likely to yield false positives - so long as something throws, you'll have passing tests, even if this is from internal errors your code and not from the throw you're trying to test.

I personnally don't use any of these patterns, I use 99% of the time eq so I would write something similar:

test('some test', t => {
   try{
     function();
     t.fail('should have thrown');
   } catch(e){
      t.eq(e.message, 'foo');
   }
})

I think the t.throws was added as syntax sugar by a contributor.

Regarding good or bad practices, I think it is a matter of opinion but you can find an answer in the subtle nuance there is in the difference between Error an Exception. Some paradigms (and languages) ban the latter entirely and the expectation becomes therefore pointless.

Would you consider a PR to change this in the next major release

Good news, I started last week end the dev of the next major release (available in alpha with npm). You can already play with it and tell me what you think. Unfortunately the doc is still sparse and type support not there yet (the rest is mostly available).

Regarding the syntax sugar you want to add: in both version you can experiment the idea by upgrading the assertion prototype:

// before any test is called

// v4
import {AssertPrototype} from 'zora'

AssertPrototype.expect = function(actual, exepected, description){
    const result = {} // your impl here
    // don't forget to collect the result
    this.collect(result)
    return result 
}

// v5: no need to collect the result
import {Assert} from 'zora'

Assert.expect = (...args) => {
   return {
   } // your result
}

And let us know how it goes to maybe add it into the core

lorenzofox3 commented 3 years ago

By the way and out of curiosity, what reporter do you use ?

mindplay-dk commented 3 years ago

Regarding the syntax sugar you want to add: in both version you can experiment the idea by upgrading the assertion prototype

I actually tried that already, but gave up and backed out... my project is written in Typescript, and AssertPrototype has an inferred const type - so declaration merging doesn't seem to help here.

By the way and out of curiosity, what reporter do you use ?

I settled on tap-diff. I'm not always happy with the output from it - but being able to deep diff objects/values is a must for me. The built-in reporter in zora-node looks nicer - but doesn't seem to be available as a package?

lorenzofox3 commented 3 years ago

the typings should be fix: check the v5 branch (or npm i zora@next). No change in code so far

lorenzofox3 / zora

Unexpected behavior in `throws` #88