Skip to content

fix: global leak 'stats'#29

Merged
arturadib merged 1 commit intoshelljs:masterfrom
ando-takahiro:fix/leak_stats
Sep 24, 2012
Merged

fix: global leak 'stats'#29
arturadib merged 1 commit intoshelljs:masterfrom
ando-takahiro:fix/leak_stats

Conversation

@ando-takahiro
Copy link
Copy Markdown
Contributor

I found a small global leak 'stats' and fixed it.
Could you take a look and merge it?

Thanks!

@aeosynth
Copy link
Copy Markdown
Contributor

whoops. the shelljs tests should test for global leaks.

@arturadib
Copy link
Copy Markdown
Collaborator

thanks, nice catch.

@aeosynth - yes it'd be nice if we had such a test. ideas welcome :)

arturadib added a commit that referenced this pull request Sep 24, 2012
@arturadib arturadib merged commit 37ae50c into shelljs:master Sep 24, 2012
@aeosynth
Copy link
Copy Markdown
Contributor

ideas welcome

Use a unit testing framework. Is there a design goal to not have any
dependencies?

@arturadib
Copy link
Copy Markdown
Collaborator

which unit test framework has global pollution tests built into it?

@arturadib
Copy link
Copy Markdown
Collaborator

also it seems to me that most leaks can be caught with a simple test for change in the number of global.* methods

@aeosynth
Copy link
Copy Markdown
Contributor

I would think that most frameworks check for global pollution. I know that
mocha does.

It is easy enough to check yourself, but again, why not include a
dependency instead of creating everything from scratch?

@arturadib
Copy link
Copy Markdown
Collaborator

haven't had to create anything from scratch yet as most of our unit tests
are one-liners and the prep work would need to be done with any framework.

dependencies introduce breakages beyond our control, so there has to be a
good reason to add one.

this global check looks like just a couple/few lines, but again I'm happy
to consider introducing a framework if there's a clear need for one.

thanks,
-Artur

On Mon, Sep 24, 2012 at 1:06 PM, James Campos notifications@github.comwrote:

I would think that most frameworks check for global pollution. I know that
mocha does.

It is easy enough to check yourself, but again, why not include a
dependency instead of creating everything from scratch?


Reply to this email directly or view it on GitHubhttps://github.com//pull/29#issuecomment-8826361.

@ando-takahiro
Copy link
Copy Markdown
Contributor Author

Thanks for your quick response @aeosynth and @arturadib!
I'm using mocha, and it tells me this issue.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants