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

Optional jasmine 2 support #330

Closed
wants to merge 31 commits into from

Conversation

akhomchenko
Copy link

Good day.

This patch adds optional jasmine 2.x support to jest.

Benefits:

  • bundled jasmine is not modified, it can be updated any time
  • done callback support
  • whatever you need from jasmine-2.x

TODOs:

  • make Add support for jasmine 2.2.x jeffmo/jasmine-pit#12 work, so I can remove dependency on my fork of jasmine-pit
  • extract base code for both runners
  • update matchers as I haven't tested them after update
  • fix all TODO's
  • add note about jasmine 1.x and jasmine 2.x differences

Patches are welcome.

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@rayshan
Copy link

rayshan commented Apr 24, 2015

+1

@kjs3
Copy link

kjs3 commented Apr 29, 2015

+1
Where does this stand?

@akhomchenko akhomchenko changed the title Update jasmine to 2.2.x Optional jasmine 2 support May 20, 2015
@tomv564
Copy link
Contributor

tomv564 commented May 25, 2015

@gagoman: there is a new pull request against your fork with some contributions for the open issues. Please check.

@cpojer
Copy link
Member

cpojer commented Oct 23, 2015

Alright, I finally got around to spend time on this. This looks really good, @gagoman would you mind rebasing this on top of master before we continue? I'm sorry for causing this pain :(

@DaleLJefferson
Copy link

@gagoman Based on what @cpojer has said, should we look at removing Jasmine 1 and using Jasmine 2 exclusively. Let me know if you need any help.

@cpojer
Copy link
Member

cpojer commented Oct 24, 2015

I think it is fine to add jasmine 2 support as an option. I'll think about the deprecation strategy for jasmine 1 later and when it makes sense to make jasmine 2 the default. Ideally we'll support any kind of test framework within jest in the future and we can split jasmine1 support out into a separate npm package.

@akhomchenko
Copy link
Author

@cpojer ok, I'll rebase it in a next few days

@cpojer
Copy link
Member

cpojer commented Nov 3, 2015

@gagoman did you get a chance to work on this? :)

@akhomchenko
Copy link
Author

@cpojer I'm working on this. A lot of things changed in jest since this PR was created.

@akhomchenko
Copy link
Author

@cpojer Done. Please review, as I can forgot something, especially new additions to reporters.

As for now I see one missing part:

  • noStackTraces in jasmine 2 Reporter

@cpojer
Copy link
Member

cpojer commented Nov 16, 2015

@gagoman thank you so much! I'll take a look in a bit. I'm hoping to merge this for the December release with the new resolve (jest 0.8.0).

@cpojer
Copy link
Member

cpojer commented Nov 17, 2015

Ok, as a first step I'm elevating jasmine-pit into the jest repo. There is no point in keeping it in a separate repo. Going to get code-review for this and push it out.

I need to figure out how to deal with your diff. I'd like to make some changes to your pull request but I want the sync script to properly attribute you for your diff. I might merge your diff internally and make my changes in a separate diff. Thanks for all the hard work and I'm deeply sorry that it took forever for someone at FB to take a look at this. This is not ok and will not happen again.

If you want to hang out with me and other people using jest online, feel free to join our channel on discord: http://facebook.github.io/jest/support.html :)

@cpojer
Copy link
Member

cpojer commented Nov 17, 2015

@facebook-github-bot import

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1663907390495818/int_phab to review.

@cpojer
Copy link
Member

cpojer commented Nov 17, 2015

Awesome. This almost works internally. I have a couple of small modifications to your diff and will make a cleanup diff afterwards to make all of this fit into jest better. I'll try to land all of this tomorrow.

@akhomchenko
Copy link
Author

@cpojer As for: Error: foo, case is API compatibility. So quick example:

it('should throw', function () {
    expect(function() {
      throw new Error('foo');
    }).toThrow('foo');
});

Will pass on Jasmine 1.x, but will fail on Jasmine 2.x with: Expected function to throw 'foo', but it threw Error: foo.

While:

it('should throw', function () {
  expect(function() {
    throw new Error('foo');
  }).toThrow(new Error('foo'));
});

Will pass on both versions.

Jasmine 2.x has toThrowError which is working with string, but Jasmine 1.x has no.

@cpojer
Copy link
Member

cpojer commented Nov 17, 2015

Yeah it seems like there is something else going on internally that is unrelated to your changes.

@akhomchenko
Copy link
Author

Can I help with any tasks related to this?

@cpojer
Copy link
Member

cpojer commented Nov 18, 2015

Nope, I'm almost done! Going to merge it tomorrow when I get up.

@ghost ghost closed this in 9c60191 Nov 18, 2015
@cpojer
Copy link
Member

cpojer commented Nov 18, 2015

It's happening!

@gagoman thank you for your contribution. I'm deeply sorry it took so long for this to get merged. I have taken your implementation and polished it up a little bit in a follow-up.

@akhomchenko akhomchenko deleted the update_jasmine branch November 18, 2015 21:41
cpojer added a commit that referenced this pull request Nov 18, 2015
Summary: This polishes the jasmine2 integration from the open source PR ( #330 and D2662964 ):

* Clean up the code, use const/let and classes (I think this is the last holdout in jest to use old patterns? Finally!). Also reformatted a bunch of code with an up-to-date code-style
* Add a `testRunner` option to switch to jasmine2 and resolve the `<rootDir>` correctly (slightly messy, but I'll redo configs and this is how I want it to work in the end).
* Moved a bunch of files around so they make more sense. Reorganized a bunch of stuff, this should now make it more apparent that the test runners are modular and can be switched out. In the future, we'll be able to do `npm install jest-jasmine2`, for example, to get jasmine support.
* Removed the jasmine2 boot file, all of this can be inlined.
* Changed package.json to run the jest tests both for jasmine1 and jasmine2
* Changed the CLI to add the name of the test runner
* Snuck in a small change to the moduleMocker to add WeakMap to the default unmocked collections (cc yungsters).

The plan is for jest 1.0 to switch to jasmine2 by default.

public

Reviewed By: voideanvalue

Differential Revision: D2667164

fb-gh-sync-id: 025ccf685abb550a6eaf9e68c478bc718cba546c
@cpojer
Copy link
Member

cpojer commented Nov 18, 2015

This is really annoying. Our FB GitHub bot picked one user in this PR to give attribution to because it flattens everything into one commit. @gagoman if you'll accept I would like to send you a ReactJS tshirt as a thank you for pushing this change through for this entire time. Send your tshirt size, name and address to cpojer@fb.com. I'm going to be out of the office for the next 12 days but I'll make sure that you receive this by the end of the year.

@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 14, 2021
This pull request was closed.
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.

None yet

9 participants