Added --detailInfo switch #92
Conversation
getResultHeader is now a static method and logged items are now better grouped together visually
| @@ -190,9 +192,9 @@ function runCLI(argv, packageRoot, onComplete) { | |||
| ['Found', numMatchingTestPaths, 'matching', (numMatchingTestPaths > 1 ? 'tests...' : 'test...')].join(' ') | |||
| ); | |||
| if (argv.runInBand) { | |||
| return testRunner.runTestsInBand(matchingTestPaths, _onResultReady); | |||
| return testRunner.runTestsInBand(matchingTestPaths, _onResultReady.bind(null, argv)); | |||
There was a problem hiding this comment.
We should really have a lint for this, but internally our style guide is to keep lines at 80chars or less. Could you split this onto multiple lines?
|
I'm sorry it took so long to get back on this -- I've been moving and vacationing (and now, hitting the ground running/catching up on all the things I missed in that time). In any case, this looks great. Also, here's come context around how things are structured that are relevant to this change: One of the reasons the bin/jest.js file is mostly separate from TestRunner.js is because I wanted Jest to be built as a library...right up until the very top level where you need to actually interface with it somehow (via a CLI, in 99% of the cases). In the interest of completeness I included the bin/jest.js CLI as the default interface. However, it should still be possible (and normal) for various platforms to be able to define their own interfaces and CLIs using those libraries. For example, you could imagine a CLI that reports results through a page served up over a web server rather than through a command interface...or even one that reports results out to a back-end test result aggregation system (which is similar to what we have at FB). So with that context in mind, the goal for src/defaultTestResultHandler.js was to be be a result handler used for that default CLI (bin/jest.js). If someone needed to write their own CLI, they might use it....but they might also not use it at all (as is the case for Facebook, where we have fb-specific flags and output formats that we need to support -- as well as those test result logging/aggregation systems I mentioned a little earlier). Anyway, I think it's fine if we want to generalize Specifically, it doesn't appear that it's necessary (given what you've built here) to have a class instead of just a module that exports a simple function or two. Instead, how about just a module that exports a |
|
Hey Jeff, not a problem at all. Have been moving until today myself. Still not finished. I will revisit this soon and come up with a better structure. Exporting just the printing functions might work fine. I'll write a follow up post as soon as I've got something! |
|
ping @jeffmo |
|
@jeffmo we need this! is there anything i can do to speed up the process of this getting merged? |
|
I'm also interested in this, detailled results would be nice + it's easy to forget a .only() call and not noticing it with just the suites summary. |
|
👍 |
|
Also interested in detailed results! |
| function _onResultReady(argv, config, result) { | ||
| if (argv.detailedResults) { | ||
| return defaultTestResultHandler | ||
| .printDetailedTestResult(config, result); |
There was a problem hiding this comment.
Nit: please indent this lint. There are a bunch of other places where you don't do this when wrapping but that's really tough to read.
|
I didn't actually look at logic but I think @jeffmo did so I was just the linter. We need a rebase but otherwise I'm inclined to just take this pretty much as is with style fixes. |
|
@zpao I'm not sure whether this PR is even usable anymore? In the time that passed since the PR was issued, So I am not sure if it really makes sense to fix the identation here now?! Feedback would be appreciated. |
|
Finally this was implemented (in #370). I guess we can close this PR safely now? |
|
Closing this one, feel free to open a new issue/PR if this needs something in addition. |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
As discussed with @jeffmo this is my first stab at incorporating a switch to display more detailed information, i.e. all tests run, instead of just the final result/fail.
I also took the liberty to rewrite the test result handler for now be a configurable class, which for now only supports showing detailed information or the default output, but could be enhanced to show pending tests etc. (which is something I really want, since it's a good way to keep track of what still has to be done)
I assume this PR will be a work in progress, but I thought I'd put it out there, before continuing further to see what you guys feel about it.
With the
--defailInfoswitch enabled, a test suite could look like this: