Skip to content
This repository has been archived by the owner on May 17, 2019. It is now read-only.

Official test assertion API #337

Open
lhorie opened this issue Apr 13, 2018 · 9 comments
Open

Official test assertion API #337

lhorie opened this issue Apr 13, 2018 · 9 comments

Comments

@lhorie
Copy link
Contributor

lhorie commented Apr 13, 2018

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

@KevinGrandon KevinGrandon changed the title Official test API Official test assertion API Apr 13, 2018
@lhorie
Copy link
Contributor Author

lhorie commented Apr 13, 2018

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

  • reads like english language
  • current Fusion.js test system uses jest
  • codemods from other major libs to jest already exist
  • built-in mocking (jest.fn, jest.spyOn, jest.mock)
  • documented snapshotting

cons

  • verbose assertions
  • magic globals
  • lacks ability to describe assertions (i.e. analogous to t.equal(foo, bar, 'response is correct')
  • lacks harnesses
  • googling noise (e.g. answers about .rejects.toMatch vs actual solution rejects.toThrow(regexp)

Assert-style

pros

  • terser than Jasmine style assertions
  • allows describing assertions (i.e. t.equal(foo, bar, 'response is correct'))
  • no magic globals
  • compatible with legacy projects at Uber without codemodding to a different assertion API style
  • we can control API design
  • assert docs are more thorough (includes types, deep equal semantics, etc)

cons

  • lacks counterparts to Jest idioms (e.g. expect.not.arrayContaining)
  • lacks built-in idioms for setup/teardown/mock timers/spies/etc
  • pollutes helper APIs (someHelper = t => {...} vs someHelper = () => {...}
  • lacks mocking out-of-box
  • lacks expect.assertions
  • snapshotting is monkeypatched, undocumented in fusion-test-utils
  • some features not supported in Node LTS yet (e.g. assert.rejects)

Other misc considerations

  • TAP is not console.log friendly (swallows output that starts w/ numbers)
  • stdout noise (e.g. long string comparison, multiple test failures)

@lhorie
Copy link
Contributor Author

lhorie commented Apr 13, 2018

Thumb up this comment if you prefer Jest API

@lhorie
Copy link
Contributor Author

lhorie commented Apr 13, 2018

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

@KevinGrandon
Copy link
Contributor

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

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

kevhuang commented May 1, 2018

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

kahwee commented Jul 17, 2018

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

@KevinGrandon
Copy link
Contributor

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

phou87 commented Sep 4, 2018

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 :) )

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

No branches or pull requests

6 participants