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

Decorate failed hook titles with test title #1447

Merged

Conversation

duncanbeevers
Copy link
Contributor

Fixes #1230

@duncanbeevers
Copy link
Contributor Author

I'd prefer #1448 over this solution.

@boneskull boneskull added the status: waiting for author waiting on response from OP - more information needed label Dec 15, 2014
@boneskull boneskull mentioned this pull request Dec 15, 2014
@danielstjules
Copy link
Contributor

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

@jbnicolai
Copy link

@danielstjules you'd prefer this one over #1448?

askhogan pushed a commit to askhogan/mocha that referenced this pull request Jun 13, 2015
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.
@askhogan
Copy link

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

@danielstjules
Copy link
Contributor

@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.

@danielstjules
Copy link
Contributor

@boneskull Given the example, what's your take on these two proposed solutions?

@jbnicolai jbnicolai force-pushed the master branch 3 times, most recently from 2f458ab to 2952eca Compare July 5, 2015 10:25
@jbnicolai jbnicolai added TO-MERGE and removed status: waiting for author waiting on response from OP - more information needed labels Jul 5, 2015
@jbnicolai
Copy link

@mochajs/mocha I say we merge this one then :)

@duncanbeevers duncanbeevers force-pushed the descriptive-before-after-each branch from 162b5a2 to 864ec20 Compare July 5, 2015 18:09
@boneskull
Copy link
Member

good

boneskull added a commit that referenced this pull request Jul 10, 2015
…each

Decorate failed hook titles with test title
@boneskull boneskull merged commit 6d20329 into mochajs:master Jul 10, 2015
@boneskull
Copy link
Member

@duncanbeevers thanks

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

Successfully merging this pull request may close these issues.

More descriptive beforeEach/afterEach messages
5 participants