jestjs / jest

Delightful JavaScript Testing.
https://jestjs.io
MIT License
44.05k stars 6.44k forks source link

Human-readable context for expectations #1965

Closed timoxley closed 7 years ago

timoxley commented 7 years ago

If there's multiple expectations in a single it , currently it appears to be impossible to figure out which expectation actually failed without cross-referencing the failure with line numbers in your code.

test('api works', () => {
    expect(api()).toEqual([]) // api without magic provides no items
    expect(api(0)).toEqual([]) // api with zero magic also provides no items
    expect(api(true)).toEqual([1,2,3]) // api with magic enabled provides all items
})

Which expectation failed? The first or the second?

image

It would be nice if there were some human readable context that made it immediately clear which expectation failed and what the expectation output actually means in human terms, without having to find the line number at the top of the stack trace and mapping that back to the code.


Compare the tape equivalent below. Ignore that tape doesn't bail after the first assertion failure.tape prints out a human-readable message above each expectation failure, allowing you to know exactly which test failed without going back to the test file.

Note this also pushes the human-readable noise off to the end of line in the test source, where you might write a comment anyway.

test('api works', t => {
  t.deepEquals(api(), [], 'api without magic provides no items')
  t.deepEquals(api(0), [], 'api with zero magic also provides no items')
  t.deepEquals(api(true), [1,2,3], 'api with magic enabled provides all items')
})

image


It seems the only way to attach human-readable information to errors with jest is to wrap everything in an additional it which is unnecessarily verbose IMO.

describe('api works', () => {
  test('api without magic provides no items', () => {
    expect(api()).toEqual([])
  })
  test('api with zero magic also provides no items', () => {
    expect(api(0)).toEqual([])
  })
  test('api with magic enabled provides all items', () => {
    expect(api(true)).toEqual([1,2,3])
  })
})

Ideally, one could attach some human-readable context onto the end of the expect somehow.

e.g.

Context message as additional optional parameter for assertion methods:

test('api works', () => {
    expect(api()).toEqual([], 'api without magic provides no items')
    expect(api(0)).toEqual([], 'api with zero magic provides no items')
    expect(api(true)).toEqual([1,2,3], 'api with magic enabled provides all items')
})

Or context message as a chained .because or .why or .comment or .t or something:

test('api works', () => {
    expect(api()).toEqual([]).because('api without magic provides no items')
    expect(api(0)).toEqual([]).because('api with zero magic provides no items')
    expect(api(true)).toEqual([1,2,3]).because('api with magic enabled provides all items')
})

Alternatively, it'd be even better perhaps if jest could simply read the file and print the actual source code line that expectation itself is on.

cpojer commented 7 years ago

Hey! So we actually used to have this in Jasmine but found that over thousands of test files at FB, nobody used it. So for now we are printing a nice error message with approximate information and a stack trace that will lead to the expectation (just like in your screenshot). I agree we could print the line that throws but quite often the assertion is multiple lines long:

expect(a).toEqual({
  …
});

so this wouldn't actually look so good and we'd have to use a parser to parse the JS and extract the relevant info (and collapse long lines) or something similar to make it pretty.

Personally I think we are showing enough information for now but happy to reconsider. If you have ideas for something that isn't very complex but adds more context which helps resolve issues faster, let me know.

timoxley commented 7 years ago

we actually used to have this in Jasmine but found that over thousands of test files at FB, nobody used it

@cpojer so the pattern is to wrap each assertion in an it? and/or just trust in the line numbers?

Is it possible that this pattern has been adopted less because it's better or worse, but more just for consistency with the existing tests? or perhaps not knowing the feature exists? I didn't know this was in Jasmine.

I agree we could print the line that throws but quite often the assertion is multiple lines long

Refactoring to a single line could encourage more semantic information in the assertion? Perhaps?

const adminUser = {
  …
}
expect(a).toEqual(adminUser);

Personally I think we are showing enough information for now but happy to reconsider

The example above shows that it's difficult to discover exactly which assertion failed unless you add verbose (IMO) wrappers around everything. This is especially true in a transpiled environment where sourcemap line numbers aren't always accurate. I believe that quickly and accurately understanding broke and where is important, as are concise tests.

If you have ideas for something that isn't very complex but adds more context which helps resolve issues faster, let me know.

I made a few suggestions above:

Are you looking for something simpler, or different?

aaronabramov commented 7 years ago

hey @timoxley! we already thought about adding something like this.

so the issue with the first option is that some matchers have optional arguments, and that makes things more complicated.

e.g. here on the second case we won't know if the argument is a proximity or an error message

expect(555).toBeCloseTo(111, 2, 'reason why');
expect(555).toBeCloseTo(111, 'reason why');

second suggestion won't work because the matcher will throw as soon as something does'n meet expectation

expect(1).toBe(2)/* will throw here */.because('reason');

we could attach the reason before the matcher executes, like this:

expect(1).because('reason').toBe(2);
// or 
because('reason').expect(1).toBe(2);

but this API doesn't really look that good.

another option would be to add a second arg to expect

expect(1, 'just because').toBe(2);

but it is pretty much the same as the previous option.

cpojer commented 7 years ago

The reason why I think this isn't very useful is because engineers don't want to waste time writing tests. Anything we do to make it harder for them will just lead to worse tests.

In the past, the best solution was actually to create custom matchers. We'll introduce expect.extend in the next version of Jest and it will allow you to easily create matchers like:

expect(a).toEqualMySpecificThing(…)

which should allow you to write more expressive failure messages. I've seen this used a lot in projects like Relay. See all the matchers: https://github.com/facebook/relay/blob/master/src/tools/__mocks__/RelayTestUtils.js#L281

cpojer commented 7 years ago

Closing due to inactivity but happy to reopen if there are good ideas.

timoxley commented 7 years ago

@cpojer @dmitriiabramov apologies for delay.

A second arg to expect or chaining a reason with .because would be great. What needs to be done to make this happen or not?

timoxley commented 7 years ago

engineers don't want to waste time writing tests

@cpojer Agreed! In addition to not wanting to waste time debugging tests, this is exactly why I believe a less verbose API with more failure context would be preferable.

For some concrete numbers, using the simple example from my comment above, to get equivalent assertions + context with tape, Jest requires the programmer write nearly double the amount of ceremonial boilerplate:

This could be improved!

// tape
test('api works', t => {
  t.deepEquals(api(), [], 'api without magic provides no items')
  t.deepEquals(api(0), [], 'api with zero magic also provides no items')
  t.deepEquals(api(true), [1,2,3], 'api with magic enabled provides all items')
  t.end()
})

// jest
describe('api works', () => {
  test('api without magic provides no items', () => {
    expect(api()).toEqual([])
  })
  test('api with zero magic also provides no items', () => {
    expect(api(0)).toEqual([])
  })
  test('api with magic enabled provides all items', () => {
    expect(api(true)).toEqual([1,2,3])
  })
})

Update: I suppose you could write the jest tests on a single line with arrows:

// jest
describe('api works', () => {
  test('api without magic provides no items', () => expect(api()).toEqual([]))
  test('api with zero magic also provides no items', () => expect(api(0)).toEqual([]))
  test('api with magic enabled provides all items', () => expect(api(true)).toEqual([1,2,3]))
})

This makes for some longer lines, but does improve the stats we compared earlier somewhat:

However I think having the test description at the start of the line, without a linebreak, makes it harder to visually parse the logic because it puts the "meat" of the test, i.e. the actual assertions, at some arbitrary column position.

Parsing the code is more important than reading the test description, which is basically just a glorified comment. This is why nobody writes comments at the start of the line .e.g. this would be sadomasochistic madness:

/* api without magic provides no items */ expect(api()).toEqual([])
/* api with zero magic also provides no items */ expect(api(0)).toEqual([])
/* api with magic enabled provides all items */ expect(api(true)).toEqual([1,2,3])

Ideally all the assertion code would line up neatly in the same column so it's easily parsed by a human. Based on this thinking, I'd strongly opt for the trailing .because form rather than alternative suggestion of a second argument to expect.

cpojer commented 7 years ago

Thanks for keeping the conversation going. Note, that you also don't need the describe block, further making things smaller. .because won't work unfortunately because when the matcher throws (which happens before .because is called), we won't have a way to extract the name.

timoxley commented 7 years ago

Use the last arg of each matcher function then?

aaronabramov commented 7 years ago

It would only work if we add it as a second arg of expect. We can't add it as a the last arg for every matcher function because of ambiguity e.g.

expect(obj).toHaveProperty('a.b.c', 'is that a reason or a value of the property?');
jayphelps commented 7 years ago

Closing due to inactivity but happy to reopen if there are good ideas.

@cpojer it's unclear if the jasmine (and others) way of expect(value).toBe(something, 'because message') has been ruled out?

One example is testing redux-saga/redux-observable stuff where you're testing a state machine. It's very helpful to have a descriptive message about at what state did it fail. That example is contrived so the descriptions are as well, though..

aaronabramov commented 7 years ago

@jayphelps we're not using the jasmine way any more since we rewrote all jasmine matchers

jayphelps commented 7 years ago

@dmitriiabramov sorry my question wasn't clear. Has the jasmine way of doing it been ruled it to be added back? Doing the same thing they allow.

aaronabramov commented 7 years ago

@jayphelps as i said before, it won't work for all matchers because of the ambiguity.

expect(obj).toHaveProperty('a.b.c', 'is that a reason or a value of the property?');

and sing jest matchers can be extended with a third party packages i don't think it's a good idea to mess with the argument list

aaronabramov commented 7 years ago

the cleanest option is probably to have it as a second argument of expect, since it always takes exactly one argument.

expect(123, 'jest because').toEqual(123);

i'm not sure if we want to overload the API though. We almost never used it in facebook test suites, and for special cases i think it'm easier to just define a new test:

beforeEach(someSharedSetup);
test('reason or description', () => expect(1).toBe(1));

it's just a few lines more :)

thymikee commented 7 years ago

Or you can even put it into another describe() call.

jayphelps commented 7 years ago

@dmitriiabramov The annoying cases are when you build up state, like in a state machine for sagas, epics, etc. Each test requires the previous state changes, isolating them requires a ton of duplication without any gain AFAIK.

it('stuff', () => {
  const generator = incrementAsync();

  expect(generator.next().value).toBe(
    call(delay, 1000)
  );

  expect(generator.next().value).toBe(
    put({ type: 'INCREMENT' })
  );

  expect(generator.next()).toBe(
    { done: true, value: undefined }
  );
});

Or you can even put it into another describe() call.

Can you elaborate this? Nesting describe calls AFAIK was just for dividing section titles, tests are still run concurrently right?

thymikee commented 7 years ago

Test suites (files) run concurrently, test() calls don't.

aaronabramov commented 7 years ago

i'm starting to think that something like

test('111' () => {
  jest.debug('write something only if it fails');
  expect(1).toBe(2);
});

can be a thing

franciscop commented 7 years ago

From this discussion and this repository I think a nice and semantic one would be:

it('has all the methods', () => {
  since('cookie is a method').expect(reply.cookie).toBeDefined();
  since('download is a method').expect(reply.download).toBeDefined();
  since('end is a method').expect(reply.end).toBeDefined();
  // ...
});

The usage is similar to the because one, but it just makes more of sense semantically.

If you like this I might be able to work out a PR adding the since functionality.

nylen commented 7 years ago

Please implement an easy way to do this. I don't use it very often, but especially for more complicated tests, it's helpful to know exactly what is failing without having to go digging.

Please don't say "rewrite your test suites to be simpler". The only thing engineers hate more than writing test suites is rewriting test suites.

Another proposal that I've seen somewhere, I forget where It was in this very same issue, with an explanation of why it wouldn't work. I should probably get some sleep :)

franciscop commented 7 years ago

I got some simple "prototype" demo working, I would need to implement the recursion now. It is a thin wrapper using Proxies around the global variables and then over each method. However, Proxies are not supported by older browsers and cannot be polyfilled so it might not be acceptable for Jest. This is the general structure for the wrapper:

const since = (text) => {
  return new Proxy(global, {
    get: (orig, key) => {
      return (...args) => {
        try {
          const stack = orig[key](...args);
          return new Proxy(stack, {
            get: (orig, key) => {
              return (...args) => {
                try {
                  const ret = orig[key](...args);

                  // ... implement recursion here

                } catch (err) {
                  console.log('2', key, text, err);
                  throw err;
                }
              }
            }
          });
        } catch (err) {
          console.log('1', key, text, err);
          throw err;
        }
      };
    }
  });
};

There are three realistic options:

Edit: see it in action:

describe('Test', () => {
  it('works', () => {
    since('It fails!').expect('a').toEqual('b');
  });
});
KazimirPodolski commented 7 years ago

You need expectation context to make test results sane when you have non-trivial tests. Real-world tests wouldn't always be that simple.

Remember custom matchers - they hide mathing complexity. But when test fails, hiding this complexity is not what you want becase you want maximum info about failure. Expectation context alows you to provide this context manually. Not ideal I guess, some kind of automatic context would be better, but it is the only way I've seen by now.

When I broke something and it fails, with Jest I have to debug it manually or add logging or whatever modifications. Which is much less convenient than just look at test run results. In Jasmine for example, we have an ability to print some context to make more sense about failure. In Java's most popular test framework JUnit we have exact same feature too.

Sorry if I'm mistaken, but I don't see any technological counter-arguments to this feature here. And things like "this shouldn't be added because it won't look good" are just ridiculous.

gaearon commented 7 years ago

Can we reopen? Even jest.debug() as suggested by @aaronabramov above would be helpful to me.

ZacharyRSmith commented 6 years ago
This:

it('has all the methods', () => {
    since('cookie is a method', () => expect(reply.cookie).toBeDefined());
});

can be supported by adding this:

// setupTestFrameworkScriptFile.js
// http://facebook.github.io/jest/docs/configuration.html#setuptestframeworkscriptfile-string
global.since = (explanation, fn) => {
    try {
        fn();
    } catch(e) {
        e.message = explanation + '\n' + e.message;
        throw e;
    }
};
ZacharyRSmith commented 6 years ago

Also, jasmine-custom-message looks similar to what's requested:

describe('test', function() {
  it('should be ok', function() {
    since(function() {
      return {'tiger': 'kitty'};
    }).
    expect(3).toEqual(4); // => '{"tiger":"kitty"}'
  });
});
figalex commented 6 years ago

Are there any plans to reopen this? Seems like there has been duplicates of this issue recently. I'm also looking to display custom message when test fails.

gricard commented 6 years ago

Hmm.. This is also something I've got on my wishlist. After reading this thread, I can understand the response pertaining to Jest usage at Facebook and not wanting to impact your own workflow, but there have been some suggestions that would not interfere with existing tests, and would add the functionality that several others would like to have (myself included).

What would it take for either the 2nd arg to expect() or the since() format to be accepted as a PR? I'm willing to donate some time to help with this.

kentcdodds commented 6 years ago

I've just read through the thread and see good arguments on both sides. I definitely want a mechanism to provide a custom error message for the same reason @timoxley originally posted. Right now I'm doing something like this:

import assert from 'assert'
import chalk from 'chalk'

test('api works', () => {
  assert.deepEqual(
    api(),
    [],
    chalk.red('api without magic provides no items')
  )
  assert.deepEqual(
    api(0),
    [],
    chalk.red('api with zero magic also provides no items')
  )
  assert.deepEqual(
    api(true),
    [1, 2, 3],
    chalk.red('api with magic enabled provides all items')
  )
})

This actually works remarkably well, but I would like to avoid having to use chalk to get the red color (prints without color otherwise) and I'd prefer this to be supported by expect.

I don't really care how it's implemented honestly. But here's an alternative to since just to throw something else out there in case others prefer it:

const expectWithMessage = expect.withMessage(
  'api with magic enabled provides all items'
)
expectWithMessage(api(true)).toEqual([1, 2, 3])

// could be rewritten like
expect
  .withMessage('api with magic enabled provides all items')(api(true))
  .toEqual([1, 2, 3])

I'm not certain I like that any better than since. I'm good with whatever, I just really would love to have this :)

kentcdodds commented 6 years ago

Oh, and to address the comment:

The reason why I think this isn't very useful is because engineers don't want to waste time writing tests. Anything we do to make it harder for them will just lead to worse tests.

I agree that we don't want to make it harder to write tests. That's why this would be an additive change. So folks who don't want to "waste time" making their tests easier to debug, they can just skip on the helpful messages, but then they'll come into a codebase that has helpful messages like this and then they'll thank the engineer who took the time to explain the assertion a little bit :wink:

mj-airwallex commented 6 years ago

Hi @cpojer any updates on this?

Jest is working great for me except for this issue... At the moment I'm struggling to fix a failed assertion within a for loop like expectationsArray.forEach(expectation => expect(...))

It's hard to figure out exactly which expectations fail without a custom error message (unless I'm doing it wrong..?)

Thank you

thymikee commented 6 years ago

@mj-airwallex you're good to wrap expectations with a test in a for loop for example:

const expectationsArray = [[0, 'a'], [1, 'b']];

expectationsArray.forEach(([expectation, desc]) => {
  test(`test ${desc}`, () => {
    expect(expectation).toBeGreaterThanOrEqual(2);
  });
});
tojuhaka commented 6 years ago

I also have an issue with jest because of the need to provide custom messages during expectations. Wrapping expectations under test seems to work for cases where there is no need for asynchronous calls. But since Jest cannot handle describe(#2235 ) returning a promise we can't create tests with asynchronous calls along with wrapping them with test. And we can't have multiple test nested.

Here is an example to illustrate the problem:

async function getArray() {
  return [0,0,0,0,0,0]
}

describe('Custom messages with async', async () => {
  const array = await getArray();
  array.forEach((item) => {
    test(`test${item}`, () => {
      expect(item).toBe(0)
    });
  });
})

Any ideas how to handle this?

SimenB commented 6 years ago

Looking at the issue in the OP ("It would be nice if there were some human readable context that made it immediately clear which expectation failed"), I think it's solved now. As of Jest 22 we print the context of the failing assertion. Is the extra message still needed? If it is, it can be a code comment above or on the side of the assertion

image

Async describe is another issue (which won't be helped by the added codeframe)

kentcdodds commented 6 years ago

I think I'd not use async describe and instead use beforeEach or beforeAll

tojuhaka commented 6 years ago

@kentcdodds could you provide an example how to handle this with beforeEach or beforeAll? If you try to build all the necessary async call results in beforeEach and beforeAll it will in the end force you to create nested test which is not allowed.

kentcdodds commented 6 years ago

Ah, I missed what you were doing with that async call. Sorry about that 😅 Yeah, you couldn't do beforeEach or beforeAll to do that.

tojuhaka commented 6 years ago

@SimenB, printing the context helps a lot already and solves most of the issues with custom messages. Thanks for this! But it would be nice to have possibility for custom messages explicitly as an argument since it helps in a situations such as using expects within loops.

thymikee commented 6 years ago

@tojuhaka try this: https://github.com/negativetwelve/jest-plugins/tree/master/packages/jest-plugin-context

timoxley commented 6 years ago

Looking at the issue in the OP ("It would be nice if there were some human readable context that made it immediately clear which expectation failed"), I think it's solved now.

Yes, this does solve the original issue, so long as you have access to the original source before transpilation, which most Jest users will have. IMO it's a bit heavy-handed compared to allowing users to just print a user-supplied message with the failure, but good enough I guess.

mpseidel commented 6 years ago

Just started using Jest and I'm missing a feature like this. My use case: A test is failing where I assert a property of an object to be truthy. It would help me understand the failure faster if I could log the object in case the assertion fails.

SimenB commented 6 years ago

You can use toHaveProperty for that.

test('property', () => {
  expect({foo: 'bar'}).toHaveProperty('baz', 'foobar');
});

image

If you just want to check that it's there, drop the second argument. If you just want to assert that it has some value, you can use expect.anything(). toMatchObject is another alternative.

You can also use assert if you want.

test('property', () => {
  const obj = {foo: 'bar'};
  assert.equal(obj.baz, 'foobar', JSON.stringify(obj));
});

image

mpseidel commented 6 years ago

Thanks for the tip. assert.equal(obj.baz, 'foobar', JSON.stringify(obj)); would do the job in my particular case.

sharikovvladislav commented 6 years ago

@SimenB @mpseidel what is assert? is it some third party library? I can't find anything in jest docs.

mpseidel commented 6 years ago

@sharikovvladislav assert is a node core module https://nodejs.org/api/assert.html

sharikovvladislav commented 6 years ago

@mpseidel oops! I didn't know. Thank you. It works.

beattyml1 commented 6 years ago

I'm using the following code fragment to hack around this limitation of the framework (in TypeScript but just remove the type annotations for JS)

export const explain = (expectation: () => void, explanation: string) => {
    try {
        expectation();
    } catch(e) {
        console.log(explanation)
        throw e;
    }
}
eric-burel commented 6 years ago

Hi, I am surprised that nobody mentioned loops yet. The message would not just be a string, but a dynamic string depending on the loop iteration. jest-plugin-context is nice, thanks for this works, but it is a bit heavy and the initial issue is still relevant imo. Look at this test

describe('MyStuff', () => {
    it('should render and contain relevant inputs', () => {
      const wrapper = shallowWrapped(<MyStuff />);
      const expectedKeys = ['love','jest','but','need','details','for','expect'];
      expectedKeys.forEach((key) => {
        expect(wrapper.find({ id: key }).length).toEqual(1);
      });
    });
  });

Good luck finding your culprit. Right now I must add a line or test an object instead like {len:.., key:..}, this is not clean and not user-friendly. I think this use case is relevant, for forms and item rendering check for example. The syntax could be as simple as toEqual(1).context("my message") or toEqual(1, "my message") (though of course I know that implementation is always harder, and I respect the great job you did with Jest).

tarjei commented 6 years ago

Maybe use the same format as chai does - i.e. add the message as a second argument to the expect call:

expect(foo, 'this detail').toEqual(2)

T

akkerman commented 6 years ago

Used jasmine before, so came here to find it's not supported. However afik these things are all functions. Can we not just do something like:

describe('MyStuff', () => {
    describe('should render and contain relevant inputs', () => {
      const wrapper = shallowWrapped(<MyStuff />);
      const expectedKeys = ['love','jest','but','need','details','for','expect'];

      expectedKeys.forEach((key) => {
        it(`contains key "${key}"`, () =>
          expect(wrapper.find({ id: key }).length).toEqual(1)
        )
      })
  });
});

2018-04-18-222246_646x390_scrot