Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardize test cases to follow a pattern when they can. #22

Closed
wants to merge 1 commit into from
Closed

Standardize test cases to follow a pattern when they can. #22

wants to merge 1 commit into from

Conversation

itsjamie
Copy link

No description provided.

@shama
Copy link
Owner

shama commented Jan 31, 2013

There is a bit of variance in the tests to cover the various ways gaze is instantiated. I rather not standardize this much so we can keep that coverage.

@itsjamie
Copy link
Author

Yep, I left the tests for creating it in various ways alone, I did remove the use of watcher.end though, however that could be easily added back in. I did that so that the tearDown function was what called the close method on the gaze instance in every test. It could be added back by simply wrapping the done callback in tearDown with a gaze.on('end'...

@itsjamie
Copy link
Author

A major reason why I did this, was that the test cases were not sandbox'd from each other. Even though a new instance of gaze was supposedly being created, when I did the work for #21, and was logging my patterns, I noticed that patterns from previous tests were leaking through onto the next test. The minor change to the close function fixed this, but, it encouraged me to go through and do some other minor changes to the tests.

@itsjamie
Copy link
Author

I did the merges onto my fork of the repo and ran the master branch through Travis in case the Travis builds were an issue. The results are here. I was happy when they passed on Travis, and not just my local machine :)

https://travis-ci.org/jamie-stackhouse/gaze/jobs/4503502

@shama
Copy link
Owner

shama commented Feb 1, 2013

Did you grunt on this? There are quite a few lint errors. I use the convention that if the var starts with a capital letter it means the function is a constructor. Also the tests are not passing locally on OSX for me.

I would rather not merge this. I try to only use setUp and tearDown for setting up / tearing down test conditions and not part of the actual tests. If the tests are leaking onto each we should figure out why those tests are not closing out properly instead. But I think it should all be contained within each test and avoid smoothing the entire suite.

@shama shama closed this Feb 1, 2013
@itsjamie
Copy link
Author

itsjamie commented Feb 1, 2013

The this._patterns file was not being reset as part of the close method. That is what was causing the tests to leak. As part of #21 I just added additional cleanup for close to clear all the containers.

Not a problem, it probably wasn't passing on OSX locally for you because I had a few tests in there that utilized the new exclusion behaviour. That's why I posted a link to the forked repo running on Travis because that has both pull requests merged. I'm pretty sure they would pass if you had both, my macbook running 10.8.2 they all passed locally.

Hmm..

If you don't mind, I'll go through and remove some of stylistic changes and only keep the actual test changes and resubmit. If it makes it easier I could simply submit the entire master branch of my fork, which will be these test changes + the code that passes them. The code will be linted and I'll link the travis build in the PR. Let me know what makes it easiest for you.

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.

2 participants