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

Break toss() into methods and test the key behaviors of after() callbacks #32

Merged
merged 6 commits into from Apr 15, 2017

Conversation

paulmelnikow
Copy link
Collaborator

No description provided.

- Improve readability by making the methods shorter.
- Avoid using `it` as an identifier, since it is a Mocha global.
- Remove parameter to `toss()`, which is redundant with `retry()`, and
  was broken
- Remove useless assignment to `retry` variable
@paulmelnikow paulmelnikow requested a review from cvega April 12, 2017 03:08
@paulmelnikow
Copy link
Collaborator Author

There's a failing test in here, the fix for which is in #28.

@paulmelnikow paulmelnikow mentioned this pull request Apr 12, 2017
@coveralls
Copy link

coveralls commented Apr 13, 2017

Coverage Status

Coverage decreased (-0.2%) to 90.651% when pulling ea8fe2c on paulmelnikow:after-tests-rebase into 11152af on MarkHerhold:master.

@paulmelnikow
Copy link
Collaborator Author

Got 👍 offline from @cvega.

* @return {object}
* @desc Run the current Frisby test
*/
Frisby.prototype.toss = function (retry) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At present, the retry parameter is broken. Does anyone know how recently it worked? Trying to judge whether or not it's a breaking change. If it's never worked for example, then removing it would certainly not be a breaking change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite completely refactoring this library, I don't recall ever even using it or seeing it used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Let's consider this a "hygiene" change, then. We're nominally removing a parameter, but it's vestigial and doesn't do anything, so it's like a bug fix, and we'll bump the patch version.

# Conflicts:
#	lib/icedfrisby.js
@coveralls
Copy link

coveralls commented Apr 15, 2017

Coverage Status

Coverage decreased (-0.2%) to 92.475% when pulling 3e920d2 on paulmelnikow:after-tests-rebase into 2b9cf86 on MarkHerhold:master.

@paulmelnikow paulmelnikow merged commit cf2da20 into IcedFrisby:master Apr 15, 2017
@paulmelnikow paulmelnikow deleted the after-tests-rebase branch April 15, 2017 15:58
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.

None yet

3 participants