Support reporting errors in afterAll. #214
Conversation
Codecov Report
@@ Coverage Diff @@
## master #214 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 14 14
Lines 416 450 +34
Branches 55 61 +6
=====================================
+ Hits 416 450 +34
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Since these afterAll errors are not spec errors, I think we should not used the spec failed method to display them.
It generates this kind of output:
suite
✓ should pass
✗ suite
- Error: suite afterAll error
Top level suite
✗ Non-spec failure
- Error: top level suite afterAll error
I would be in favor of something like:
suite
✓ should pass
AfterAll error: suite afterAll error
AfterAll error: top level suite afterAll error
What do you think?
src/execution-metrics.ts
Outdated
| @@ -14,6 +14,7 @@ export class ExecutionMetrics { | |||
| public skippedSpecs = 0; | |||
| public totalSpecsDefined = 0; | |||
| public executedSpecs = 0; | |||
| public errors: CustomReporterResult[] = []; | |||
There was a problem hiding this comment.
In jasmine default reporter, this kind of error is specific to afterAll error, what about naming it afterAllErrors instead of errors?
Then, the summary report should also be something like * AfterAll Errors *
There was a problem hiding this comment.
I was planning to cheat and use the same mechanism in karma-jasmine, for global errors.
In karma-jasmine, global errors have to be passed from the browser back to the server, then reported via the karma-reporter. Our reporter translates karma API into jasmine calls then into jasmine-spec-reporter. The jasmineDone() api provides a path to report global errors.
That case need not concern you, so my only other question is whether there could be other errors in future? The jasmineDone()/suiteDone() reporting path is not specific to afterAll(), but then again I don't know of any other possible errors.
Not a big deal, so decide and I will follow up.
There was a problem hiding this comment.
I'm ok with this approach, I'm in favor of naming it globalErrors or globalFailures though
src/display/summary-display.ts
Outdated
|
|
||
| private errorSummary(error: CustomReporterResult, index: number): void { | ||
| this.logger.log(`${index}) ${error.fullName}`); | ||
| if (this.configuration.summary.displayErrorMessages) { |
There was a problem hiding this comment.
These errors are not related to failed assertion, I would be in favor of always displayed it for now
|
For a case like describe('an error in describe callback', () => {
it('should not pass', () => {
expect(true).toBe(true);
});
throw new Error('describe error');
});We see: Jasmine started
an error in describe callback
✓ should not pass
✗ encountered a declaration exception
- Error: describe error
**************************************************
* Failures *
**************************************************
1) an error in describe callback encountered a declaration exception
- Error: describe error
Executed 2 of 2 specs (1 FAILED) in 0.011 sec.So jasmine chose to create a new spec and attach the error to it. Does that change your thinking? Not a big deal to me, but I do think that nesting (indenting) the suite error under the suite makes sense, but it could be w/o the x for example. (I will follow up in one week after my vacation). |
|
I was not aware of this behavior. |
Starting in jasmine 2.5, errors in afterAll are reported to jasmineDone or suiteDone. Add ExecutionMetrics.globalErrors to store these errors; report them. Fixes bcaudan#210
|
I think I addressed your concerns, if not let me know. |
Starting in jasmine 2.5, errors in afterAll are reported to jasmineDone or suiteDone.
Add ExecutionMetrics.errors to store these errors; report them.
Fixes #210