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

after/before/afterEach/beforeEach hooks should not have a title #105

Closed
sindresorhus opened this issue Oct 25, 2015 · 13 comments · Fixed by #190
Closed

after/before/afterEach/beforeEach hooks should not have a title #105

sindresorhus opened this issue Oct 25, 2015 · 13 comments · Fixed by #190
Assignees
Labels

Comments

@sindresorhus
Copy link
Member

It doesn't make sense for them to have a title as you're just hooks.

Should we just prevent it being set in the first place?

@vadimdemedes
Copy link
Contributor

I'd handle this issue like this:

  1. beforeEach/afterEach can't have titles, never display them
  2. before/after can have titles and display them only when they exist. If before/after hook does not have a title, don't display it.

If any of the supported hook methods fail, display it like usual: x [anonymous]: error.

@sindresorhus
Copy link
Member Author

before/after can have titles and display them only when they exist. If before/after hook does not have a title, don't display it.

Why? In what cases is that needed or useful?

If any of the supported hook methods fail, display it like usual: x [anonymous]: error.

If a beforeEach/afterEach hook fails, we could show something like: beforeEach for 'test title' failed

@vadimdemedes
Copy link
Contributor

If a beforeEach/afterEach hook fails, we could show something like: beforeEach for 'test title' failed

👍

Why? In what cases is that needed or useful?

I was thinking, that having titles for beforeEach/afterEach will pollute the output, but titles for before/after won't do much noise. That's the only reason ;)

@sindresorhus
Copy link
Member Author

I was thinking, that having titles for beforeEach/afterEach will pollute the output, but titles for before/after won't do much noise. That's the only reason ;)

I figured, but I'm more interested in use-cases for when titles would be useful in before/after? If there are good use-cases, we might want to document that too.

@schnittstabil
Copy link

Currently ava supports multiple before(Each)/after(Each) hooks.
It would be convenient to know which one failed (e.g. by its title).
(And also which test triggered that hook.)

@vadimdemedes
Copy link
Contributor

I can think only of one use-case: I have many before/after hooks and having a title helps me find quicker which one failed. We can't give them friendly titles, like for beforeEach: beforeEach for "something" failed.

@sindresorhus
Copy link
Member Author

You could also look at the line number and just go to it in the test file, though.

@vadimdemedes
Copy link
Contributor

hahaha, I know that, that's why I wrote "find quicker" :D

@sindresorhus
Copy link
Member Author

How is not the line number the quickest way? With the line number you can just go directly to that line using your editor shortcut. With the title you have to search for the correct hook.

@vadimdemedes
Copy link
Contributor

I'm fine with either options, you just asked for a use-case and here it is :) Let's see what else might come up in this thread.

@schnittstabil
Copy link

How is not the line number the quickest way? With the line number you can just go directly to that line using your editor shortcut. With the title you have to search for the correct hook.

The same applies to titles of regular tests 😏, but I would vote for keeping them, at least in this case.

Considering the following, I think it would be absolutely sufficient to display the error stack, which includes the function names:

test.before(function createFixtures() {});
test.before(function createTestdir() {} );

Displaying the error stack would also indicate that the test script itself is erroneous, and not the system under test.

The same holds for beforeEach/afterEach. However, it would be helpful to know the test title. Therefore, I would suggest to wrap the Error e.g. with nested-error-stacks:

throw new NestedError(`beforeEach for '${test.title}' failed`, beforeEachError);

@vadimdemedes
Copy link
Contributor

So what's the status on this?

@sindresorhus
Copy link
Member Author

Keep title support for all hooks, but only show it on failure. I realized it can also be beneficial to have titles on them for code readability in the test files if you have many hooks. For the beforeEach/afterEach also show the connected test. (Note to self: This behavior also needs to be documented).

@schnittstabil Not a big fan of nested-error-stacks. It creates unparsable stack traces. We can still include the test title in the beforeEach/afterEach error.

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

Successfully merging a pull request may close this issue.

3 participants