fusionjs / fusion-cli

Migrated to https://github.com/fusionjs/fusionjs
MIT License
140 stars 37 forks source link

Official test assertion API #337

Open lhorie opened 6 years ago

lhorie commented 6 years ago

It has been raised that the current test API used by Fusion.js' yarn test command is limited compared to the underlying framework (Jest). This is due to historical reasons (compatibility with older projects at Uber), but can be improved nonetheless.

The purpose of this discussion thread is to shed some light into what test assertion API flavor Fusion.js consumers prefer, and pros and cons of each API flavor

lhorie commented 6 years ago

Thoughts on API differences:

As far as Fusion.js/Uber web projects are concerned, there are two dominant styles: Jasmine-style (expect(foo).toBe(bar)) and assert-style (t.equal(foo, bar)).

Jasmine-style

pros

cons

Assert-style

pros

cons


Other misc considerations

lhorie commented 6 years ago

Thumb up this comment if you prefer Jest API

lhorie commented 6 years ago

Thumb up this comment if you prefer Assert (Tape) API

KevinGrandon commented 6 years ago

There's also potential that we can take what we have with the Assert-style helper today and make them better, or start to fold in pieces of things that the jasmine-style assertions have. E.g., today we fold in a few helpers like matchSnapshot to make life easier.

lassedamgaard commented 6 years ago

These are some things we'd like to see in the assert injectable, regardless of the underlying lib and style. This list assumes that where relevant asserts will have negative counterparts.

// Async equivalent of assert#throws. Assert that a promise is rejected with a given error type
throwsAsync: async (func: () => Promise<mixed>, error?: RegExp | Function | Object))

// Assert that actual contains expected
contains<K,V>(actual: Array<V> | Object | Map<K, V> | Set<V>, expected: V | [K, V], message?: string)

// Assert that actual contains the expected number of elements
hasLength(actual: Array | Object | Map | Set, expected: int, message?: string)

// Assert that actual is empty
isEmpty(actual: Array | Object | Map | Set, message?: string)

// Assert that actual and expected are deep equals, ignoring order
deepUnorderedEqual(actual: Array, expected: Array, message?: string)

// Assert that actual matches expected as a regular expression
matches(actual: string, expect: string | Regex)
kevhuang commented 6 years ago

+1 for @lassedamgaard's suggestion on deepUnorderedEqual with an alternative naming suggestion: sameDeepMembers.

For a similar assertion but without the deep comparison: unorderedEqual or sameMembers.

kahwee commented 6 years ago

Couldn't we already use describe, it and expect from jest? I'm not sure I follow what's missing here.

KevinGrandon commented 6 years ago

My current thoughts are that as of right now we should just use default jest globals for testing (describe, test/it, expect). I think that it's possible to make them better, but we're spread fairly thin at the moment, and the jest stuff has a large team working on it, is actively developed, and documented very well. I think that deferring to jest here for the time being is better than trying to invent and maintain a new wheel.

phou87 commented 6 years ago

Throwing in some way-too-late thoughts here:

cons

  • verbose assertions

Taking the example you cite (expect vs. t.equal) the difference is 21 vs. 17 characters, which doesn't seem like a considerable difference. Plus, if anyone really wants to use assert, they can import it directly.

  • lacks ability to describe assertions (i.e. analogous to t.equal(foo, bar, 'response is correct')

I've rarely found the need to expand beyond the default Jest descriptions and if I do, expect.extend is always there.

  • lacks harnesses

Can you give an example of where a harness would be greatly preferred over using Jest's lifecycle methods? (I'm sure they exist, I'm just not familiar with them)

  • googling noise (e.g. answers about .rejects.toMatch vs actual solution rejects.toThrow(regexp)

Unclear what this means, can you clarify?

Overall, my biggest issue with how Fusion provides utility by e.g. wrapping assert is that it doesn't add enough value to justify the additional layer of abstraction and non-standard syntax. If you strip away the comments and Flow annotations in https://github.com/fusionjs/fusion-test-utils/blob/master/src/index.js, it's pretty much 5ish lines of code. (caveat: I also never Flow annotate my test files, so maybe that value is wasted on me :) )