-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Decorate failed hook titles with test title #1447
Decorate failed hook titles with test title #1447
Conversation
I'd prefer #1448 over this solution. |
While #1448 avoids throwing more logic in runner.js, I much prefer the simplicity of this PR. It doesn't switch from using a property to a getter, let alone 3 additional getters spread across runnable/hook/suite. It also means looser coupling between hooks and specs, as hooks don't need to be aware of spec titles - the runner handles that presentation. @boneskull Since it's a "short title", it only includes the spec name for which it was hopefully associated with, and not the full title (suite name and all). $ cat example.js
describe('example', function() {
beforeEach(function(done) {
setTimeout(done, 1000);
});
it('test', function(done) {
setTimeout(done, 0);
});
});
$ ./bin/mocha example.js
․
0 passing (204ms)
1 failing
1) example "before each" hook for "test":
Error: timeout of 200ms exceeded
at null.<anonymous> (/Users/danielstjules/git/mocha/lib/runnable.js:158:19)
at Timer.listOnTimeout [as ontimeout] (timers.js:110:15) +1 from me :) |
@danielstjules you'd prefer this one over #1448? |
When receiving fail emitters you do not get the file where the fail hook occurred, as you do with regular test failures. This pull mochajs#1447 handles the title issue, but the file issue remains outstanding.
Added #1745 to handle the context loss of the filename in the hook.file property. I am using this pull request 1447 and my pull request in a project with success. Seems like a simple way to solve this issue. +1 |
@jbnicolai I'm happy with either solution. It would be nice if we didn't require a getter in #1448, and the title was just another property set by the constructor. But that would mean changing the constructor's signature and injecting the ctx, which isn't an option for BC as part of this major. I think #1447, as-is, keeps things a bit simpler. #1448 results in 3 name related properties/methods: title, shortTitle, fullTitle. I feel like that's a bit more confusing. |
@boneskull Given the example, what's your take on these two proposed solutions? |
2f458ab
to
2952eca
Compare
@mochajs/mocha I say we merge this one then :) |
162b5a2
to
864ec20
Compare
good |
…each Decorate failed hook titles with test title
@duncanbeevers thanks |
Fixes #1230