[fixes #841] ensure testEnvironment.{beforeEach,afterEach} are released #842
Conversation
foundation set to mitigate some common memory leaks, still pending: qunitjs/qunit#842
|
Thank you for the contribution. Can you please sign our CLA? http://contribute.jquery.org/CLA/ |
Sent it to legal, lets see what they have to say. @jzaefferer can you provide feedback now, I can atleast iterate to make sure this lands quickly. This leak is causing grief on many large apps, so we should try to get this in as soon as possible. |
| @@ -70,9 +70,11 @@ Test.prototype = { | |||
|
|
|||
| config.current = this; | |||
|
|
|||
| if (this.module.testEnvironment) { | |||
There was a problem hiding this comment.
Minor style issue: Missing spaces inside if. Not sure why jscs didn't catch this.
|
The actual change looks good. I can fix the style issue when landing this, if you don't get around to it. |
stefanpenner
force-pushed
the
fix-mem-leak
branch
from
6ee0583
to
ea6fbce
Compare
|
@jzaefferer style fixed, I'll push for +1 on the CLA quickly. Are we looking at a quick turn-around on release once merged? (Just gotta plan how I can roll this out in the interim.) |
|
I was looking at it, but then I had to stop for some scheduled meetings. It took me a while because I went to the original sources to take a deep look. This fix seems good to me. |
|
It also seems ok to release QUnit 1.19.0 after this. |
|
I'm ready to merge #842 I just need to confirm if you signed the CLA. Merging it, I'll release QUnit 1.19, probably on Monday morning |
|
Ya, still pending legal 👍. |
|
sorry for the delay folks, still pending legal 👍 |
|
ok, I'm holding the 1.19 release until we get the approval. |
|
Sorry about the delay, I have just gotten the legal 👍 and signed the CLA. Apparently there was a small concern about some specific verbiage, if anyone is interested I can put them in contact with them. On a positive note, anyone at Yahoo! now has 👍 to contribute. |
|
Thanks, @stefanpenner! It's merged now. |
|
sorry for the delay |
@cibernox & @stefanpenner