Add test for #9 (and deal with #16) #31
Conversation
- Confirmed that it failed by using constant window ID (á la red-green-refactor)
|
@sindresorhus Only gonna do this once to learn what you prefer. Do you want me to remind you every now and then of open PRs or just submit them and leave it up to you to eventually get to it? If you find a little poke in the side helpful, at what interval? I mean, how much time should pass from submission for a PR to be considered forgotten? Thanks! PS. While I (potentially) have your attention, here's another PR 😉 |
|
Extracted unrelated change into #32 |
|
@sindresorhus Since you merged the memory leak (#32), I'm curious: Are you not satisfied with this PR? If you don't agree with my reasoning above and insist on including a specific test for #16, I'll do that. Otherwise, this PR should be really quick to review. Nothing fancy going on. Thanks! |
|
Makes sense. Thanks 👍 And just FYI. Don't read too much into my slow replies. It has nothing to do with the quality of PRs. I just have a lot of PRs to review ;) |
|
Alright, cool stuff 😎. Yeah, I can’t even imagine 💭 how much you have on your table... Thanks for the clarification. Won’t burden you further then 😉 |
Add a test (confirmed it failed with old behavior) for #9.
I was not able to reproduce #16 by reverting to the old code. However, I think #9 actually "fixed" this as well, as each channel is now unique and thus listeners are not removed by cleaning up another channel.
On the other hand, at the time this fix was introduced, there was actually only one
callMain/Renderer()test and therefore the erroneous behavior was not caught. However, as soon as another test was added, this behavior was tested as a bonus.What I'm trying to say is: if the implementation was reverted to before #16, but the test suite was kept intact, only one
callMain/Renderer()test would pass and adding an explicit test for this wouldn't make much sense, IMO.Closes #12
IssueHunt Summary
Referenced issues
This pull request has been submitted to: