Set correct "thisArg" when calling nonAsync jasmine lifecycle hooks #3957
Conversation
|
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. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
jseminck
changed the title
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
| }); | ||
|
|
||
| it('contains all actions', function() { | ||
| this.x = 42; |
There was a problem hiding this comment.
I find this reseting of x a little confusing to the rest of the test, and presumably not needed. Can it be removed?
BTW Thanks for fixing this!
There was a problem hiding this comment.
Yeah! Actually, it should be removed completely. As I'd like to test that this.x has the same value that was initially set in the first beforeEach() function!
Codecov Report
@@ Coverage Diff @@
## master #3957 +/- ##
=======================================
Coverage 59.86% 59.86%
=======================================
Files 196 196
Lines 6763 6763
Branches 6 6
=======================================
Hits 4049 4049
Misses 2711 2711
Partials 3 3
Continue to review full report at Codecov.
|
|
We actually decided that we don't want to support this behavior going forward, so this is an intentional breaking change. We are currently working for a replacement for Jasmine and we are hoping to provide more and better APIs for this in the future and breaking this now will give us an opportunity to rethink some things. |
|
The only thing I found in the change-log was the following - even though there was a re-write, I would have expected things to continue working as is. ( although I now realise that if everything would continue to work as is, it wouldn't be a "breaking change" ) Edit: I saw now it was a part of jest 20.0.1: PR: #3506 + Seems I was not the first one experiencing this problem: #3553 :) -- apologies, probably should have done a better search before! We use this pattern quite a lot. I guess one option is to re-write tests by declaring the variables outside of the tests instead, as suggested in the above ticket. I think we can close this PR then. |
|
@cpojer This is pretty frustrating. People who have used Jasmine for a while do this, as this has always been the recommended pattern within it. Now we have two projects that use it throughout that will require quite a bit of tedious work. 😢 😢 😢 Next time please consider deprecating this behavior to give those who use it a chance to rewrite. 😞 Or at least a clear breaking change notice in the release notes. Just frustrated. Thanks for listening. |
|
@ndp we have >4000 unit tests and we use this pattern in almost all of our tests. Doing it manually would be very frustrating. :) I think it shouldn't be that hard to write a code-mod that fixes this though? I do not have any experience writing code-mods but have some experiences manipulating ASTs. Just need to get the code-mod dev environment set up and figured out. Edit: I started looking into code-mods. Here's a very raw example that we can play with. It just replaces To do:
|
|
For anybody running into this, here's a codemod that will get you most, if not all, of the way there: cpojer/js-codemod#81 |
|
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. |
Summary
In the latest version of Jest, the
thisArgforcallwhen calling functions in the jasmine-async file is set every time to a new object ({}).I am not sure why this was done, but this caused a regression bug for us: #3956
In the code for v19, the
thisArgis set tothis:https://github.com/facebook/jest/blob/v19.0.2/packages/jest-jasmine2/src/jasmine-async.js#L42
With this PR, I reverted back to the old code. I am not sure if the
argumentsparameter is needed either.Test plan
My repo in #3956 can help with reproducing this issue.
I'd love to add a unit test to this as well, but have no idea where to get started with that. I guess something similar to
lifecycle_hooks_testwould be needed?Edit: I added a test in
jest-jasmine2/__tests__/integration/lifecycle_hooks_test.jsto test this case. However found 2 odd things:_testand not.test. On the other hand, maybe integration tests are run somewhere separately?afterAll(() => {doesn't seem to be executed. One can modify the expected array to anything and the test will pass (I ran it just by changing the file name to end in.test.js)