[CLOSED] Fixed leak in menu.js file. Fixes #12765

[core-ai-bot]

Issue by daytonjallen
Friday Sep 09, 2016 at 17:30 GMT
Originally opened as adobe/brackets#12767

---

Fixes adobe/brackets#12765

---

daytonjallen included the following code: https://github.com/adobe/brackets/pull/12767/commits

[core-ai-bot]

Comment by ficristo
Friday Sep 09, 2016 at 18:01 GMT

---

I know it is a bit confusing, but please, revert the changes to src/config.json.

[core-ai-bot]

Comment by daytonjallen
Friday Sep 09, 2016 at 19:18 GMT

---

I don't remember touching that file but I've reverted the changes. Sorry about that. Now the test is failing, looking into that now.

[core-ai-bot]

Comment by daytonjallen
Friday Sep 09, 2016 at 19:29 GMT

---

Checking the CI logs, I think the reason for the build failure isn't anything on my end. Right?

[core-ai-bot]

Comment by MarcelGerber
Friday Sep 09, 2016 at 19:35 GMT

---

Yes, the build failure was on our side. It passed now 👍

cc@zaggino for review

[core-ai-bot]

Comment by zaggino
Friday Sep 09, 2016 at 20:19 GMT

---

@daytonallen the code is great, but is missing a test and that should be done in this case as test for this is easy to write, please add it somewhere here: https://github.com/adobe/brackets/blob/master/test/spec/Menu-test.js#L660-L737

You should write it in a way that it fails when you comment out the removeMenuItemEventListeners(commandObj); call you added and passes when you uncomment it.

You'll need to compare contents of ._eventHandlers[eventName].length before and after calling removeMenuItem in the test.

[core-ai-bot]

Comment by daytonjallen
Friday Sep 09, 2016 at 21:37 GMT

---

How do I run the tests locally to check if it's done correctly? I tried the debug's menu run tests option but it doesn't seem to be detecting a new test case.@zaggino

[core-ai-bot]

Comment by zaggino
Friday Sep 09, 2016 at 23:58 GMT

---

@daytonallen did you setup your installed Brackets with your cloned Brackets folder like in here https://github.com/adobe/brackets/wiki/How-to-Hack-on-Brackets ? See the setup_for_hacking thingy.

[core-ai-bot]

Comment by zaggino
Friday Sep 09, 2016 at 23:59 GMT

---

If you did, please push the testcase here, maybe something else is wrong with it, you can still correct it with additional commits.

[core-ai-bot]

Comment by zaggino
Saturday Sep 10, 2016 at 03:39 GMT

---

@daytonallen you need to look for it here, it runs for me:

[core-ai-bot]

Comment by daytonjallen
Saturday Sep 10, 2016 at 05:14 GMT

---

Turns out I was testing under the wrong grouping.

[core-ai-bot]

Comment by zaggino
Saturday Sep 10, 2016 at 05:27 GMT

---

@daytonallen
You can remove stuff copied from other tests, it should be somthing simple like

menu.addMenuItem(commandId);
expect(command._eventHandlers....);
menu.removeMenuItem(commandId);
expect(command._eventHandlers....);

everything else doesn't need to be there

[core-ai-bot]

Comment by zaggino
Saturday Sep 10, 2016 at 23:16 GMT

---

Thanks@daytonallen merged to master! 👍

[core-ai-bot]

Comment by daytonjallen
Saturday Sep 10, 2016 at 23:20 GMT

---

My pleasure. Thanks for all the guidance also.