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

Add testPath to jest-jasmine2 reporter callbacks #4594

Merged
merged 1 commit into from
Oct 4, 2017

Conversation

palmerj3
Copy link
Contributor

@palmerj3 palmerj3 commented Oct 3, 2017

Summary

Adds testPath to jest-jasmine2 reporter callbacks.
For jest reporter callbacks such as onTestStart we are provided with the associated file path of the test being reported on. This adds this functionality to jest-jasmine2 reporters.

My personal (and organizational) need for this is that I need a jasmine reporter that is capable of properly uniquely identifying a particular test block. I do this by utilizing both the fullName and the relative file path of the test.

For large projects it is not at all unheard of for fullName (e.g. nested test description + it block description) to not be unique.

Before (specStarted):

{
      id: 'spec0',
      description: 'should play',
      fullName: 'playlist should play',
      failedExpectations: [],
      passedExpectations: [],
      pendingReason: ''
}

After (specStarted):

{
      id: 'spec0',
      description: 'should play',
      fullName: 'playlist should play',
      failedExpectations: [],
      passedExpectations: [],
      pendingReason: '',
      testPath: '/Users/jpalmer/Desktop/test/__tests__/foo.test.js' 
}

Test plan

Ran all tests

I see there is not much in the way of tests for jasmine reporters. An integration test might be nice in this case at the top level. Let me know if you'd like me to try adding that.

@palmerj3
Copy link
Contributor Author

palmerj3 commented Oct 3, 2017

This handles my original needs mentioned in #4471

@palmerj3
Copy link
Contributor Author

palmerj3 commented Oct 4, 2017

@aaronabramov perhaps you want to take a look since you're working on some reporter stuff & things :)

@codecov-io
Copy link

Codecov Report

Merging #4594 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4594      +/-   ##
==========================================
- Coverage   55.68%   55.67%   -0.01%     
==========================================
  Files         186      186              
  Lines        6348     6349       +1     
  Branches        3        3              
==========================================
  Hits         3535     3535              
- Misses       2812     2813       +1     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-jasmine2/src/jasmine/Spec.js 0% <ø> (ø) ⬆️
packages/jest-jasmine2/src/jasmine/Env.js 0% <0%> (ø) ⬆️
packages/jest-jasmine2/src/index.js 0% <0%> (ø) ⬆️
...ackages/jest-jasmine2/src/jasmine/jasmine_light.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2bc5ad...497d6f0. Read the comment docs.

Copy link
Contributor

@aaronabramov aaronabramov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look good to me :)

@@ -34,7 +34,9 @@ async function jasmine2(
testPath,
);
const jasmineFactory = runtime.requireInternalModule(JASMINE);
const jasmine = jasmineFactory.create();
const jasmine = jasmineFactory.create({
testPath,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding this to Jasmine, could we instead patch the results object like we do for snapshots below on line 110?

See https://github.com/facebook/jest/pull/4594/files#diff-e4399d1682ddb42f8a43f1a917d42d87R110

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll give that a shot

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpojer so I think that code is what deals with jest reporters not with jasmine reporters. Jasmine reporters are what I'm trying to modify because currently jest reporters do not provide callbacks for when an individual test is going to be run. The lowest level offered for those are before a describe block is run.

@cpojer cpojer merged commit 7244b23 into jestjs:master Oct 4, 2017
@cpojer
Copy link
Member

cpojer commented Oct 4, 2017

alright.

@palmerj3 palmerj3 deleted the jasmineReporterTestPath branch October 4, 2017 11:55
@Vanuan
Copy link
Contributor

Vanuan commented Oct 5, 2017

Wow, I've just stumbled across this issue (duplicate suite names).
It's astonishing that this was merged just yesterday.
#4607

@Vanuan
Copy link
Contributor

Vanuan commented Jan 25, 2018

It looks like this is only added to the test spec. Would it be possible to add this to test suite too?

@Vanuan
Copy link
Contributor

Vanuan commented Jan 25, 2018

Please, review #5394

@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants