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

Testing for Evented module #215

Merged
merged 2 commits into from
Aug 22, 2018
Merged

Testing for Evented module #215

merged 2 commits into from
Aug 22, 2018

Conversation

chuckcarpenter
Copy link
Member

this branch adds some tests for the base class and increases code coverage

TOTAL: 21 SUCCESS
-------------|----------|----------|----------|----------|-------------------|
File         |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
-------------|----------|----------|----------|----------|-------------------|
All files    |    72.01 |    59.19 |       64 |    72.78 |                   |
 evented.js  |    84.85 |    80.77 |      100 |    84.85 |    30,31,32,33,35 |
 shepherd.js |      100 |      100 |      100 |      100 |                   |
 step.js     |    66.67 |    55.12 |       40 |    68.02 |... 84,385,400,401 |
 tour.js     |    73.81 |       50 |      100 |    73.49 |... 61,165,166,167 |
 utils.js    |    90.48 |     87.5 |      100 |    90.48 |             34,52 |
-------------|----------|----------|----------|----------|-------------------|

@@ -21,7 +21,7 @@ export class Evented {

off(event, handler) {
if (typeof this.bindings === 'undefined' || typeof this.bindings[event] === 'undefined') {
return;
return false;
Copy link
Member

Choose a reason for hiding this comment

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

Why did we change to return false here? Not saying it's wrong, just want to understand what it does for us.

Copy link
Member Author

Choose a reason for hiding this comment

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

clearer response for tests

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if maybe we should skip the return part, and just wrap the whole method in

if (!_.isUndefined(this.bindings) && !_.isUndefined(this.bindings[event])) {

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should save that for a later refactor though, since we might replace evented anyway

@RobbieTheWagner RobbieTheWagner merged commit f1d3c9a into master Aug 22, 2018
@RobbieTheWagner RobbieTheWagner deleted the feature/test-evented branch August 22, 2018 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants